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.

Sign up for GitHub

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

fix run with nested quotes #17487

Merged
merged 9 commits into from
Dec 18, 2024
Merged

fix run with nested quotes #17487

Changes from 2 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
839eb2f
fix run with nested quotes
Todiq Dec 17, 2024
8e71b29
fix escaping of double quotes
Todiq Dec 18, 2024
e546d30
Merge branch 'develop2' into fix-nested-quotes
Todiq Dec 18, 2024
8e7c9bf
Merge branch 'develop2' into fix-nested-quotes
Todiq Dec 18, 2024
5d91413
add some tests for quoted commands
Todiq Dec 18, 2024
efebba4
simplify test and fix quoting
czoido Dec 18, 2024
1bafe87
only win
czoido Dec 18, 2024
7f71fb4
add to right test
czoido Dec 18, 2024
050bb7d
Merge branch 'develop2' into fix-nested-quotes
Todiq Dec 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions conan/tools/env/environment.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,18 +58,18 @@ def environment_wrap_command(conanfile, env_filenames, env_folder, cmd, subsyste
if bats:
launchers = " && ".join('"{}"'.format(b) for b in bats)
if ps1s:
ps1_launchers = f'{powershell} -Command "' + " ; ".join('&\'{}\''.format(f) for f in ps1s) + '"'
cmd = cmd.replace('"', "'")
return '{} && {} ; cmd /c {}'.format(launchers, ps1_launchers, cmd)
ps1_launchers = f'{powershell} -Command "' + " ; ".join('&\'{}\''.format(f) for f in ps1s)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hello @czoido

I am pretty sure the .join('&\'{}\''.format(f) for f in ps1s) could be changed as well to .join('{}'.format(f) for f in ps1s)
The minimal local tests I have run seem to go well with this change and it does improve readability in my opinion
I think the & syntax is not necessary with the -Command anymore. Am I mistaken?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @Todiq,
Removing the & when invoking the scripts causes that the environment variables that should be injected from the ps1 files will not be applied correctly in the execution contexts, so I'm afraid those are necessary.
Is it possible to give us an small reproducible example to be able to add a test to check what this PR is fixing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just added some tests.
Reproducible case is here : conan-io/conan-center-index#26187

Copy link
Contributor

@czoido czoido Dec 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Thank you so much @Todiq , I could reproduce the issue, let me check what you added and simplify a bit.

cmd = cmd.replace('"', r'\"')
return '{} && {} ; cmd /c {}"'.format(launchers, ps1_launchers, cmd)
czoido marked this conversation as resolved.
Show resolved Hide resolved
else:
return '{} && {}'.format(launchers, cmd)
elif shs:
launchers = " && ".join('. "{}"'.format(f) for f in shs)
return '{} && {}'.format(launchers, cmd)
elif ps1s:
ps1_launchers = f'{powershell} -Command "' + " ; ".join('&\'{}\''.format(f) for f in ps1s) + '"'
cmd = cmd.replace('"', "'")
return '{} ; cmd /c {}'.format(ps1_launchers, cmd)
ps1_launchers = f'{powershell} -Command "' + " ; ".join('&\'{}\''.format(f) for f in ps1s)
cmd = cmd.replace('"', r'\"')
return '{} ; cmd /c {}"'.format(ps1_launchers, cmd)
czoido marked this conversation as resolved.
Show resolved Hide resolved
else:
return cmd

Expand Down