Skip to content

Fix VS2022 Build Tools installation removing VS2019, fixes #1175#4288

Merged
atalman merged 1 commit intopytorch:mainfrom
Blackhex:fix-vs2022-install
Jun 13, 2023
Merged

Fix VS2022 Build Tools installation removing VS2019, fixes #1175#4288
atalman merged 1 commit intopytorch:mainfrom
Blackhex:fix-vs2022-install

Conversation

@Blackhex
Copy link
Copy Markdown
Contributor

@Blackhex Blackhex commented Jun 13, 2023

Fixes issue that the VS2022 Build Tools installation is removing VS2019 Build Tools installation. This is a follow up fix of already merged #1175.

@vercel
Copy link
Copy Markdown

vercel bot commented Jun 13, 2023

@Blackhex is attempting to deploy a commit to the Meta Open Source Team on Vercel.

A member of the Team first needs to authorize it.

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jun 13, 2023

if (Test-Path "${env:ProgramFiles(x86)}\Microsoft Visual Studio\Installer\vswhere.exe") {
$existingPath = & "${env:ProgramFiles(x86)}\Microsoft Visual Studio\Installer\vswhere.exe" -products "Microsoft.VisualStudio.Product.BuildTools" -version "[${env:VS_VERSION}, ${VS_VERSION_major + 1})" -property installationPath
$existingPath = & "${env:ProgramFiles(x86)}\Microsoft Visual Studio\Installer\vswhere.exe" -products "Microsoft.VisualStudio.Product.BuildTools" -version "[${env:VS_VERSION}, $($VS_VERSION_major + 1))" -property installationPath
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.

The former syntax was incorrect. The "${env:VS_VERSION}, ${VS_VERSION_major + 1}" expression evaluates to "". See https://www.pdq.com/blog/using-subexpressions-within-strings/ for more details.

exit 1
}

if ($pathToRemove -ne $null) {
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.

Swapping $null and $pathToRemove order is a VSCode lint suggestion.

# Install the Visual Studio 2019
provisioner "powershell" {
environment_vars = ["INSTALL_WINDOWS_SDK=1", "VS_YEAR=2019", "VS_VERSION=16.11.21"]
environment_vars = ["INSTALL_WINDOWS_SDK=1", "VS_YEAR=2019", "VS_VERSION=16.11.21", "VS_UNINSTALL_PREVIOUS=1"]
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.

Although the introduced VS_UNINSTALL_PREVIOUS variable logic breaks the current behavior, IMO, it's better to keep the VS installation by default. It does not seem that the Install-VS.ps1 script is used anywhere else so it should not be an issue anyway.

Copy link
Copy Markdown
Contributor

@atalman atalman Jun 13, 2023

Choose a reason for hiding this comment

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

@Blackhex should we add VS_UNINSTALL_PREVIOUS=0 to Visual Studio 2022 install ?

Copy link
Copy Markdown
Contributor

@atalman atalman left a comment

Choose a reason for hiding this comment

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

LGTM

@Blackhex Blackhex force-pushed the fix-vs2022-install branch from f4857bd to 4103bfe Compare June 13, 2023 13:20
@atalman
Copy link
Copy Markdown
Contributor

atalman commented Jun 13, 2023

Merging this - tested in canary with success

@atalman atalman merged commit 8f97379 into pytorch:main Jun 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants