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

Toolset update: VS 2022 17.11 Preview 1 #4687

Merged
merged 29 commits into from
May 23, 2024

Conversation

StephanTLavavej
Copy link
Member

📜 Changelog

  • Infrastructure improvements:
    • Overhauled our Azure Pipelines machinery:
      • Improved the reliability of the submodule checkout task.
      • Improved how clang-format diffs are uploaded and logged.
    • Updated dependencies.
      • Updated build compiler to VS 2022 17.11 Preview 1.

⚙️ Azure Pipelines Commits

Followup to #4594. I manually tested all of the success and failure scenarios for these commits.

  • Detect provision-image.ps1 failure.
    • This fixes a major headache in the toolset update process. Previously, if provision-image.ps1 failed while downloading and installing components on the VM, it would stop, but create-1es-hosted-pool.ps1 would keep going on the maintainer's machine, creating a totally doomed pool. When failure occurs, Invoke-AzVMRunCommand doesn't indicate that, so I have a simple strategy - have the VM script print a special message for success, then have the maintainer script search for that message.
    • Use case-sensitive matching for 'PROVISION_IMAGE_SUCCEEDED'.
  • Permanently install PowerShell 7.4.2.
    • We'll need this for the following commits.
    • We need to preserve the original filename 'PowerShell-7.4.2-win-x64.msi' instead of generating 'RANDOM.exe', otherwise we won't be able to run it.
    • This means that provision-image.ps1 no longer attempts an "in-place upgrade" of the PowerShell version that's running it. This significantly simplifies the script, and we don't need any PowerShell 7.x features here.
  • Download installers to D:\installerTemp.
    • CUDA is 3 GB, so this may be helpful to make the script run faster.
    • On the VM, D: is fast temporary storage.
  • Guard against locally running provision-image.ps1.
    • I've never made this mistake, but let's make sure it never happens for any maintainers or contributors. (Obviously, checking for the computer being named 'PROTOTYPE' isn't absolutely perfect, but it's the simplest way to recognize the VM.)
    • This uses a case-sensitive comparison; the default is case-insensitive.
  • Skip a blank line emitted by cmd /c ver.
  • Set pwsh: true.
    • Needed for the following commit. I'm changing the 'Create Diff' task for consistency.
  • Fix Submodule checkout failure should short-circuit checks #4632.
  • Retry submodule checkout up to 4 times.
  • checkout-submodule.yml: Use case-sensitive -creplace and -ceq.
    • git submodule status emits lowercase hexits, and we pass lowercase 'llvm-project'.
    • Although PowerShell defaults to case-insensitive, choosing case-sensitive for string operations should align with C++ expectations and avoid surprises.
  • Style: Wrap Restart-AzVM arguments.
  • Move Get-AzVM up so we can use $VM.ID earlier.
    • $VM.ID and $VM.StorageProfile.OsDisk.Name are unchanged between these locations.
    • Calling Get-AzVM is necessary; the return value of New-AzVm isn't useful.
  • Inline away the Wait-Shutdown function.
    • The progress bar message makes the Write-Host messages unnecessary.
    • -ResourceId $VM.ID is much shorter than -ResourceGroupName $ResourceGroupName -Name $Name, allowing us to directly say 'PowerState/stopped' without wrapping.
    • The original implementation in Add scripts to generate new Virtual Machine Scale Sets and images for build machines #633 (named Start-WaitForShutdown) was verbose, justifying a separate function, but now a direct loop is clearer.
  • Consistently use $VM.ID.
  • Run sysprep with -ScriptString.
    • $ErrorActionPreference wasn't useful because sysprep is a native command.
    • Write-Host wasn't useful because we aren't printing the message returned by Invoke-AzVMRunCommand.
    • The PowerShell call operator & wasn't useful because we aren't expanding any variables here.
    • In addition to being slightly simpler, this avoids having a script file that someone might try to run.
  • Style: For New-AzVMConfig, use -VMName instead of the alias -Name.
  • New-AzVm defaults to Trusted Launch as of Azure PowerShell 11.0.0 (November 2023).
  • Give validationBuildOutputLocation a dedicated name, and clean it.
  • Inline create-prdiff.ps1, part 1.
    • No changes except for:
      • Drop redundant banner.
      • $DiffFile was never passed as an argument, so set it as an ordinary variable.
      • Indentation.
  • Inline create-prdiff.ps1, part 2.
  • Inline create-prdiff.ps1, part 3.
    • Overhaul messaging.
  • Inline create-prdiff.ps1, part 4.
    • Actually name the file format.diff before uploading, as this appears in the zip.
  • Inline create-prdiff.ps1, part 5.
    • Name the zip differently from the file within.
  • Inline create-prdiff.ps1, part 6.
    • Style: Avoid Yoda condition when checking git diff output.
  • Improve provision-image.ps1's synopsis/description.
    • They were talking about our old scale set system. Now they talk about hosted pools, and explain the overall process in more detail.

🧰 Toolset Update Commits

  • Require Node.js 22.2.0.
  • New pool.
  • VS 2022 17.11 Preview 1.
    • Not yet required - the internal toolset is still 19.40.
  • Remove compiler warning suppressions.
    • MSVC no longer emits warning C4521 'multiple copy constructors specified', since at least MSVC-PR-368907 on 2021-12-09.
  • Work around VSO-2064546 "EDG ICE when deriving from std::num_get".

STL-ASan-CI passed.

StephanTLavavej and others added 29 commits May 20, 2024 17:33
Use case-sensitive matching for 'PROVISION_IMAGE_SUCCEEDED'.
We need to preserve the original filename 'PowerShell-7.4.2-win-x64.msi' instead of generating 'RANDOM.exe'.
CUDA is 3 GB, so this may be helpful.
This uses a case-sensitive comparison; the default is case-insensitive.
See:
https://learn.microsoft.com/en-us/azure/devops/release-notes/2021/pipelines/sprint-195-update#automatic-retries-for-a-task

The release notes said "The failing task is retried immediately" but I now observe quadratic backoff, waiting 1, 4, 9 seconds.
…ested]

`git submodule status` emits lowercase hexits, and we pass lowercase 'llvm-project'.

Although PowerShell defaults to case-insensitive, choosing case-sensitive for string operations should align with C++ expectations and avoid surprises.
`$VM.ID` and `$VM.StorageProfile.OsDisk.Name` are unchanged between these locations.

Calling `Get-AzVM` is necessary; the return value of `New-AzVm` isn't useful.
The progress bar message makes the `Write-Host` messages unnecessary.

`-ResourceId $VM.ID` is much shorter than `-ResourceGroupName $ResourceGroupName -Name $Name`, allowing us to directly say `'PowerState/stopped'` without wrapping.

The original implementation in GH 633 (named `Start-WaitForShutdown`) was verbose, justifying a separate function, but now a direct loop is clearer.
`$ErrorActionPreference` wasn't useful because sysprep is a native command.

`Write-Host` wasn't useful because we aren't printing the message returned by `Invoke-AzVMRunCommand`.

The PowerShell call operator `&` wasn't useful because we aren't expanding any variables here.
No changes except for:

* Drop redundant banner.
* `$DiffFile` was never passed as an argument, so set it as an ordinary variable.
* Indentation.
Simplify by using the PowerShell redirection operator `>` instead of `Start-Process`.

https://learn.microsoft.com/en-us/powershell/module/microsoft.powershell.core/about/about_redirection?view=powershell-7.4#powershell-redirection-operators

Verified that the diff can be applied cleanly.
Actually name the file format.diff before uploading, as this appears in the zip.
Name the zip differently from the file within.
Style: Avoid Yoda condition when checking `git diff` output.
They were talking about our old scale set system. Now they talk about hosted pools, and explain the overall process in more detail.
Not yet required - the internal toolset is still 19.40.
MSVC no longer emits warning C4521 'multiple copy constructors specified',
since at least MSVC-PR-368907 on 2021-12-09.
@StephanTLavavej StephanTLavavej added the infrastructure Related to repository automation label May 22, 2024
@StephanTLavavej
Copy link
Member Author

I'm mirroring this to the MSVC-internal repo - please notify me if any further changes are pushed.

@StephanTLavavej StephanTLavavej merged commit 37b793c into microsoft:main May 23, 2024
39 checks passed
@StephanTLavavej StephanTLavavej deleted the vs17.11p1 branch May 23, 2024 20:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
infrastructure Related to repository automation
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Submodule checkout failure should short-circuit checks
2 participants