★ wanayoo — archive 1999 https://github.com/pre-commit/pre-commit-hooks/pull/512Nouvelle recherche | Portail wanayoo
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

Check git mode also in WSL #512

Open
wants to merge 3 commits into
base: master
from
Open

Conversation

@emontnemery
Copy link

@emontnemery emontnemery commented Aug 17, 2020

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

emontnemery added 2 commits Aug 17, 2020
@@ -21,7 +22,9 @@ def zsplit(s: str) -> List[str]:


def check_executables(paths: List[str]) -> int:
if sys.platform == 'win32': # pragma: win32 cover
if (
sys.platform == 'win32' or 'microsoft' in platform.uname()[3].lower()

This comment has been minimized.

@asottile

asottile Aug 17, 2020
Member

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

This comment has been minimized.

@emontnemery

emontnemery Aug 18, 2020
Author

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'
>>>

This comment has been minimized.

@emontnemery

emontnemery Sep 5, 2020
Author

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

This comment has been minimized.

@asottile

asottile Sep 5, 2020
Member

this check seems much too weak imo

Copy link
Author

@emontnemery emontnemery left a comment

Why does it seem too weak?

@asottile
Copy link
Member

@asottile asottile commented Sep 5, 2020

Why does it seem too weak?

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

@emontnemery
Copy link
Author

@emontnemery emontnemery commented Sep 5, 2020

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
Copy link
Member

@asottile asottile commented Sep 5, 2020

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
Copy link
Member

@asottile asottile commented Sep 5, 2020

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
Copy link
Author

@emontnemery emontnemery commented Sep 5, 2020

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
Copy link
Author

@emontnemery emontnemery commented Sep 5, 2020

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.