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

Fix sqlite #858

Closed
wants to merge 10 commits into from
Closed

Fix sqlite #858

wants to merge 10 commits into from

Conversation

CaseyFaist
Copy link
Contributor

@CaseyFaist CaseyFaist commented Sep 27, 2019

Rebased from #812 for further work + upstreaming - see other pr for full context on approach

@CaseyFaist CaseyFaist requested a review from a team as a code owner September 27, 2019 21:59
builds/cedar-14.Dockerfile Outdated Show resolved Hide resolved
@dzuelke
Copy link
Contributor

dzuelke commented Sep 27, 2019

So right now this won't allow pip installs of packages that need the headers.

To achieve that, we need to, roughly, do this:

  1. apt-get install libsqlite3-dev in the Python formula (see https://github.com/heroku/heroku-buildpack-php/blob/343fa959d4c195beb7febe1996340624baf713b9/support/build/php#L59-L71 for a generic way of doing this)
  2. ensure realpath is also apt-get installed on cedar-14 in the Python formula (see previous point's link for the pattern)
  3. mkdir -p ${OUT_PREFIX}/include ${OUT_PREFIX}/lib/x86_64-linux-gnu
  4. cp /usr/include/sqlite3*.h ${OUT_PREFIX}/include
  5. ln -fs $(realpath /usr/lib/x86_64-linux-gnu/libsqlite3.so) ${OUT_PREFIX}/lib/x86_64-linux-gnu/libsqlite3.so

This will ensure that the libsqlite3 headers, as well as a symlink for the generic .so library (which gets resolved when compiled against) are included in the built Python tarball

Then, in bin/compile, just before pip install, we need to point the compiler infrastructure at those additional locations:

  1. export CFLAGS="-I/app/.heroku/python/include ${CFLAGS:-}"
  2. export LDFLAGS="-L/app/.heroku/lib/x86_64-linux-gnu ${LDFLAGS:-}"

This should then allow building e.g. a pysqlite dependency in requirements.txt.

@CaseyFaist CaseyFaist mentioned this pull request Sep 30, 2019
@CaseyFaist
Copy link
Contributor Author

CaseyFaist commented Sep 30, 2019

Capturing for future reference:

The reason for the shift in approach is that the apt-buildpack won't load headers properly given the package is still on the platform and ONLY the sqlite headers are not, and thus would not allow users of pysqlite or other utilities to use sqlite on runtime. We're opting to preserve this functionality instead, with added complexity as the tradeoff 🎉

@CaseyFaist CaseyFaist changed the title [WIP] Fix sqlite Fix sqlite Sep 30, 2019
@CaseyFaist
Copy link
Contributor Author

Also important: the c and ld flags will not be updated in this pr, but a future one when this functionality is flipped on. That's a potentially breaking change, so slicing this up there unblocks the rest of this work without causing breakage.

@CaseyFaist CaseyFaist closed this Mar 18, 2020
@edmorley edmorley deleted the fix-sqlite branch July 30, 2020 10:03
cybniv added a commit to cybniv/heroku-buildpack-python that referenced this pull request Sep 15, 2020
dm.xmlsec.binding, which comes as a dep for python-saml,
does not compile on Heroku, using this buildpack, because
apparently /usr/include is not available at that time.

After some research, it seems that is known behavior and the reason
why workarounds for some packages (e.g.sqlite [1]) exist.

Hence it may be easiest to simply do something very similiar for
libxmlsec1, hopefully without breaking anything else.

[1] heroku#858
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.

None yet

3 participants