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.10 Preview 3 #4576

Merged
merged 28 commits into from
Apr 12, 2024

Conversation

StephanTLavavej
Copy link
Member

📜 Changelog

  • Fixed bugs:
    • Fixed is_trivial to correctly handle unusual types.
  • Code cleanups:
    • Removed compiler bug workarounds.
  • Infrastructure improvements:
    • Updated dependencies.
      • Updated build compiler to VS 2022 17.10 Preview 3.
      • Updated Python to 3.12.3.

💡 Non-Changelog Overview

  • This fixes our scripts for preparing 1ES Hosted Pools (after create-1es-hosted-pool.ps1: Avoid ConvertTo-SecureString #4535 broke them), and massively overhauls/simplifies them (also getting rid of the weird "run as admin" dance).
  • The resulting pools should require slightly less storage because we're now cleaning up installer executables after running them, and not installing Python docs.
  • To meet new security requirements, I'm dropping our use of pip to install psutil, which gives up our ability to have per-test timeouts, but we weren't relying on them anyways (and I don't use them locally). We still have overall timeouts enforced by Azure Pipelines.

⚙️ Commits

  • Drop the PsExec dance, but keep the PowerShell 7.4.1 upgrade.
    • The PsExec "switching to AdminUser" machinery was introduced back in Add scripts to generate new Virtual Machine Scale Sets and images for build machines #633 but is no longer necessary (perhaps due to changes in Azure itself, Azure PowerShell, or our scripts). We can run our installers with whatever user Invoke-AzVMRunCommand gives us, and everything works.
    • For now, I've retained the "download modern PowerShell and invoke ourselves to upgrade it" machinery. This isn't actually necessary (the entire script could run as old PowerShell) and removing it would simplify things. However, modern PowerShell has lots of conveniences, and we might want to use them in the future even if the beginning of the script has to remain old-compatible. I think the way that I've structured this is easier to understand than the old dance, but I could be persuaded to drop it entirely.
  • Add exit to improve the transcript.
    • This eliminates weird lines:
      PS>$global:?
      True
      
  • We don't need the PowerShell call operator when invoking curl.exe.
    • This was the only affected invocation.
  • Cleanup the transcript and all downloaded files.
    • This should make the VM image slightly smaller.
  • Extract Python args.
    • This is a step towards unification in a later commit.
  • Wrap Python installation in try-catch.
    • Ditto.
  • For Python, use /quiet instead of /passive, and don't install docs.
    • This should save 50 MB.
    • See Using Python on Windows:
      • /quiet means "to install/uninstall without displaying any UI"
      • /passive means "to display progress without requiring user interaction"
  • Move $ProgressPreference to the top, drop duplicate $ErrorActionPreference.
  • We can directly invoke pwsh, no need for a transcript.
  • Dramatically simplify code with Get-SecureRandom.
  • Suppress breaking change warnings from Update-AzConfig itself.
  • Simply inline PrintMsiExitCodeMessage.
  • Simplify script output.
  • Directly invoke the VS installer without cmd.exe /c.
    • I'm not sure why this technique was being used, but it was unnecessary.
  • Grand unified DownloadAndInstall.
    • This significantly simplifies the script by invoking our installers in a uniform manner.
  • Always disable positional binding, make all parameters mandatory.
    • This is a modern convention.
  • Fix VSO-1784520 by dropping psutil and --timeout=240.
    • Without psutil, attempting to use --timeout emits:
      stl-lit.py: D:\GitHub\STL\llvm-project\llvm\utils\lit\lit\main.py:72: note: The test suite configuration requested an individual test timeout of 0 seconds but a timeout of 240 seconds was requested on the command line. Forcing timeout to be 240 seconds.
      stl-lit.py: D:\GitHub\STL\llvm-project\llvm\utils\lit\lit\LitConfig.py:129: fatal: Setting a timeout per test not supported. Requires the Python psutil module but it could not be found. Try installing it via pip or via your operating system's package manager.
      
  • Simplify output.
  • Python 3.12.3.
  • New-AzGalleryImageVersion needs -SourceImageVMId in Azure PowerShell 11.5.0.
  • VS 2022 17.10 Preview 3.
  • Print whether pool creation was successful.
    • I've seen this script appear to succeed, except that the pool isn't actually created (e.g. due to core quota limitations). This can waste a bunch of time, if I create a PR and then have to investigate why it isn't working. I'm not sure why we aren't getting a failure from New-AzResource (and investigating by running the script is extremely time-consuming), but it's simple to add a final check that the pool resource exists - if it doesn't, then we're definitely doomed.
  • Style: } else {
  • New pool.
  • Remove _VCRT_EXPORT_STD workarounds.
    • std.ixx was explicitly including <yvals_core.h> for the workaround, which is no longer necessary.
  • Remove workaround for VSO-1975579 "Standard Library Modules: fatal error C1116: unrecoverable error importing module 'std'. Specialization of 'std::invoke_result_t' with arguments '_Fn, _Ty...'".
  • Fix DevCom-1586179 VSO-1439353 by removing workarounds for VSO-119526 (fixed) and LLVM-41915 (still active).
    • VSO-119526 "Multiple versions of special member function should be allowed" was fixed after 11 years.
    • LLVM-41915 "Implement CWG-1496" is still active, but implementing is_trivial with __is_trivially_constructible(_Ty) && __is_trivially_copyable(_Ty) isn't a transparent workaround - it causes DevCom-1586179 VSO-1439353 "std::is_trivial incorrectly fails because of protected defaulted default constructor". Let's just use the dedicated compiler builtin, and blame the compiler if it goes wrong - we've moved away from attempting to compensate for compiler deficiencies in type traits.
  • Fix test: ranges::dangling is non-trivial (properly detected by Clang), but (portably) trivially default constructible.

StephanTLavavej and others added 28 commits April 9, 2024 15:50
This eliminates weird lines:

```
PS>$global:?
True
```
I had removed this environment variable technique in GH 3651, saying "The previous method stopped working.", but it clearly works here now. Let's keep both.
```
stl-lit.py: D:\GitHub\STL\llvm-project\llvm\utils\lit\lit\main.py:72: note: The test suite configuration requested an individual test timeout of 0 seconds but a timeout of 240 seconds was requested on the command line. Forcing timeout to be 240 seconds.
stl-lit.py: D:\GitHub\STL\llvm-project\llvm\utils\lit\lit\LitConfig.py:129: fatal: Setting a timeout per test not supported. Requires the Python psutil module but it could not be found. Try installing it via pip or via your operating system's package manager.
```
`std.ixx` was explicitly including `<yvals_core.h>` for the workaround, which is no longer necessary.
VSO-1975579 "Standard Library Modules: fatal error C1116: unrecoverable error importing module 'std'.
Specialization of 'std::invoke_result_t' with arguments '_Fn, _Ty...'".
… (fixed) and LLVM-41915 (still active).

VSO-119526 "Multiple versions of special member function should be allowed" was fixed after 11 years.

LLVM-41915 "Implement CWG 1496" is still active, but implementing `is_trivial` with `__is_trivially_constructible(_Ty) && __is_trivially_copyable(_Ty)`
isn't a transparent workaround - it causes DevCom-1586179 VSO-1439353 "std::is_trivial incorrectly fails because of protected defaulted default constructor".
Let's just use the dedicated compiler builtin, and blame the compiler if it goes wrong - we've moved away from attempting to compensate for compiler deficiencies in type traits.
…ng), but (portably) trivially default constructible.
@StephanTLavavej StephanTLavavej added the infrastructure Related to repository automation label Apr 10, 2024
@StephanTLavavej StephanTLavavej requested a review from a team as a code owner April 10, 2024 22:45
@StephanTLavavej
Copy link
Member Author

/azp run STL-ASan-CI

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@StephanTLavavej
Copy link
Member Author

STL-ASan-CI passed.

Copy link
Member

@CaseyCarter CaseyCarter left a comment

Choose a reason for hiding this comment

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

This is amazing - so much red!

@StephanTLavavej StephanTLavavej self-assigned this Apr 12, 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 c0fe1fa into microsoft:main Apr 12, 2024
35 checks passed
@StephanTLavavej StephanTLavavej deleted the image-comics branch April 12, 2024 18:09
@BillyONeal
Copy link
Member

microsoft/vcpkg#40658 I guess I ended up somewhere pretty close with a recent complaint about ConvertTo-SecureString 😅

We don't need the PowerShell call operator when invoking curl.exe

I believe that this was extra ultra paranoia of avoiding PowerShell's alias of "curl" to "Invoke-WebRequest".

Directly invoke the VS installer without cmd.exe /c.
I'm not sure why this technique was being used, but it was unnecessary.

I think that whole mess was copy pasta from somewhere else; getting this to do the right thing at least at one time or another was a pain because the installer binary would return control to the console immediately.

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.

3 participants