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

Make _EditableFinder case-sensitive on case-insensitive filesystems (for certain editable installations) #3995

Merged
merged 13 commits into from
Aug 15, 2023

Conversation

aganders3
Copy link
Contributor

@aganders3 aganders3 commented Jul 27, 2023

Summary of changes

This updates the _EditableFinder that is added to sys.meta_path for certain editable installations to respect filename case on case-insensitive (but case-preserving) filesystems. I believe this behavior better matches normal installations and PEP 235.

The way the change works is by checking the path is in the directory listing (which returns proper case) in addition to checking if the file exists (which is generally case-insensitive if the filesystem is). I believe this is pretty much how it is done in the FileFinder in the standard library (see here and here for example).

Closes #3994

Pull Request Checklist

@aganders3
Copy link
Contributor Author

It's not terribly surprising to see this Windows failure. Unfortunately I don't have very easy access to a Windows system but I'll see what I can do.

@aganders3
Copy link
Contributor Author

I pushed 07d0242 to hopefully address the lint errors in the docs build -- I'm not sure why these errors showed up here. Maybe there was a new release of sphinx-lint or something. Let me know if you'd rather see that commit moved to a different PR.

Copy link
Contributor

@abravalheri abravalheri left a comment

Choose a reason for hiding this comment

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

Hi @aganders3, thank you very much for looking into this (including the changes in the docs).
Please see some comments below.

setuptools/tests/test_editable_install.py Show resolved Hide resolved
setuptools/command/editable_wheel.py Outdated Show resolved Hide resolved
@aganders3
Copy link
Contributor Author

aganders3 commented Jul 30, 2023

Unfortunately I still couldn't make this work with public classes from importlib.machinery. It mostly worked for normal imports, but needed enough special logic for namespace packages that just expanding the previous method ended up being simpler. I can keep trying along those lines if you'd prefer, but I don't immediately see a clean solution.

I expanded the tests here to cover more of the cases you brought up. Thanks for the review!

@abravalheri
Copy link
Contributor

Hi @aganders3 thank you very much for working on this.
Sorry I haven't had the time to review this PR in detail.
My initial/preliminary thoughts are:

  • I plan to split the commits targeting the docs linter (thank you very much for that!) into a separated PR. This way I can merge the docs part and rebase this PR, which will make it a little cleaner and easier to review.
  • I am concerned with the increase in complexity for the finder. I am also thinking about the loops to check file/directory existence in the file system, since they might make import statements slow (with only 1 project installed as editable, that should be fine, but maybe if the user installs multiple editable projects in the same virtualenv it might became a problem).

Once I have a bit more time I can work on this, but please feel free to bet me to that.
Regardless, thank you very much for the contribution (that includes tests ❤️), even if we don't add exactly the code as it is right now, I am sure we are going to reuse most of it in the implementation.

@aganders3
Copy link
Contributor Author

No worries - thanks again for the reviews! I may spend a little more time looking again at using importlib.machinery but otherwise will probably leave this alone for the moment. At the very least I am happy to see this issue being addressed and I will walk away with a much deeper understanding of the import system 😅.

@abravalheri abravalheri mentioned this pull request Aug 2, 2023
2 tasks
@abravalheri abravalheri force-pushed the fix-editable-case-sensitivity branch from 4e5668d to 65d05dd Compare August 2, 2023 10:02
@abravalheri
Copy link
Contributor

@aganders3, I submitted a few commits trying something out...
If it does not work we can revert the commits.

@aganders3
Copy link
Contributor Author

aganders3 commented Aug 2, 2023

Nice! It at least passes my tests locally (on macOS). _find_nested_spec is what I couldn't quite work out, but this looks very clean. I had not considered using spec_from_file_location along with a spec already discovered from a finder.

Thanks!

edit: I also tried your solution with my test project and it seems to be working as expected

@abravalheri abravalheri merged commit 463b4df into pypa:main Aug 15, 2023
21 checks passed
@abravalheri
Copy link
Contributor

Thank you very much

@aganders3 aganders3 deleted the fix-editable-case-sensitivity branch August 15, 2023 17:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] certain editable installs allow case-insensitive imports
2 participants