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

[red-knot] Allow multiple site-packages search paths #12609

Merged
merged 4 commits into from
Aug 2, 2024

Conversation

AlexWaygood
Copy link
Member

@AlexWaygood AlexWaygood commented Aug 1, 2024

Summary

If a virtual environment's pyvenv.cfg file contains the line include-system-site-packages = true, anything installed in the system Python installation's site-packages directory will be accessible when the virtual environment is activated, as well as anything inside the virtual environment's site-packages directory. That means that we could have 0, 1, or many site-packages directories that we'd need to consider as search paths -- currently we only contemplate the 0 or 1 possibilities. As such, it makes sense for the site_packages field on Program to be a Vec rather than an Option. This is the first commit of this PR.

The second commit of the PR updates some of the logic in the resolver to reflect the fact that if there are two site-packages directories on sys.path at runtime, any editable installs provided via .pth files in the first site-packages directory will take precedence over the second site-packages directory when it comes to module resolution.

Test plan

cargo test

@AlexWaygood AlexWaygood added the red-knot Multi-file analysis & type inference label Aug 1, 2024
@AlexWaygood AlexWaygood marked this pull request as draft August 1, 2024 11:14
Copy link
Contributor

github-actions bot commented Aug 1, 2024

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

@AlexWaygood AlexWaygood marked this pull request as ready for review August 1, 2024 11:40
@AlexWaygood AlexWaygood changed the title [red-knot] Make the site-packages search-path setting a vector of paths [red-knot] Allow multiple site-packages search paths; enable easier mocking in tests Aug 1, 2024
@AlexWaygood AlexWaygood changed the title [red-knot] Allow multiple site-packages search paths; enable easier mocking in tests [red-knot] Allow multiple site-packages search paths Aug 1, 2024
@MichaReiser
Copy link
Member

Okay, I'll wait with reviewing then

@AlexWaygood
Copy link
Member Author

I added a test and updated the PR description. Ready for re-review!

crates/red_knot_module_resolver/src/resolver.rs Outdated Show resolved Hide resolved
site_packages_dir,
FileRootKind::LibrarySearchPath,
);
dynamic_paths
Copy link
Member

Choose a reason for hiding this comment

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

I do think that moving site packages constructing into the dynamic modules query will make it harder for validation but whatever. Let's ignore this for now

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, @carljm was wondering if all this lazy .pth-file iteration was a bit too fancy -- we should maybe just collect all search paths eagerly. Definitely open to that; I have no idea how much lazy iteration through .pth files actually saves us.

Copy link
Contributor

Choose a reason for hiding this comment

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

I can't imagine lazy iteration of .pth files saves us anything at all in any realistic scenario. It would require that there is a site-packages path, with editables installed into it, that the project never actually imports anything from; all imports are first-party. So yeah, I'd favor simplifying by just collecting all search paths eagerly.

@AlexWaygood AlexWaygood enabled auto-merge (squash) August 2, 2024 13:29
@AlexWaygood AlexWaygood merged commit fbab04f into main Aug 2, 2024
16 checks passed
@AlexWaygood AlexWaygood deleted the alex/multiple-site-packages branch August 2, 2024 13:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
red-knot Multi-file analysis & type inference
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants