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

Update powershell conf to select from powershell and pwsh #17416

Merged
merged 30 commits into from
Dec 16, 2024

Conversation

czoido
Copy link
Contributor

@czoido czoido commented Dec 5, 2024

Changelog: Feature: Updated tools.env.virtualenv:powershell conf to allow specifying the PowerShell executable (e.g., powershell.exe or pwsh) and passing additional arguments.
Changelog: Feature: Deprecate use of tools.env.virtualenv:powershell=True/False.
Docs: conan-io/docs#3947

This will change how our powershell scripts are executed from:

powershell.exe "&'C:\path\conanrunenv.ps1'" ; "&'C:\path\conanbuildenv.ps1'" ; cmd /c mycompiler0.bat

to

powershell.exe -Command "&'C:\path\conanrunenv.ps1' ; &'C:\path\conanbuildenv.ps1'" ; cmd /c mycompiler0.bat
pwsh -Command "&'C:\path\conanrunenv.ps1' ; &'C:\path\conanbuildenv.ps1'" ; cmd /c mycompiler0.bat

Depending if we select to use pwsh (version 7) or powershell.exe

Not exactly meant to close this: #17339 but related to that.

@czoido czoido added this to the 2.11.0 milestone Dec 5, 2024

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
@czoido czoido marked this pull request as ready for review December 10, 2024 11:47
@AbrilRBS AbrilRBS self-assigned this Dec 10, 2024

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
@czoido czoido marked this pull request as draft December 11, 2024 07:49
@czoido czoido marked this pull request as ready for review December 12, 2024 07:43
czoido and others added 2 commits December 12, 2024 08:43

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
Co-authored-by: Abril Rincón Blanco <git@rinconblanco.es>
Copy link
Member

@memsharded memsharded left a comment

Choose a reason for hiding this comment

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

We have this ticket in #17434, that the user was affected by their powershell profile changing the cwd. This could be avoided by -noProfile, but as @Todiq points out in #17434 (comment), this might break users that rely on this.

We could add yet another conf, or maybe allow users to do tools.env.virtualenv:powershell="powershell -noProfile" or any other arguments they might want to provide?

@czoido
Copy link
Contributor Author

czoido commented Dec 12, 2024

We have this ticket in #17434, that the user was affected by their powershell profile changing the cwd. This could be avoided by -noProfile, but as @Todiq points out in #17434 (comment), this might break users that rely on this.

We could add yet another conf, or maybe allow users to do tools.env.virtualenv:powershell="powershell -noProfile" or any other arguments they might want to provide?

I can think of two alternatives:

  • Using a dict instead a str and passing the shell and arguments, so it would be like:
tools.env.virtualenv:powershell={"shell": "pwsh", "args": ["-NoProfile", "-ExecutionPolicy", "Bypass"]}

or

tools.env.virtualenv:powershell={"shell": "powershell.exe", "args": ["-NoProfile", "-ExecutionPolicy", "Bypass"]}
  • Using a new conf for arguments:
tools.env.virtualenv:powershell_args=["-NoProfile", "-ExecutionPolicy", "Bypass"]

What do you think? @memsharded @AbrilRBS

@memsharded
Copy link
Member

I'd probably go for the 1 configuration if we still support the basic string simple case, good for vast majority of users: tools.env.virtualenv:powershell=pwsh, because defining a dict in the command line is always painful with the quoting. Then I like 1 configuration to avoid explosion of confs, and if it needs some further argument or variation we can just add them to the dict.

@czoido
Copy link
Contributor Author

czoido commented Dec 12, 2024

I'd probably go for the 1 configuration if we still support the basic string simple case, good for vast majority of users: tools.env.virtualenv:powershell=pwsh, because defining a dict in the command line is always painful with the quoting. Then I like 1 configuration to avoid explosion of confs, and if it needs some further argument or variation we can just add them to the dict.

Let's try with the simpler version of using a string like tools.env.virtualenv:powershell="powershell.exe -noProfile" because it's true that dicts are not the best for passing in the CLI.

@AbrilRBS
Copy link
Member

I agree - we just might want to make sure the docs mention that you're only ever supposed to use powershell.exe/pwsh.exe

conan/tools/microsoft/visual.py Outdated Show resolved Hide resolved
wip
fix
fix
Comment on lines 55 to 57
powershell = "powershell.exe" if conanfile.conf.get(
"tools.env.virtualenv:powershell") is True else conanfile.conf.get("tools.env.virtualenv:powershell")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We have to check if this is True to provide compatibility with the deprecated syntax.
The problem is that conanfile.conf.get("tools.env.virtualenv:powershell", check_type=str) does not work because it will evaluate True or False as strings and not raise any exceptions.

@czoido czoido merged commit 77a7051 into conan-io:develop2 Dec 16, 2024
33 checks passed
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.

None yet

3 participants