Skip to content

sqlite3 headers #1049

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

Merged
merged 4 commits into from
Apr 4, 2021
Merged

sqlite3 headers #1049

merged 4 commits into from
Apr 4, 2021

Conversation

drewgilliam
Copy link
Contributor

Previous versions of manylinux2010 included sqlite3 files that allowed downstream dockers to build against the recent version of sqlite3 included in the docker image. Files include:

  • headers /usr/local/include/{sqlite3.h,sqlite3ext.h}
  • pkgconfig file /usr/local/lib/pkgconfig/sqlite3.pc
  • libtool file /usr/local/lib/libsqlite3.la
  • .so file /usr/local/lib/libsqlite3.so

PR #972 eliminated these files during cleanup. This PR restores the above files (which have a relatively small footprint).

@drewgilliam
Copy link
Contributor Author

@auvipy - thanks for the approval! Not sure what to do about the failing Travis build, which appears to fail way before it gets to the SQLite install. I know quay.io was down right when I submitted the PR, not sure if that mattered here. Any suggestions?

@mayeut
Copy link
Member

mayeut commented Mar 27, 2021

Hello @drewgilliam,

Thanks for the PR. Indeed there was a regression here.

While waiting for nealef/clefos#7 resolution (will fix the s390x failure in CI), would you be willing to also contribute a small test to ensure this does not happen again ?
All tests were moved in the tests folder. Something really small would do just fine, if possible, using all files that have been added back.

@drewgilliam
Copy link
Contributor Author

drewgilliam commented Mar 28, 2021

@mayeut - I added a simple ctest structure based on the CMake unit test for FindSQLite3
https://gitlab.kitware.com/cmake/cmake/-/tree/master/Tests/FindSQLite3/Test

This should test the intent of the PR, being able to compile & run against sqlite3. The simple ctest system can also be easily extended with other similar tests in the future.

Note I also added sqlite3 --version to the minimal test section of run_tests.sh which required keeping the /usr/local/bin/sqlite3 executable around. FYI, it appears there may be a competing version of sqlite3 in some docker images (e.g., /usr/bin/sqlite3 with a different version) though /usr/local/bin/sqlite3 should be favored by the path.

Copy link
Member

@mayeut mayeut left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI, it appears there may be a competing version of sqlite3 in some docker images (e.g., /usr/bin/sqlite3 with a different version) though /usr/local/bin/sqlite3 should be favored by the path.

This might also be the case for other tools & I'd like to address this potential issue at some point with a better check than just printing out the version (which only ensures that some version of the tool is in the PATH).
For now this will be enough given /usr/local/bin is before /usr/bin in the PATH.

Test for exact sqlite3 version

Co-authored-by: Matthieu Darbois <[email protected]>
@drewgilliam drewgilliam requested a review from mayeut April 3, 2021 22:25
@mayeut mayeut merged commit eb65a12 into pypa:master Apr 4, 2021
@mayeut
Copy link
Member

mayeut commented Apr 4, 2021

Thanks @drewgilliam

@drewgilliam drewgilliam deleted the sqlite3_headers branch April 4, 2021 19:19
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

Successfully merging this pull request may close these issues.

3 participants