Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Test for self.assertEqual(has_full, hasattr(c, "description_full")) fails #363

Closed
mgorny opened this issue Sep 5, 2022 · 13 comments
Closed

Comments

@mgorny
Copy link

mgorny commented Sep 5, 2022

It seems that the newly added test fails on my system:

$ python setup.py test
running test
                Python  /tmp/apsw/.venv/bin/python sys.version_info(major=3, minor=10, micro=6, releaselevel='final', serial=0)
Testing with APSW file  /tmp/apsw/.venv/lib/python3.10/site-packages/apsw/__init__.cpython-310-x86_64-linux-gnu.so
          APSW version  3.39.2.1
    SQLite lib version  3.39.2
SQLite headers version  3039002
    Using amalgamation  False
.......................F........................................................................
======================================================================
FAIL: testCursor (apsw.tests.APSW)
Check functionality of the cursor
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/tmp/apsw/.venv/lib/python3.10/site-packages/apsw/tests.py", line 778, in testCursor
    self.assertEqual(has_full, hasattr(c, "description_full"))
AssertionError: True != False

----------------------------------------------------------------------
Ran 96 tests in 31.321s

FAILED (failures=1)
@stuertz
Copy link

stuertz commented Sep 5, 2022

Same here: conda-forge/apsw-feedstock#38

@rogerbinns
Copy link
Owner

The test is looking for the SQLite compile option ENABLE_COLUMN_METADATA. What has happened here is that SQLite was compiled with that option, but apsw was not. APSW needs to see the same SQLite compile options in order to make matching APIs. Another example is extension loading functionality. (It isn't possible to do this at runtime since symbols are or aren't present based on the defines.)

Possible resolutions:

  1. Update the APSW test so that it only looks at the compile option when the amalgamation is in use, in which case there can't be mismatches

  2. Conda (and others) have to update their apsw build configuration so that the same flags are passed to apsw as to sqlite. Note that unless this is done, apsw won't expose the attribute that depends on ENABLE_COLUMN_METADATA being defined. Sadly pkg-config isn't useful.

  3. When not using the amalgamation, have apsw's setup.py dynamically load libsqlite3 and try to get flags from it

If think 3 will be too error prone since libsqlite3 could be anywhere on the system. I have no control over 2. I can do 1. Any suggestions or desired outcomes?

rogerbinns added a commit that referenced this issue Sep 5, 2022
@rogerbinns
Copy link
Owner

That commit has a one line change to tests.py that implements option 1 above.

@stuertz
Copy link

stuertz commented Sep 5, 2022

Thanks for the hint with ENABLE_COLUMN_MENTADATA, I enabled that for the conda build and it seems to work fine.

@rogerbinns
Copy link
Owner

SQLite 3.39.3 just came out - less than 24 hours after my release! I will do a corresponding APSW in 6 days which also includes the tests change.

@mgorny
Copy link
Author

mgorny commented Sep 5, 2022

Thanks for the explanation. I've added --enable=column_metadata and it fixed the issue indeed. It's unfortunate that we have to keep these defines in sync between sqlite and apsw, though.

rogerbinns added a commit that referenced this issue Sep 5, 2022
@rogerbinns
Copy link
Owner

It is really annoying that the SQLite compile time options have to be known at APSW compile time in order to match functionality. If sqlite functions ended up as null pointers or returned not implemented, it would be so much easier ...

@mgorny @stuertz I may be able to make this easier for you, if you can do a test for me. In particular you need to know where the SQLite shared library is on the filesystem at the time you compile APSW. Try running https://github.com/rogerbinns/apsw/blob/master/tools/sqliteliboptions.py with the library filename and you should get a long list of #undef SYMBOL / #define SYMBOL VALUE. I've tested on Mac, Windows, and Linux.

If it works, I'll update setup.py and you would use it something like this:

setup.py gen_sqlite_config /path/to/libsqlite3.so build [normal options]

ie a subcommand like gen_sqlite_config takes the shared library and generates a header file sqlite3/sqlite3config.h which is automatically #included if it exists during build.

@thesamesam
Copy link

Hi @rogerbinns, I hail from Gentoo (whence mgorny came) and we've been struggling with https://bugs.gentoo.org/851741 which I think is related.

I do indeed get a long list!

@rogerbinns
Copy link
Owner

@thesamesam thanks for the heads up, and it is indeed the same issue. Note that the default for SQLite itself is for extension loading to be disabled (not compiled in) which is why APSW does the same, and the enable flag needs to be used.

You will indeed get a long list of the SQLITE_ defines most of which aren't relevant (eg default sector size). However I chose not to filter the list since the extras will not hurt, and every SQLite release adds more.

The plan is (hashed out in #364) to make another release this weekend (SQLite 3.39.3 has come out hence the need for folks relying on pypi binary wheels) which will have an extra gen_config command. Today you are doing something approximately like this:

python setup.py build --enable foo,bar

Instead you will do something like this:

python setup.py gen_config --system build

gen_config --system will extract the system SQLite library flags into the file sqlite3/sqlite3config.h and the build step already automatically includes that file. Note that you should no longer need to specify enables/omit/disables at all with this change.

Sound good?

@thesamesam
Copy link

Thanks! This sounds good, although I'm wondering if it's going to be a problem to use a custom target (gen_config) as we're currently doing a PEP517 build. Like you, I've struggled with figuring out what the right way to do certain things is in the new PEP517 world.

I think this should work, but I'll defer to @mgorny in case he's got another plan in mind.

@mgorny
Copy link
Author

mgorny commented Sep 7, 2022

I think it'll work if we only make sure to call setup.py gen_config first.

rogerbinns added a commit that referenced this issue Sep 8, 2022
@rogerbinns
Copy link
Owner

Thanks for all the feedback. While gen_config appeals as a programmer (keep separate things separate), it is also the wrong choice because build_ext is what needs to know about it, and it needs to be easier to morph to new packaging standards. Consequently I implemented it as build_ext --use-system-sqlite-config. You should be able to add that flag to your existing command line and all is good. (Actual implementation here).

I'd appreciate if you can test with it. You only need the updated setup.py from git. Hopefully you aren't doing this!

@rogerbinns
Copy link
Owner

This was shipped yesterday as 3.39.3.0, so I am calling this done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants