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 to Rust/Cargo v1.75 and simplify Windows Conda setup #9935

Merged
merged 20 commits into from
Oct 28, 2024

Conversation

oskirby
Copy link
Collaborator

@oskirby oskirby commented Oct 4, 2024

Description

It took me three tries to get this done, but this should upgrade our Cargo/Rust toolchain to version v1.75 up from v1.69. The work was fairly straightforward for every platform except Windows, where all we really needed to do was bump a version number and work around some minor build quirks.

Windows however, turned out to be a beast, and I found it unable to build after touching the rust crates. This forced me to finally reckon with my personal development machine and figure out why it never worked with Conda. A quick tour of the changes on Windows that made this all work for me:

  • The rust-std-apple-xxx packages now have a dependency on the pseudo-package __unix which makes them uninstallable on Windows. For this reason, I felt it was best to split out conda env.yml file into a UNIX and Windows variant. This enabled some other cleanup in the Windows env too and to omit the extras.ps1 script.
  • A bump to Clang/LLVM 16.x.x brings with it significant improvement in the ability to autodetect the Visual Studio installation and any associated SDKs. This allows the Conda toolchain to work with an existing VS2019 installation.
  • We write a toolchain file scripts/windows/conda-toolchain.cmake that can be used as a one-stop-shop to configure CMake with Clang-CL and the conda environment. This effectively replaces all of the environment hacking in the old conda_setup_win_sdk.ps1 script.
  • We auto-generate a config.toml for Windows that passes the CMake toolchain to cargo this eliminates much of the environment hacking needed to get cargo to build.
  • I wrote a new conda-setup-xwin-sdk.ps1 script using the latest version Jake-Shadle/xwin project using the new --use-winsysroot-style option which allows us to massively simplify the arguments to Clang and LLVM in order to connect the two.
  • The MSM/MergeModules aren't installed by the Jake-Shadle/xwin project, and word on the internet is that they are only required to support Windows 8 and earlier. Given that we explicitly require Windows 10 (and even block installation on older version) we can just remove this and it ought to work.
  • The MSM/MergeModules aren't installed by the Jake-Shadle/xwin project, so in order to have them available for the Wix installer, it was necessary to write a new tool scripts/windows/fetch-vsix-package.py that can parse the Visual Studio manifests and download VSIX extensions. This tool is then used to fetch the vcredit MergeModules into the xwin SDK.

I think this was especially painful for me because my Windows machine has MS Visual Studio 2019 installed for other projects too and I am hesitant to uninstall it to get conda working. Hopefully with these changes we should get a working build environment both with and without VS2019 installed.

Reference

First Attempt: #9812
Second Attempt: #9914

Checklist

  • My code follows the style guidelines for this project
  • I have not added any packages that contain high risk or unknown licenses (GPL, LGPL, MPL, etc. consult with DevOps if in question)
  • I have performed a self review of my own code
  • I have commented my code PARTICULARLY in hard to understand areas
  • I have added thorough tests where needed

@oskirby oskirby requested a review from a team as a code owner October 4, 2024 19:52
@oskirby oskirby requested review from jcristau and removed request for a team October 4, 2024 19:52
@oskirby oskirby marked this pull request as draft October 4, 2024 19:53
@oskirby oskirby removed the request for review from jcristau October 4, 2024 19:53
@oskirby oskirby force-pushed the naomi-tries-win-conda branch 7 times, most recently from 4dd8bf9 to 7b7d7ad Compare October 5, 2024 00:22
@oskirby oskirby changed the title Experiment: Rust and Conda hacking for Windows Update to Rust/Cargo v1.75 and simplify Windows Conda setup Oct 8, 2024
@oskirby oskirby marked this pull request as ready for review October 8, 2024 17:33
@oskirby oskirby requested a review from strseb October 9, 2024 08:48
env-windows.yml Outdated
- pip=22.3.1
- rust=1.75
- rust-std-armv7-linux-androideabi=1.75
- rust-std-x86_64-linux-android=1.75
Copy link
Collaborator

Choose a reason for hiding this comment

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

We don't support compiling android on windows, so no need to have those :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Huzzah!

<!--
Merge modules
-->
<DirectoryRef Id="MozillaVPNFolder">
Copy link
Collaborator

@strseb strseb Oct 9, 2024

Choose a reason for hiding this comment

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

I am waaaay to far away rn from a real windows PC to test this but afaik we require a VC_redist to be installed for the client to run. So if none are installed, (which is unlikey tbf) things might break, no ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

My understanding is that from Windows 10 onwards microsoft has switched to a universal CRT that no longer requires a vcredist, and since the WIX installer goes out of its way to deny installation on earlier versions of Windows then this shouldn't be needed anymore.

But yes, much testing is required.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Alas, I finally went and tested it and unfortunately it seems we really do need to install the MSVC redistributable somehow for the VPN client to run on Windows 10. Ugh. Back to the drawing board with this one.

Copy link
Collaborator Author

@oskirby oskirby Oct 24, 2024

Choose a reason for hiding this comment

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

Okay, so, I really don't want to have to download the entire VS environment just to extract the MSM/VCRedist package, but I can't really find a good link to it on its own. The official word from Microsoft is that vcredists as MergeModules are deprecated and that we should be using the vcredist_x64.exe intstead, which should be version-agnostic.

However, this isn't a great solution either because of how the vcredist is generated... see this thread for some complaints from the VS developer community, and some notes about how it prevents automated installation using MSIs.

Running the vcredist_[arch].exe from the Wix installer will fail with error 1618: Another installation is currently in progress.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@@ -12,13 +12,16 @@ task-defaults:
worker-type: b-win2022
fetches:
fetch:
- win-dev-env
Copy link
Collaborator

Choose a reason for hiding this comment

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

This line brings me so much joy.

@oskirby oskirby force-pushed the naomi-tries-win-conda branch from 60caccc to 74fc57a Compare October 9, 2024 16:05
@oskirby oskirby force-pushed the naomi-tries-win-conda branch from 74fc57a to 41b9461 Compare October 24, 2024 19:21
@oskirby oskirby requested a review from strseb October 25, 2024 19:45
@oskirby oskirby merged commit 0d8d2be into main Oct 28, 2024
115 checks passed
@oskirby oskirby deleted the naomi-tries-win-conda branch October 28, 2024 15:57
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.

2 participants