This feels like it could be a breaking change to me (so we could try it in preview first), but I'm curious what others think. We may also need to update the rule docs, which both mention WSL.

Otherwise the change looks reasonable to me, and it's nice to drop a dependency!

cc @MichaReiser who reviewed #17584.

Oops, missed the doc change. Fixed it now.

@K900 would you mind testing the example in #5445

I don't use WSL myself, so I'm not very familiar with how it works but what I understand is that all files on a windows disk /mnt/... always have the executable bit set, in which case we'd flag every single file on that disk if they miss a shebang.

I agree, that we should preview gate this change.

It's a little more complicated than that on WSL2. Linux permissions are persisted on Windows partitions, as NTFS xattrs, but files without any xattrs, e.g. ones created on the Windows side, are defaulted to 0777. So if you, say, run git clone on Windows to clone a repo and then try to access it on WSL, it will have 0777 permissions on everything, but if you git clone the same repo to the same path from the WSL side, it will have correct permissions on both sides. This is not entirely obvious behavior, but it will also throw off git (if used from WSL) and basically any other tool, and I don't think Ruff has to try and work around this.

To clarify:

~
❯ cd /mnt/d/test/

/mnt/d/test
❯ ls
╭────────────╮
│ empty list │
╰────────────╯

/mnt/d/test
❯ touch beep

/mnt/d/test
❯ /mnt/c/Windows/System32/cmd.exe /c "echo > boop"  # this escapes back to Windows and creates a file "from Windows"

/mnt/d/test
❯ ls -la
╭────┬──────┬──────┬────────┬──────────┬───────────┬───────────┬────────────────────┬──────┬───────┬──────┬─────────┬─────╮
│  # │ name │ type │ target │ readonly │   mode    │ num_links │       inode        │ user │ group │ size │ created │ ... │
├────┼──────┼──────┼────────┼──────────┼───────────┼───────────┼────────────────────┼──────┼───────┼──────┼─────────┼─────┤
│  0 │ beep │ file │        │ false    │ rw-r--r-- │         1 │ 143552238122452878 │ k900 │ users │  0 B │         │ ... │
│  1 │ boop │ file │        │ false    │ rwxrwxrwx │         1 │ 133137663984158607 │ k900 │ users │ 13 B │         │ ... │
╰────┴──────┴──────┴────────┴──────────┴───────────┴───────────┴────────────────────┴──────┴───────┴──────┴─────────┴─────╯

/mnt/d/test
❯ chmod 0666 boop

/mnt/d/test
❯ ls -la
╭────┬──────┬──────┬────────┬──────────┬───────────┬───────────┬────────────────────┬──────┬───────┬──────┬─────────┬─────╮
│  # │ name │ type │ target │ readonly │   mode    │ num_links │       inode        │ user │ group │ size │ created │ ... │
├────┼──────┼──────┼────────┼──────────┼───────────┼───────────┼────────────────────┼──────┼───────┼──────┼─────────┼─────┤
│  0 │ beep │ file │        │ false    │ rw-r--r-- │         1 │ 143552238122452878 │ k900 │ users │  0 B │         │ ... │
│  1 │ boop │ file │        │ false    │ rw-rw-rw- │         1 │ 133137663984158607 │ k900 │ users │ 13 B │         │ ... │
╰────┴──────┴──────┴────────┴──────────┴───────────┴───────────┴────────────────────┴──────┴───────┴──────┴─────────┴─────╯

I'm not yet convinced that we should make this change but we should explain this caveat in the documentation and explain to users how they can "fix their setup" if they run into this situation (clone from within WSL)

I don't know if I agree, to be honest. This is kind of similar levels of user error as, say, putting stuff on a noexec filesystem, or on a FAT32 or something. There's really no good way to actually know if the filesystem has sane semantics. Maybe some sort of global detection that skips the check if all files come back executable? But even that feels iffy.

I'm not sure if you're misunderstanding me. What I'm suggesting is that we make this change in preview, but we also extend the rule documentation with a WSL section, explaining in which situations they might see violations for every single file and what they can do to fix it. I don't see what the issue is with helping users in case they see confusing behavior and I would also find this documentation useful for myself, if a user asks for help in the future.

The use case I'm worried about is users who switch between WSL and non-WSL, creating some files in Windows and others in WSL, but flake8-executable is enforced in their organisation. They would have to change their behavior. We could consider recommending that they turn off the rule at a user-level configuration.

The thing here is that WSL preserves permissions correctly, if set on the WSL side, so just running autofix should repair any sort of WSL setup, even if the files are then updated from the Windows side, because xattrs stay. (wait, does this rule even have an autofix? maybe it should if it doesn't yet) However, there are also other kinds of weird setups that are already not caught by this check, and I'm not sure there's a good way to identify those in a general enough way to try and be smart about this particular check.

We don't necessarily have to agree here, but I see some documentation giving users a hint on how to solve this problem that users have run into with Ruff in the past, which is a prerequisite to merging.

Sorry, to be clear, I'm not saying this exact change shouldn't have documentation, I'm kind of thinking out loud about whether there is a way to implement this that doesn't need documentation. What do you think about adding an autofix for this, and then maybe explicitly documenting something like "if the autofix didn't work, your filesystem is probably being weird"?

I see.

I don't see an obvious way for Ruff to detect this, given that it depends on where a file was created. I'm leaning towards making the change you suggestg with better documentation and we can always introduce a skip_on_wsl option

Do you mean just this change, or also adding an auto fix? I can look into doing that in a couple hours.

Just this change. Adding an auto fix seems unrelated to this PR

The autofix is a good way to fix a messed up repo, possibly, if you cloned from Windows and ran ruff from WSL. Also, I wonder how hard it'll be to have git support the WSL xattrs... That would solve most issues before they even happen.

Added some documentation that is hopefully generic enough to catch most causes of false positives. Sorry for the delay, caught a cold :(

(and yes, NTFS filesystems mounted to WSL technically do support Unix permissions, but this is a can of worms that is probably out of scope for Ruff rule documentation)

Added some documentation that is hopefully generic enough to catch most causes of false positives. Sorry for the delay, caught a cold :(

No need to feel sorry. Thanks for looking into this. I find it very helpful to have someone with deep WSL understanding to talk this change through.

I hope it's OK to chime in here as the author of #10084 and someone with a lot of experience both using WSL and looking at potential solutions to this issue.

@ntBre, you are correct - this is a breaking change and will cause false positives for a subset of users, where the status quo causes false negatives for a different subset of users. (I'll add a table summary to #10084 and copy it here too)

@MichaReiser's original argumentation when discussing this was that false negatives are worse, as settings are mainly on a per-project basis - so it's hard to disable the rule only for some contributors.

@K900 - this is definitely the easiest of "solutions" and I agree with you, and initially argued myself, that it has long been a strong recommendation by MS, that WSL users avoid using mounted windows partitions for performance reasons (see the note at the end of the description on #17584 for example timings)

My personal opinion, FWIW, is that identifying default file permissions is the best: covers all use-cases, actually improves performance. (This is not what I first thought, when beginning to discuss and look at this issue; I actually agreed with the approach proposed in the PR when I first opened the issue).

The problem I have with identifying file permissions is that to do that we need to:

• find a place to write a test file to (we can't always do that)
• be reasonably confident that the filesystem will behave the same way as the file we just wrote (drvfs doesn't)

Edit: to be clear, #17584 does not in fact work for the "WSL, source root on drvfs, cloned from Windows" use case, because it will create a file with 644 permissions, drvfs will record that and give us back the correct permissions, but none of the other files will have the correct xattrs.

Overview of currently proposed solutions:

#10084 (comment)

The problem I have with identifying file permissions is that to do that we need to:

• find a place to write a test file to (we can't always do that)
• be reasonably confident that the filesystem will behave the same way as the file we just wrote (drvfs doesn't)

Did you take a look at the approach in the other PR?

1. check if pryproject.toml is executable (if not, then we can guarantee that we have a settable executable-bit)
2. touch a file in project_root (could fail if project root is read-only, although this is a more acceptable edge case than current status quo and only triggers on FS with no exe-bit)

What specifically do you have in mind by cases where we can't write a test file?

I've not tested drvfs, do you have a test setup to see whether #17584 fails in that case? My understanding is that it is only relevant for WSL1 (as you say, now dead) and the above test would disable the lints at step 1 anyway.

drvfs is still what WSL2 calls the 9p stuff internally, the case I'm imagining is: user clones repo from Windows, then runs ruff in WSL. What happens then is:

• pyproject.toml is executable, so we continue
• we touch a file in project root, which is 644 by default because umask, that's recorded in NTFS xattrs, reading back the permissions gives us 644
• all the other files are still marked executable because they have no xattrs

drvfs is still what WSL2 calls the 9p stuff internally, the case I'm imagining is: user clones repo from Windows, then runs ruff in WSL. What happens then is:

• pyproject.toml is executable, so we continue
• we touch a file in project root, which is 644 by default because umask, that's recorded in NTFS xattrs, reading back the permissions gives us 644
• all the other files are still marked executable because they have no xattrs

imagining or tested?

I have tested exactly this setup both locally, and in CI (https://github.com/MusicalNinjaDad/forks-ruff/actions/runs/14663820841/job/41154168824) and in both cases the touched file is executable and the lints are disabled.

If you have a specific setup which leads to the behaviour you describe then I'd be really interested to hear the config in place, both from general curiosity and to help with this case.

(EDIT: just tested this manually, again, on WSL2 with cd /mnt/c/Users.../Temp; touch foo; ls -al foo and foo has 0777 permissions)

On my end, the file touched from WSL is 0644.

Also here's a run with ruff from your branch (and a few dbg's sprinkled in):

d/test/chinatsu-master
❯ ~/ruff/target/debug/ruff check --no-cache --select EXE
[crates/ruff_linter/src/rules/flake8_executable/helpers.rs:29:9] is_executable(&settings.project_root.join("pyproject.toml")) = Ok(
    true,
)
[crates/ruff_linter/src/rules/flake8_executable/helpers.rs:33:37] is_executable(tmpfile.path()) = Ok(
    false,
)
chinatsu/__init__.py:1:1: EXE002 The file is executable but no shebang is present
(etc)

diff --git a/crates/ruff_linter/src/rules/flake8_executable/helpers.rs b/crates/ruff_linter/src/rules/flake8_executable/helpers.rs
index 717c805721..47e1e7352d 100644
--- a/crates/ruff_linter/src/rules/flake8_executable/helpers.rs
+++ b/crates/ruff_linter/src/rules/flake8_executable/helpers.rs
@@ -26,11 +26,11 @@ static EXECUTABLE_BY_DEFAULT: OnceLock<bool> = OnceLock::new();
 #[cfg(target_family = "unix")]
 pub(super) fn executable_by_default(settings: &LinterSettings) -> bool {
     *EXECUTABLE_BY_DEFAULT.get_or_init(|| {
-        is_executable(&settings.project_root.join("pyproject.toml")).unwrap_or(true)
+        dbg!(is_executable(&settings.project_root.join("pyproject.toml"))).unwrap_or(true)
             // if pyproject.toml is executable or doesn't exist, run a slower check too:
             && NamedTempFile::new_in(&settings.project_root)
                 .map_err(std::convert::Into::into)
-                .and_then(|tmpfile| is_executable(tmpfile.path()))
+                .and_then(|tmpfile| dbg!(is_executable(tmpfile.path())))
                 .unwrap_or(false) // Assume a normal filesystem in case of read-only, IOError, ...
     })
 }

(for posterity)

Thanks for the confirmation.

Did you adjust your [automount] settings in wsl.conf to enable metadata and set a specific umask? Or are you running with default config and getting these results?

I have options=metadata,uid=1000,gid=100, the uid/gid stuff is just NixOS weirdness for historical reasons.

Thanks - think I've got it now... I see that NixOS defaults metadata enabled, unlike the base WSL default, and for various reasons you have chosen/need to keep your code on an NTFS partition rather than the native linux one.

1. I'm assuming from your comments above that you usually clone with linux git. So in your case your system usually behaves like the native WSL case and either of the PRs give you valid lints, status quo gives you potentially false negatives (?)

2. I don't have git for windows installed, but from looking at your example under d/test/chaintsu-master it appears that cloning to an NTFS partition under windows doesn't add xattr info to maintain the *nix exe-bit. I'm assuming this is not your usual workflow(?)

   • I'd expect that this could get messy quickly, as any future git pulls inside WSL will update file permissions for added/changed files that it retrieves from the remote, and any new files will not have 0644 perms. These perms will persist thanks to the metadata setting. <-- Could you confirm that easily?
   • As you say, there's no decent heuristic available to help in this situation. So the only option would be a command-line-option/environment-variable. I looked into this and dropped it as doing it nicely and well documented affects a lot more code and forces the option to also be exposed via the config files. It's doable but at a higher cost to the maintainers ...
   • There is an argument to say that anyone working this way hopefully understands the consequences of their setup choices and will be in a small minority. Personally, I never like taking that attitude if it can be avoided ...

If I've understood things correctly this means we have the following situation:

To be clear, this is not my actual setup that I actually use, it's a setup that I assume people ran into which led to the initial change. However, it's also yet another proof that guessing "is the filesystem broken" in the general case is impossible (even ignoring all kinds of obviously-pathological things like FileStation 9000 which returns random permission bits on every read).

My normal usage of Ruff doesn't actually trigger this bug - I've run into it because I occasionally build NixOS from source on my WSL systems, and Ruff fails in its own tests because those are not designed to run under WSL.

Also, Git knows nothing about the NTFS Unix-permission xattrs, so it never writes or updates them for any reason, meaning newly added files (if pulled from Windows) will be 0755, and any updated files will keep their existing permissions (unless moved, possibly?). Teaching Git about these xattrs is another thing that could probably improve things a lot, but not my can and not my worms.

Thanks. Yeah, no heuristic will be perfect for this. I'd see the "WSL user, metadata enabled, cloned on Win" as an unusual edge case which requires someone to actively adjust settings from defaults AND work against recommended practices, with consequences far beyond these lints - so reasonable to assume "they know what they're doing".

Could you confirm the below table matches your experience and I'll update the issue to reflect it:

This seems accurate yeah. I hate how the table just keeps growing...

So folks, should we make some kind of call here? I'm still leaning towards just removing the check and documenting it better that cursed filesystem setups are the end user's problem.

So folks, should we make some kind of call here? I'm still leaning towards just removing the check and documenting it better that cursed filesystem setups are the end user's problem.

I don't feel a strong need to make a change here, especially since this has a very high chance of breaking someone's setup.

I'd be open to adding a config option that allows opting in on WSL, and extending the documentaiton

The other issue, and the one that actually led me to discovering this behavior, is that running cargo test on Ruff itself breaks on WSL, because it makes this assumption and the tests aren't designed for it. This could be fixed by skipping the tests on WSL, I guess?

The other issue, and the one that actually led me to discovering this behavior, is that running cargo test on Ruff itself breaks on WSL, because it makes this assumption and the tests aren't designed for it. This could be fixed by skipping the tests on WSL, I guess?

Yes, I consider this to be a separate issue from changing the rule's behavior. Skipping it on WSL seems fine and something we can do in a seperate PR

OK, so here's skipping the tests, for now: #22721