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

Fixes xargs partition length on Windows for EXE hooks #3125

Closed
wants to merge 1 commit into from

Conversation

amarvin
Copy link

@amarvin amarvin commented Feb 8, 2024

Closes #3124

@asottile
Copy link
Member

asottile commented Feb 8, 2024

I don't think this is the right fix or really the problem

I've verified the length computation is correct on my windows machine so something is different about yours (and it's not this)

you'll need to do some more debugging on your end too figure out what's happening

and usually when the length is exceeded there's an error. if that's not happening there's likely something else at play!

@asottile asottile closed this Feb 8, 2024
@amarvin
Copy link
Author

amarvin commented Feb 8, 2024

Can you confirm that in your system the pretty-format-json hook in pre_commit.xargs.xargs() gets shebang normalized to a .bat or .cmd file? For me:

> cmd[0]
'C:\\Users\\alexm\\.cache\\pre-commit\\repodli143ao\\py_env-python3.12\\Scripts\\pretty-format-json.EXE'

so then _max_length stays at the default of 30720, which is too long for Windows batch file lines.

@asottile
Copy link
Member

asottile commented Feb 8, 2024

batch is not in play on my system, no

@amarvin
Copy link
Author

amarvin commented Feb 8, 2024

Huh? All I'm saying is that on my system Windows 11, this if-statement is never entered:

image

because for pretty-format-json, it's an EXE

@amarvin
Copy link
Author

amarvin commented Feb 8, 2024

The length of the first command sent to cmd_fn was 15440 characters. I can't even paste that long of a string into cmd.exe.

@amarvin
Copy link
Author

amarvin commented Feb 8, 2024

You're right that neither batch files nor cmd.exe are being used in this case / on my system by the subprocess.Popen() in pre_commit.util. Not sure where this differing behavior is coming or why shorting the partitioning max length fixes it.

@asottile
Copy link
Member

asottile commented Feb 8, 2024

yes that's why I'm saying you need to do some debugging to figure out why. I'm not blindly changing the code

@amarvin amarvin deleted the patch-1 branch February 29, 2024 16:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

In Windows, pre-commit -a silently skips files as xargs.py partitions are too long for command line
2 participants