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 --exclude. #2409

Merged
merged 10 commits into from
Jun 12, 2024
Merged

Fix --exclude. #2409

merged 10 commits into from
Jun 12, 2024

Conversation

jsirois
Copy link
Member

@jsirois jsirois commented May 15, 2024

Previously, --excludes were processed after all resolution was
complete. Not only was this sub-optimal, incurring more resolution work
(downloads, backtracks, etc.), but it also could lead to failed
interactions (metadata extraction & wheel builds) with excluded
distributions.

Fix this by plumbing --excludes all the way through the Pip resolve
process. Additionally, fix the existing plumbing through the two other
resolve processes: PEX repository resolution and lock file resolution.

Fixes #455
Fixes #2097

@jsirois jsirois marked this pull request as ready for review June 9, 2024 20:32
@jsirois
Copy link
Member Author

jsirois commented Jun 9, 2024

This is on the big side, but it is broken up into functioning independent commits. Thanks in advance for your attention.

I will note this opens up --override since the deep --exclude plumbing already tricks Pip into following doctored requirements lists; it's just that the only doctoring right now is removal of requirements from the list and not editing of requirements in the list. No one has asked for this yet, so I'll save that for a follow up.

Filed #2425 to track --override.

@jsirois
Copy link
Member Author

jsirois commented Jun 10, 2024

This should be all green now.

cc:@cognifloyd who inspired going a bit further and implementing deep excludes.

@@ -358,7 +358,7 @@ def _spawn_pip_isolated(
with ENV.strip().patch(
PEX_ROOT=ENV.PEX_ROOT,
PEX_VERBOSE=str(ENV.PEX_VERBOSE),
__PEX_UNVENDORED__="1",
__PEX_UNVENDORED__="setuptools",
Copy link
Member Author

Choose a reason for hiding this comment

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

The issue before this change was the un-vendoring was global and just above in pex/pip/download_observer.py, pex was newly exposed to Pip to support patches that use Pex code (the --exclude patches leverage this). That pex code, including everything under pex.vendor._vendored was being exposed which broke pex.third_party vendored imports for Python2.7 where the meta-path importer has different precedence than under newer Pythons. Since Pex itself never imports setuptools, exposing just that serves both the Pex patches of Pip (which don;t need pex.third_party.setuptools) and vendored Pip itself (which needs setuptools to build sdists). Ideally Pex would not re-write setuptools imports at all (since it never does a pex.third_party import of the code), but Lambdex does and this change would break Lambdex. A test caught this and serves as a backstop to not break Lambdex users.

Copy link

@cognifloyd cognifloyd left a comment

Choose a reason for hiding this comment

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

LGTM

I read through each of the commits. Impressive!

This should reduce the pain caused by evil packages by providing an elegant escape hatch. Thank you for figuring out how to plumb excludes into pip.

Comment on lines +267 to +268
if "BEHAVE" not in os.environ:
sys.exit("I'm an evil package.")

Choose a reason for hiding this comment

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

🤣

This is so true. Packages that put sys.exit in setup.py are EVIL!

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, I felt a bit bad about this. It's really Python that is evil here. The pyinotify project is ~17 years old and, IIRC there was no way at that time (just like today!) for an sdist to do anything but fail when you try to compile it on a platform it doesn't support. The sys.exit in the position pyinotify puts it is aggressive, but with out it the build will just fail later anyhow.

The real problem then, is the distutils / setuptools build system here. Since those require evaluating Python code (in greater degrees further back in time) to learn just project metadata, aggressive tends towards evil - the project sdist acts as a hand grenade for the only curious.

Setuptools grew setup.cfg eventually to become up to ~100% declarative and then later Python people (but crucially not Python!) woke up and forked PyPA and eventually produced PEP 517/518 and now setuptools even supports pyproject.toml declarative metadata.

I can't describe the brokenness of a language not having its story straight better than Indy Greg or the related discussion, or, the icing on the cake, the ensuing inaction:

It's true it can be hard for an OSS project to muster the resources to tackle a big project, but Python has some serious financial backers and it could decide to make this a priority and get the money and people-time to clean this up in, say, a 5 year period. Instead though its floundering and leaving a confusing landscape in its wake for users.

Comment on lines +31 to +39
"--platform",
"macosx_10.12-x86_64-cp-310-cp310",
"--platform",
"linux-x86_64-cp-310-cp310",
"doit==0.34.2",
"--exclude",
"MacFSEvents",
"--exclude",
"pyinotify",

Choose a reason for hiding this comment

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

Ooh. Nice! This looks great! Blasted evil pyinotify with its sys.exit ...

Copy link
Collaborator

@kaos kaos left a comment

Choose a reason for hiding this comment

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

Reviewed first commit on specifier sets.

pex/specifier_sets.py Show resolved Hide resolved
pex/specifier_sets.py Show resolved Hide resolved
pex/specifier_sets.py Outdated Show resolved Hide resolved
pex/specifier_sets.py Outdated Show resolved Hide resolved
pex/specifier_sets.py Outdated Show resolved Hide resolved
pex/specifier_sets.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@kaos kaos left a comment

Choose a reason for hiding this comment

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

Nice.
Didn't spend as much time as I'd like on the remaining commits, but nothing that jumped out to me.

@jsirois jsirois merged commit a08ba2f into pex-tool:main Jun 12, 2024
26 checks passed
@jsirois jsirois deleted the issues-2097/fix branch June 12, 2024 00:01
Copy link
Collaborator

@huonw huonw left a comment

Choose a reason for hiding this comment

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

Late review, that would've been approval ✅ just some minor questions.

pex/pip/excludes/requires.py Show resolved Hide resolved
pex/pip/excludes/requires.py Show resolved Hide resolved
@@ -142,7 +149,22 @@ def test_exclude_complex(dist_graph):

pex_info = PexInfo.default()
pex_builder = PEXBuilder(pex_info=pex_info)
dependency_manager.configure(pex_builder, excluded=["c"])
with pytest.raises(AssertionError) as exec_info:
dependency_manager.configure(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry, but I'm a bit slow on the uptake from this code. Why does this configure call fail but the later one does not?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because the input requirements are dist_graph.root_reqs (see: 147) which is ["a", "b"]. The --exclude, however is for "c", which is a transitive dependency, but "c" has not been excluded from The DependencyManager.distributions. The DependencyManager only handles excluding root reqs in the case you both ask for a thing and --exclude the thing. The tricky case being pex /this/local/project --exclude "foo<1" ... where Pex doesn't know the project name and version of the /this/local/project requirement until after the Pip resolve and wheel build, which is when the DependencyManager finishes things up building the PEX.

The latter call doesn't fail because the DependencyManager.distributions has a hole - "C" is excluded already (by Pip).

Copy link
Collaborator

Choose a reason for hiding this comment

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

The DependencyManager only handles excluding root reqs in the case you both ask for a thing and --exclude the thing

Ah, okay 👍 makes sense thanks.

@jsirois jsirois mentioned this pull request Aug 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants