Check git mode also in WSL

[emontnemery]

Extend #480 to check git mode when running under WSL too

[asottile]

can you share an example output here and/or add a test? I don't have access to WSL to verify this change

[emontnemery]

Should I add a comment in the code with the sample output?
I'm not sure what kind of test to add for this, mock the output of platform.uname() and assert _check_git_filemode is called?

Sample output:

$ python3
Python 3.7.5 (default, Nov  7 2019, 10:50:52)
[GCC 8.3.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import platform
>>> platform.uname()
uname_result(system='Linux', node='DESKTOP-A7438J1', release='4.4.0-19041-Microsoft', version='#1-Microsoft Fri Dec 06 14:06:00 PST 2019', machine='x86_64', processor='x86_64')
>>> platform.uname()[3].lower()
'#1-microsoft fri dec 06 14:06:00 pst 2019'
>>>

[emontnemery]

@asottile Is the sample output enough, and if not, is the suggested test case acceptable?

[asottile]

this check seems much too weak imo

[emontnemery]

Why does it seem too weak?

[asottile]

Why does it seem too weak?

checking if the string "microsoft" is in the uname doesn't necessarily mean this is WSL

[emontnemery]

I think it's OK, Microsoft builds their own kernels.

Checking uname seems to be the accepted solution, see for example here: https://www.scivision.dev/python-detect-wsl/

[asottile]

I think it's OK, Microsoft builds their own kernels.

I can build my own kernel too, that doesn't mean this is ok

Checking uname seems to be the accepted solution, see for example here: https://www.scivision.dev/python-detect-wsl/

just because someone else makes the same mistake doesn't mean we should. the uname is an arbitrary string and not necessarily an indication this is WSL

[asottile]

microsoft/WSL#423

The "microsoft" substring could appear in non-WSL2 kernel images as well, so it shouldn't be relied upon to find out if one is in WSL.

[emontnemery]

OK, so you're saying it's likely someone else builds their own custom kernel, sets uname to 'Microsoft', and then has the git mode check fail on them?
If so, sure, the check is too weak and this PR can be closed.

[emontnemery]

The comment in the thread you're linking to seems to about distinguishing wsl from wsl2 btw, in both cases it's likely that file modes are not supported.

Edit: The best way would be to try to properly detect if file modes are supported though, instead of checking for win32 or trying to detect wsl as I attempt in the PR.

[emontnemery]

@asottile is there any way we could get this merged?

I must admit I don't understand your comment about building your own kernel, is your concern false positives due to someone building their own kernel, where the substring "microsoft" is present in platform.uname()?

Another method for detecting WSL, which is proposed in the WSL bugtracker, is to check for the presence of a Windows binary on the path, e.g. the output of which cmd.exe. Of course, there's again the risk of false positives, it's possible, but not likely, that there is a cmd.exe in the path of a non WSL system.

How about making the WSL detection opt-in, then it can be used by those who can accept false positives?

[asottile]

@asottile is there any way we could get this merged?

not as-is, no

I must admit I don't understand your comment about building your own kernel, is your concern false positives due to someone building their own kernel, where the substring "microsoft" is present in platform.uname()?

correct, looking for a string in a kernel name is not a proper detection

Another method for detecting WSL, which is proposed in the WSL bugtracker, is to check for the presence of a Windows binary on the path, e.g. the output of which cmd.exe. Of course, there's again the risk of false positives, it's possible, but not likely, that there is a cmd.exe in the path of a non WSL system.

that seems less error-prone at the very least

How about making the WSL detection opt-in, then it can be used by those who can accept false positives?

I'd rather not have configuration options -- worst case you can opt-in by forking

[Kurt-von-Laven]

There are a number of different options for detecting WSL, so many, in fact, that I have stopped well short of compiling a comprehensive list. One could use any subset of them depending on the degree of reliability desired. Personally, I would simply use the first one when Git is present, because Git already inspects the file system it's on for us for precisely the behavior in question:

Some filesystems lose the executable bit when a file that is marked as executable is checked out, or checks out a non-executable file with executable bit on. git-clone[1] or git-init[1] probe the filesystem to see if it handles the executable bit correctly and this variable is automatically set as necessary. ~ git-config: core.fileMode

• git config core.fileMode is true if and only if "the executable bit of files in the working tree is to be honored." This correctly evaluates to true when using the Linux file system in WSL and false when using the Windows file system in WSL, for example, so it is as precise as I could possibly hope for. It returns 1 when run outside of a Git repo.
• which wsl.exe returns 0 on WSL or Windows and 1 otherwise.
• $WSL_DISTRO_NAME is set on WSL and typically not set otherwise.
• systemd-detect-virt --container evaluates to wsl on WSL, and the command is typically either not found or evaluates to something else otherwise.
• Similar to what has been discussed above in the context of uname -r (and hence presumably with the same drawback), /proc/sys/kernel/osrelease contains "microsoft" and "WSL," but generally doesn't otherwise.

If the first option only is used and Git isn't present, then the script can simply do what it already does on non-Windows platforms. Theoretically, one could check $LXMOD, but that seems like overkill at least for an initial iteration and more of a maintenance burden than is warranted for the relatively obscure benefit it would provide.

[emontnemery]

@Kurt-von-Laven should I close this and you can replace it with a PR implementing one of the other metods?

FWIW, home-assistant is no longer using check_executables_have_shebangs in local pre-commit tests, we just run it in automated CI where we know the environment. This works fine to prevent PRs from accidentally including a file with the wrong flags set. It still would be nice to see it fixed though so we can again use it in pre-commit.

[Kurt-von-Laven]

That sounds good to me if @asottile agrees with my implementation plan.

That is a clever workaround. How did you arrange for certain tests to be skipped locally? We should probably do the same since this isn't the only check we run that struggles in some environments.

[hamdanal]

Hello all, this is what microsoft uses to detect WSL in the VSCode launcher script (https://github.com/microsoft/vscode/blob/main/scripts/code.sh#L11-L13):

if grep -qi Microsoft /proc/version && type powershell.exe > /dev/null 2>&1; then
	IN_WSL=true
fi

Maybe this could work here as well.

[Kurt-von-Laven]

@hamdanal, no, I don't think that will be accepted here in light of the above response from the owner to a very similar proposition:

Why does it seem too weak?

checking if the string "microsoft" is in the uname doesn't necessarily mean this is WSL

To summarize my above suggestions, I would simply check whether git config core.fileMode evaluates to true. This has the benefit of leaving the question of WSL out of it altogether.

[Kurt-von-Laven]

@emontnemery, I have opened #730, so you may feel free to close this pull request now if you agree with my changes.