-
Notifications
You must be signed in to change notification settings - Fork 989
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
fix run with nested quotes #17487
fix run with nested quotes #17487
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @Todiq
Thanks very much for your contribution.
It would be great (mostly necessary to guarantee that this keeps working fine in the future) to add some unittest. You might be able to copy&paste some existing test from test_virtualenv_powershell.py
and use it as inspiration and start point?
If you can't don't worry, we can help and add the test. But it would be great to have some code or instructions to reproduce the original issue fixed by this. Thanks!
I'll try first to fix the tests that I have broken, then I'll add some, sure 👌 |
conan/tools/env/environment.py
Outdated
@@ -59,6 +59,7 @@ def environment_wrap_command(conanfile, env_filenames, env_folder, cmd, subsyste | |||
launchers = " && ".join('"{}"'.format(b) for b in bats) | |||
if ps1s: | |||
ps1_launchers = f'{powershell} -Command "' + " ; ".join('&\'{}\''.format(f) for f in ps1s) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
Hi @Todiq, I have pushed some changes to your branch, please can you check if those changes solve your issue and the test there represents the original issue? |
name = "pkg" | ||
version = "0.1" | ||
def build(self): | ||
self.run('python -c "print(\\'Hello World\\')"', scope="build") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this tries to replicate what happens when doing:
conan install --requires=boost/1.85.0 --build=boost/* -c:a tools.env.virtualenv:powershell=pwsh -o "boost/*:without_python=False"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changelog: Bugfix: Fix running commands in powershell with single quotes.
Docs: Omit
Closes #17486
develop
branch, documenting this one.