-
Notifications
You must be signed in to change notification settings - Fork 993
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
Add support for vcvars when using Powershell #15461
Add support for vcvars when using Powershell #15461
Conversation
conan/tools/microsoft/visual.py
Outdated
generator_path = conanfile.generators_path | ||
content_ps1 = textwrap.dedent(f"""\ | ||
pushd "{generator_path}" | ||
cmd /c "conanvcvars.bat&set" | |
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.
we could use $PSScriptRoot/conanvcvars.bat
instead of directly by name? this would mean we don't have to do pushd
and popd
- and use the same structure as conanbuild.ps1
already does - unless I'm missing something?
(see docs)
conans/test/integration/toolchains/env/test_virtualenv_powershell.py
Outdated
Show resolved
Hide resolved
The deactivate environments needs fixing, I'm looking into it. Edit: It was already broken when trying to deactivate conanvcvars envs, it will be fixed in a different PR. |
if is_ps1: | ||
content_ps1 = textwrap.dedent(f"""\ | ||
if (-not $env:VSCMD_ARG_VCVARS_VER){{ | ||
Push-Location "$PSScriptRoot" |
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.
I reverted this back to using "push/pop" as calling the path directly in the cmd command was causing trouble when path had spaces.
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.
Looks good!
conans/test/functional/toolchains/env/test_virtualenv_powershell.py
Outdated
Show resolved
Hide resolved
cmd /c "conanvcvars.bat&set" | | ||
foreach {{ | ||
if ($_ -match "=") {{ | ||
$v = $_.split("=", 2); set-item -force -path "ENV:\$($v[0])" -value "$($v[1])" | ||
}} | ||
}} | ||
Pop-Location | ||
write-host conanvcvars.ps1: Activated environment}} | ||
""") |
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.
I know no-one asked for my opinion ( :) ) but this makes me uneasy.
here's how you could do it similarly than the Launch-VsDevShell.ps1 ($vspath\Common7\Tools\Launch-VsDevShell.ps1
)
$vspath=$(vswhere -products '*' -requires Microsoft.Component.MSBuild -property installationPath -latest)
$devShellModule = "$vspath\Common7\Tools\Microsoft.VisualStudio.DevShell.dll"
Import-Module $devShellModule
Enter-VsDevShell -VsInstallPath $vspath -SkipAutomaticLocation -Arch amd64
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.
Get-Help Enter-VsDevShell
NAME
Enter-VsDevShell
SYNTAX
Enter-VsDevShell -VsInstallPath <string> [-SkipExistingEnvironmentVariables] [-StartInPath <string>] [-Arch {Default | x86 | amd64 | arm | arm64}] [-HostArch {Default | x86 | amd64}]
[-DevCmdArguments <string>] [-DevCmdDebugLevel {None | Basic | Detailed | Trace}] [-SkipAutomaticLocation] [-SetDefaultWindowTitle] [-ReportNewInstanceType {PowerShell | Cmd |
LaunchScript}] [<CommonParameters>]
Enter-VsDevShell [-VsInstanceId] <string> [-SkipExistingEnvironmentVariables] [-StartInPath <string>] [-Arch {Default | x86 | amd64 | arm | arm64}] [-HostArch {Default | x86 |
amd64}] [-DevCmdArguments <string>] [-DevCmdDebugLevel {None | Basic | Detailed | Trace}] [-SkipAutomaticLocation] [-SetDefaultWindowTitle] [-ReportNewInstanceType {PowerShell | Cmd
| LaunchScript}] [<CommonParameters>]
Enter-VsDevShell [-Arch {Default | x86 | amd64 | arm | arm64}] [-HostArch {Default | x86 | amd64}] [-Test] [-DevCmdDebugLevel {None | Basic | Detailed | Trace}] [<CommonParameters>]
ALIASES
None
REMARKS
None
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 @jmarrec - thanks for your feedback.
This is the approach we considered initially, but discarded it for a couple of reasons:
Launch-VsDevShell.ps1
and the underlying implementation do not expose the same interface that vcvars provides, in particular the ability to specify the toolset with the-vcvars_ver
, or using the Windows Store SDK, or specifying the WIndows SDK version. We do rely on mapping Conan settings/ conf to some of these - and the interface for the powershell ones do not expose these.- The documented way would be to invoke
Launch-VsDevShell.ps1
once located - rather than importingDevShell.dll
- this is evidenced:- in the documentation, where it says:
Launch-VsDevShell.ps1 is the recommended way to initialize Developer PowerShell interactively or for scripting build automation.
- This response by a Microsoft employee, where it says that the interface to
Enter-VsDevShell
is considered undocumented and can change without notice, and once again reiterates thatLaunch-VsDevShell.ps1
is to be used - https://developercommunity.visualstudio.com/t/MicrosoftVisualStudioDevShell-document/1348197
we wouldn't want to use undocumented/subject to breakages features, and the Launch-VsDevShell.ps1
does not provide feature parity, so we relied on this fallback.
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.
Gotcha, thanks a lot for the detailed explanation!
Changelog: Feature: Add support for use of vcvars env variables when calling from powershell.
Docs: conan-io/docs#3541
Trying to activate the generated
conanbuild.ps1
file to set the environment from a Powershell console was not working.To fix this there's now a new
conanvcvars.ps1
file that saves the environment from callingconanvcvars.bat
to be used in the Powershell.Fixes: #15267