Allow EXE001 & EXE002 on WSL, fix issues with other cases of mounting non-unix filesystems#17584
Allow EXE001 & EXE002 on WSL, fix issues with other cases of mounting non-unix filesystems#17584MusicalNinjaDad wants to merge 6 commits intoastral-sh:mainfrom
Conversation
crates/ruff_linter/src/rules/flake8_executable/rules/shebang_missing_executable_file.rs
Outdated
Show resolved
Hide resolved
|
Thank you. Your test setup is very impressive. There's a lot happening right now. I'm not sure when I'll get the time to review this PR. |
|
Thanks - I fully understand and that quick note was really valuable. I'd also personally much rather have red-knot available than this included in ruff ;) I'll probably hang back on taking up other topics until I do get feedback on this (unless I get bored or randomly motivated) - so I can include any feedback on style etc from the start. |
|
Is there something I could do to make this approach easier to maintain? I understand if you are reluctant to take over the additional test infrastructure. (@ntBre, @MichaReiser, @amyreese) The logic change itself is simple and shouldn't add maintenance burden, particularly with CI-based tests in place. I could remove the testing and just provide the logic, although personally I dislike the idea ;) |
|
Thanks again for your work on this! Taking a quick look back at the diff, the main things that make this look hard to review to me are:
These all raise the bar of how closely I feel I need to review the changes compared to modifying the rule's Rust code or normal test fixtures. Beyond that, I'm just not that familiar with Windows and don't have a Windows machine (easily) available for testing. The core change does look straightforward, but it's also a bit surprising to create a temporary file and check its permissions (assuming I'm understanding correctly) instead of calling Hopefully that gives some context. That's why the PR looks complicated to review to me in its current form and why I haven't found a chance to prioritize it yet. |
|
Fully understand, and I sort of expected that answer ;) I'll restructure to make it easier to review.
Correct - that's what the logic change does. Reason: |
- Update existing EXE001 & EXE002 test fixture contents to make failure analysis easier - Also run EXE001 & EXE002 tests on windows - Add specific tests for current failure cases on alternative filesystems (NTFS etc.)
- Add dedicated github workflow to run flake8_executable lints on multiple operating systems & filesystems - Add github action to install & cache WSL on windows runner. This is a resource & time intensive activity. Cache key is based upon selected distro version & hash of action.yml. - Add a `ci-short` nextest profile. Standard CI profile lists all skipped tests, this list is very long for this workflow.
- Run standard EXE001 & EXE002 tests by default on unix - Run NTFS tests on windows - Create a cfg option `test_environment` to override default unix behaviour - Run NTFS tests on unix if `test_environment` is set to `ntfs` At this point test results in CI accurately reflect current status quo before implementing and logic changes: - Standard WSL case fails - Linux, Windows & WSL-on-NTFS all pass
- Remove all checks from EXE001. All files are always executable on NTFS
filesystems, therefore the EXE001 lint will never fail. Running any logic
here simply slows down performance for ALL unix users.
- Add a helper function to identify whether files are executable by default.
1. Using a `OnceLock` to avoid repeated checks improves performance
vs previous repeated is_wsl checks for ALL users
2. If `pyproject.toml` exists and is not executable we know that this
filesystem supports `is_executable`.
3. If no `pyproject.toml` is available we create a `NamedTempFile` in
the project root and check whether `is_executable`.
- If files are executable by default EXE002 lint is not viable. Replace
`is_wsl` with `executable_by_default`
- Remove dependency on `is_wsl` crate. No longer needed.
- Update documentation to reflect improved logic and link to WSL
documentation regarding filesystem recommendations.
19a6b3f to
c40cea0
Compare
|
@ntBre - I've simplified a few things when rebasing this onto current main; and tried to simplify the review by packaging it into 5 distinct and easily reviewable commits (see new description). Happy to answer any questions that are still open ... Thanks! |
amyreese
left a comment
There was a problem hiding this comment.
Why is this changing nextest config and github actions?
It's changing the test setup to test the changes, but I think I agree with the sentiment. It doesn't feel worth it to me to introduce two new CI workflows among the other complexity to test these relatively niche rules. I'm still trying to decide how best to proceed here. Thanks again for working on it! |
I can remove them if you prefer. It's a single commit to revert. I needed them for testing during development as the easiest way to cover all the necessary OS & FS combinations. |
|
Thanks again for working on this and for your patience with all the back and forth. I'd be curious to get Micha's thoughts when he's back from PTO at the end of the month, but then I think we should make a decision here one way or the other. I'm always hesitant to say this because it's nice to have regression tests, like you said, but my current thinking is that I would be okay landing this without the testing setup, which seems to be the most contentious part. |
Thanks for the update. |
|
I don't understand WSL enough to say whether this change is correct and works in all situations. My understanding after participating in #21724 is that there will still be cases where this heuristic fails, which can be frustrating for users, especially because it "used to work". To move this forward, I'd have to spend some time familiarizing myself with WSL, but it never felt pressing enough to put on top of my tasks. I'm more than happy if someone else wants to take a look. From what I understand today (and I might be completely wrong on this), I'm leaning towards checking the permission bits on WSL by default, extending the rule documentation with some examples where the permission checks might fail and why, and introducing an option that brings back the existing behavior to skip the check on WSL. |
|
It's unfortunate that someone bringing up random, hypothetical edge cases can torpedo a change that would benefit 3 different, reasonably sized, groups of users. Even more so, given that the fix works for the author of that other PR. 😢 The reasoning behind my suggestion not to simply remove the WSL checks is in the updated description. So far, no actual, real-world use case has been reported which this change doesn't catch. Of course, no heuristic will be 100% accurate; but I would have thought a change was worthwhile if it:
If you want someone from astral to take a look at this, I'd suggest someone who knows their way around filesystems - the whole topic of WSL is a red herring which is just causing noise. (FWIW @ntBre previously suggested "my current thinking is that I would be okay landing this without the testing setup, which seems to be the most contentious part.") |
Summary
EXE001 & EXE002 lints require a file-system which supports unix-like filemodes. Users mounting projects from USB sticks (#12941) and users mounting windows file-systems (#3110, #5445) will receive false positives from lint EXE002.
There is nothing inherent to WSL which is related to this issue. WSL users following recommended usage will not be storing files on a windows file-system. #5735 introduced a check which unnecessarily disables these lints for all WSL users (#10084).
This change implements a heuristic check to identify whether the filesystem supports unix-like filemodes and additionally improves performance of these lints.
fixes #10084, fixes #12941, fixes #5445, fixes astral-sh/ruff-action#307
Reviewers Guide
Changes have been squashed into 5 molecular commits, which can be reviewed independently:
is_wslcheck with file-system heuristicpyproject.tomlexistsWhy not just remove
is_wslchecks?We can realistically expect that users who store their project on their windows file-system will mainly be less experienced and newer coders. Introducing a situation where they receive false positives for every file is unreasonable from a UX perspective.
What's this got to do with WSL?
Fundamentally, nothing. The current code disable lints for WSL users, where it does not need to. A key user group who use windows file-systems will be relatively inexperienced WSL users, not following Microsoft's recommendations on file locations.
Performance
This change improves performance of these lints by approx 3% in almost all cases and does not reduce performance in any cases. (Bench results available, if wanted)
Testing
Given the need to run tests on multiple OS & file-system combinations, this PR provides a dedicated github workflow to avoid future regression. The workflow is only triggered on relevant changes.