Skip to content

Fix Filename.quote_command hanlding of forward slashes in program path under Win32#13417

Merged
nojb merged 7 commits intoocaml:trunkfrom
nojb:fix_quote_comamnd_win32
Sep 11, 2024
Merged

Fix Filename.quote_command hanlding of forward slashes in program path under Win32#13417
nojb merged 7 commits intoocaml:trunkfrom
nojb:fix_quote_comamnd_win32

Conversation

@nojb
Copy link
Copy Markdown
Contributor

@nojb nojb commented Sep 3, 2024

Fixes #13415

Tweak the code to quote the program path if it contains a forward slash.

@nojb nojb requested a review from dra27 September 3, 2024 09:53
@nojb
Copy link
Copy Markdown
Contributor Author

nojb commented Sep 3, 2024

(The CI failure is unrelated.)

@nojb nojb added this to the 5.3 milestone Sep 3, 2024
@dra27
Copy link
Copy Markdown
Member

dra27 commented Sep 4, 2024

For cmd, I think we still have some fairly tight limits on command length to worry about - might it be better to convert forward slashes to backslashes instead?

@nojb
Copy link
Copy Markdown
Contributor Author

nojb commented Sep 4, 2024

For cmd, I think we still have some fairly tight limits on command length to worry about - might it be better to convert forward slashes to backslashes instead?

Yes. Also, this quoting solution may not work in all cases, see the discussion at ocaml/dune#10869. I will revise the patch.

@nojb
Copy link
Copy Markdown
Contributor Author

nojb commented Sep 4, 2024

For cmd, I think we still have some fairly tight limits on command length to worry about - might it be better to convert forward slashes to backslashes instead?

I did this. Do you mind taking a look? Thanks!

Copy link
Copy Markdown
Member

@damiendoligez damiendoligez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. I have a small suggestion to simplify the test.

Comment on lines +106 to +109
if not (Sys.file_exists "sub") then Sys.mkdir "sub" 0o777;
copy_file "myecho.exe" "sub/myecho.exe";
printf "-------- Forward slashes in program position\n";
run "sub/myecho.exe" ["alea iacta est"]
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could simplify like this:

Suggested change
if not (Sys.file_exists "sub") then Sys.mkdir "sub" 0o777;
copy_file "myecho.exe" "sub/myecho.exe";
printf "-------- Forward slashes in program position\n";
run "sub/myecho.exe" ["alea iacta est"]
printf "-------- Forward slashes in program position\n";
run "./myecho.exe" ["alea iacta est"]

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion applied. Thanks @damiendoligez!

@nojb nojb merged commit b3eb4a8 into ocaml:trunk Sep 11, 2024
@nojb nojb deleted the fix_quote_comamnd_win32 branch September 11, 2024 15:39
nojb added a commit that referenced this pull request Sep 11, 2024
@nojb
Copy link
Copy Markdown
Contributor Author

nojb commented Sep 11, 2024

Cherry-picked to 5.3: 8a1ba11

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.

Filename.quote_command on Win32 does not handle forward slashes in program path correctly

3 participants