-
Notifications
You must be signed in to change notification settings - Fork 1k
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] Add support for editable installs to the module resolver #12307
Merged
Merged
Changes from 7 commits
Commits
Show all changes
19 commits
Select commit
Hold shift + click to select a range
287fcec
[red-knot] Add support for editable installs to the module resolver
AlexWaygood 645532b
Deduplicate search path roots
AlexWaygood a456ef9
Make all module resolution search paths absolute
AlexWaygood 9d68c07
Improve docs
AlexWaygood 99d87b4
Be more lenient about trailing and leading whitespace
AlexWaygood f7219f2
Make it a Salsa query and add caching tests
AlexWaygood f664391
Lazily discover editable installations
AlexWaygood 42551d1
Misc review comments
AlexWaygood 1709e91
Simplify
AlexWaygood 64f03c6
Use `report_untracked_read()`
AlexWaygood b1b2ffb
Avoid an unnecessary allocation
AlexWaygood 1adf37d
Don't add a new method for creating arbitrary directories in tests
AlexWaygood 388fdca
Change test so that it asserts the cache is invalidated after modific…
AlexWaygood 1c267b5
Update crates/red_knot_module_resolver/src/resolver.rs
AlexWaygood 6d8acb9
Cleanup tests
AlexWaygood e29d3d4
Fix comment
AlexWaygood 23d2d8b
Fix bugs, address review, add TODOs
AlexWaygood 52f457b
Merge branch 'editables' of https://github.com/astral-sh/ruff into ed…
AlexWaygood 0d64399
Merge branch 'main' into editables
AlexWaygood File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
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
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
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
Oops, something went wrong.
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I considered just using the
SitePackages
variant for editable installs, since these search paths do come about as a consequence of.pth
files in thesite-packages
directory. However, the search paths for editable installs aren't necessarily children of thesite-packages
search path (and indeed most often won't be), and I think it would be too easy to assume that all instances of theSitePackages
variant have thesite-packages
directory as their root.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have a problem with introducing a new enum variant for pth-added paths, though I'm also not totally convinced by it: I don't know when we'd ever handle them differently, and it requires one more chunk of duplicated code wherever we handle this enum. I don't think the confusion you mention should occur: in general search paths should never be children of other search paths (this is a really bad setup that leads to the same module being importable under two different names), so it doesn't seem to me to be a likely confusion to assume this would be the case.
I'm not sure about the name
EditableInstall
.pth
files are a feature of the Python import system that are used by the editable-install feature of some package managers, but the feature predates that use, andpth
files have been (and I'm sure still are) used in many other ways as well (e.g. for the zipped-egg installation format of setuptools). So I think it's potentially a little misleading to imply that every path discovered in a.pth
file is an editable install.I'd suggest
PthEntry
or something similar instead.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hrm, true, but the validation we do elsewhere should ensure that we only add paths that point to directories when we iterate through
.pth
files to try to find paths that could potentially be editable installs. While it's true that a directoy path in a.pth
file could point to something which wasn't intended to be an editable install, the Python runtime will still consider Python modules at that path to be importable, correct (since they'll be added tosys.path
bysite.py
)? So IIUC a directory path in a.pth
file is in effect an editable install, even if it... wasn't meant to be.All of which is to say, from the perspective of the module resolver, this variant should only really be used if we think it's more-likely-than-not that a path represents a directory that has been editably installed, or a path relative to a path representing a directory that has been editably installed. We should have done sufficient validation at this point that it's more than just "an arbitrary line in a
.pth
file".There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm okay with either name but it might be at the time that we add some documentation to each variant because I don't think the linked order still applies (it doesn't mention editable installs)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding some more docs makes sense but I won't do it just now, since I may still refactor this pretty soon according to the idea Carl suggested on one of my earlier PRs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Certainly any path found in a
.pth
file will be added tosys.path
, and particularly if that path is a directory, it will result in modules there being importable. I'm not sure if there is any other validation you're referring to that we do.The logical jump you're making that I don't quite follow is "if we add a path to sys.path that makes things importable, that's an 'editable install'." I don't think that follows. It's just importable Python source code. "Editable install" is a very specific user-facing feature that implies a certain use case. For example, there's no reason the path added by the
pth
file line couldn't point to some Python code on a read-only filesystem. We should still support that just fine, but it's not very editable! In that case the pth file entry is being used for some other use case that I wouldn't describe as an "editable install."