Fix edge case when building packages with optional c extensions #421
Conversation
The edge case occurs when a package can compile optional c speedups which succeed during the build process but lock the generated wheel file to the arch the package was built on. This makes the wheel file incompatible with lambda and causes it to fail to add that dependency even though it could have by building it without the optional c extensions. This is a bit tricky to fix as means we need to selectivly break the ability to compile c extensions during some runs of our wheel buiding phase. It is also quite differnet on POSIX and Windows.
Codecov Report
@@ Coverage Diff @@
## master #421 +/- ##
=========================================
- Coverage 93.54% 92.94% -0.6%
=========================================
Files 18 18
Lines 2805 2850 +45
Branches 369 374 +5
=========================================
+ Hits 2624 2649 +25
- Misses 133 150 +17
- Partials 48 51 +3
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Looks reasonable given what we have to work with.
I think we should start adding integration tests for the packager. Something where we give it various requirements.txt files and verify it does the right thing. I get that it it'll vary per platform but I think the extra safety net would be a good thing. We can address that as a separate PR if you want.
chalice/compat.py
Outdated
| if os.name == 'nt': | ||
| # windows | ||
| # On windows running python in a subprocess with no environment variables | ||
| # will cause sevral issues. In order for our subprocess to run normally we |
chalice/compat.py
Outdated
| if 'SYSTEMROOT' in os.environ: | ||
| subprocess_python_base_environ['SYSTEMROOT'] = os.environ['SYSTEMROOT'] | ||
|
|
||
| # This is the acutal patch used on windows to prevent distutils from |
chalice/compat.py
Outdated
| # This is the acutal patch used on windows to prevent distutils from | ||
| # compiling C extensions. The msvc compiler base class has its compile | ||
| # method overridden to raise a CompileError. This can be caught by | ||
| # setup.py code which can then fallback to making a purepython |
| # `prevent_msvc_compiling_patch` and extra escapes have been added on line | ||
| # 5 because it is passed through another layer of string parsing before it | ||
| # is executed. | ||
| _SETUPTOOLS_SHIM = ( |
There was a problem hiding this comment.
The escaping makes this hard to read. Could you just use r""" to avoid all the double escaping?
chalice/compat.py
Outdated
| # _SETUPTOOLS_SHIM. | ||
| pip_no_compile_c_shim = ( | ||
| "import pip;" | ||
| "pip.wheel.SETUPTOOLS_SHIM = \"\"\"%s\"\"\";" |
There was a problem hiding this comment.
Same thing. Can you use r""" to cut down on the amount of escaping needed?
chalice/compat.py
Outdated
| "import pip;" | ||
| "pip.wheel.SETUPTOOLS_SHIM = \"\"\"%s\"\"\";" | ||
| ) % _SETUPTOOLS_SHIM | ||
| pip_no_compile_c_env_vars = {} # type: ignore |
There was a problem hiding this comment.
I'd still like to get these typed. These are passed to code that we own in package.py.
chalice/compat.py
Outdated
| # setup.py code which can then fallback to making a purepython | ||
| # package if possible. | ||
| # We need mypy to ignore these since they are never actually called from | ||
| # within ourprocess they do not need to be a part of our typechecking |
There was a problem hiding this comment.
s/ourprocess/our process/
chalice/deploy/packager.py
Outdated
| @@ -374,12 +384,25 @@ def _download_dependencies(self, directory, requirements_filename): | |||
|
|
|||
| # Re-count the wheel files after the second download pass. Anything | |||
| # that has an sdist but not a valid wheel file is still not going to | |||
| # work on lambda and our last ditch effort is to try and build the | |||
| # sdists into wheel files. | |||
| # work on lambda and our we must now try and build the sdist into a | |||
There was a problem hiding this comment.
s/our we must now/we must now/
docs/source/topics/packaging.rst
Outdated
|
|
||
| * A package is installable with ``requirements.txt`` but has optional c | ||
| extensions. Chalice can build the dependency without the c extensions, but | ||
| if you want better performance you can vendor a version taht is compiled. |
Also a couple minor changes to the shim strings to make them more readable.
stealthycoin
force-pushed
the
fix-optional-c-ext
branch
from
88d56cc
to
aaa4106
Compare
The edge case occurs when a package can compile optional c speedups
which succeed during the build process but lock the generated wheel file
to the arch the package was built on. This makes the wheel file
incompatible with lambda and causes it to fail to add that dependency
even though it could have by building it without the optional c
extensions.
This is a bit tricky to fix as means we need to selectively break the
ability to compile c extensions during some runs of our wheel building
phase. It is also quite different on POSIX and Windows.
cc @kyleknap @jamesls @dstufft