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

ExternalProgram: Ensure that the path has no forward slashes on Windows #13924

Closed
wants to merge 1 commit into from

Conversation

lb90
Copy link
Contributor

@lb90 lb90 commented Nov 19, 2024

Some programs like cmd.exe do not expect forward slashes in the first part of GetCommandLineW() (the part corresponding to argv[0]) and confuse the slash for a command-line switch.

This is a fix for test cases\common\33 run program. Also helps #1526

Note: shutil.which uses forward slash also on Windows. We may introduce a mesonlib.which() helper (see also commit 935aa452)

https://github.com/python/cpython/blob/v3.13.0/Lib/shutil.py#L1503

@lb90 lb90 requested a review from jpakkane as a code owner November 19, 2024 10:16
Some programs like cmd.exe do not expect forward slashes in the first part
of GetCommandLineW() (the part corresponding to argv[0]) and confuse the
slash for a command-line switch.
@eli-schwartz
Copy link
Member

This is a fix for test cases\common\33 run program. Also helps #1526

I don't see how this helps #1526 since it actually does the exact opposite of what was proposed there? The issue is that meson globally replaces command lines in ninja using the opposite of what you do here, in order to solve quoting hell on Windows. The solution is to only do it ("the opposite of this PR") for filepaths, including ExternalProgram.

Anything you do in this PR will get reverted again by the global replacement in ninjabackend, so this PR only has an effect on commands run at configure time.

@lb90
Copy link
Contributor Author

lb90 commented Nov 19, 2024

Hi @eli-schwartz!

I don't see how this helps #1526 since it actually does the exact opposite of what was proposed there? The issue is that meson globally replaces command lines in ninja using the opposite of what you do here, in order to solve quoting hell on Windows. The solution is to only do it ("the opposite of this PR") for filepaths, including ExternalProgram.

Thanks, I admit that I don't know much about the internals of Meson, so that's very helpful! 🙂 anyway, isn't #1526 about using backward slashes \ for program and file paths? It mentions cmd.exe, which AFAIK has problems when invoked as C:/Windows/System32/cmd.exe, but not C:\Windows\System32\cmd.exe

Anything you do in this PR will get reverted again by the global replacement in ninjabackend, so this PR only has an effect on commands run at configure time.

Yes, indeed the test fails during configure time. The path will be reverted to forward slashes / when writing the ninja build rules, but probably that's an issue that can be tackled separately from this

@eli-schwartz
Copy link
Member

eli-schwartz commented Nov 19, 2024

Thanks, I admit that I don't know much about the internals of Meson, so that's very helpful! 🙂 anyway, isn't #1526 about using backward slashes \ for program and file paths? It mentions cmd.exe, which AFAIK has problems when invoked as C:/Windows/System32/cmd.exe, but not C:\Windows\System32\cmd.exe

No, it's about using forward slashes / for programs and file paths, and ceasing meson's current, highly unfortunate, violation of user-supplied arguments such as sed s/@PREFIX@/\/usr/ becoming sed s/@PREFIX@///usr/.

See:
#737
#12097
#13172
#4393

The ticket is badly worded since we currently do "normalize" them everywhere, except for the one place we pass filenames (not commands!) to cmd.exe

@lb90
Copy link
Contributor Author

lb90 commented Nov 19, 2024

This has been fixed by #13886 it seems, closing!

@lb90 lb90 closed this Nov 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants