[WIP] Pants distutils extensions to directly use our toolchain environment #6273
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
A bit too many ??? and TODO: ??? to look at, please re-ping when this is in a reviewable state. |
This was referenced Aug 8, 2018
|
This is blocking on the issue described in #6338 getting resolved, as it is not possible to |
cosmicexplorer
added a commit
that referenced
this pull request
Sep 4, 2018
…eps, and some other unrelated native toolchain changes (#6275) ### Problem This PR solves two separate problems at once. It should have been split into two separate PRs earlier, but has undergone significant review and it would be a significant amount of unnecessary effort to do that at this point. #### Problem 1 The `setup_requires` kwarg of `python_dist()` holds addresses for `python_requirement_library()` targets. Currently, these requirements are fetched into a directory which is added to `PYTHONPATH`. This currently produces an invalid set of packages (for reasons I'm not quite sure of), which can cause errors on `import` statements within the `setup.py`. #### Problem 2 *See #6273 for more context on this second issue:* `distutils` does all sorts of things with our environment variables (e.g. adding `CFLAGS` to `LDFLAGS`), so until #6273 is merged, trying to modify anything but the `PATH` in the environment in which we invoke `setup.py` in for `python_dist()` targets causes lots of failures in many scenarios. ### Solution #### Solution 1: - Extract out the pex-building logic from the `Conan` subsystem into an `ExecutablePexTool` base class. - Collect `setup_requires` into a pex, using `ExecutablePexTool`. #### Solution 2 - Only modify the `PATH` in the environment dict produced from `SetupPyExecutionEnvironment`, no other env vars relevant to compilation. - Also, prepend our toolchain to the current PATH (commented inline, with a TODO pointing to #6273). - Add `LC_ALL=C` to the environment as well so compilation error output doesn't have decode errors on smart quotes. ### Result #### Result 1 `setup_requires` now fetches all necessary transitive deps into a pex, which means you can reliably import python requirements declared in `setup_requires` in the `python_dist()`'s `setup.py`. #### Result 2 Some of the native toolchain code is slightly simplified until #6273 is merged.
|
After #7126, this is obsolete. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
WIP because #6275 and #6271 need to be merged for this to work
Problem
See #5661 for more context. It's hard to get
python_dist()to compile with the flags we want -- see here in the python source whereCFLAGSare pushed into theLDSHAREDcommand line (which causes linking to fail, funnily enough because it is interpreted as compilation). I'm sure that's there to cover some specific use case, but since we have control of the compilation/linking environment we want the ability to pass our arguments through precisely.Solution
distutils_extensions.pyin the python backend to ensure the compiler and linker command line invoked in the setup.py invocation match exactly what we provide through environment variables. This subclasses thebuild_extcommand from distutils and overrides it with our subclass.Result
We can now have precise control over the compilation and linking occuring when building local
python_dist()targets with native sources. A setup.py project can be made to work both in and outside of pants without significant modification (a new example project was made to show this -- this workflow will require a small amount of additional testing in this PR).Current Blockers
setup_requiresdoesn't seem to download transitive dependencies, or at least, trying to use it in the new example project pointing to apants_requirement()target fails trying to load Pygments.