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

Use "fosskers/rs-versions" to determine the latest CRT and SDK versions. #67

Merged
merged 1 commit into from
Nov 30, 2022

Conversation

mite-user
Copy link
Contributor

Problem with 17.4.1 manifest

xwin/src/lib.rs

Lines 342 to 350 in 5ece33c

let crt_version = build_tools
.dependencies
.keys()
.filter_map(|key| {
key.strip_prefix("Microsoft.VisualStudio.Component.VC.")
.and_then(|s| s.strip_suffix(".x86.x64"))
})
.last()
.context("unable to find latest CRT version")?;

The latest CRT version is obtained from a dependency of Microsoft.VisualStudio.Product.BuildTools package.
A dependency with a CRT version is expected to begin with Microsoft.VisualStudio.Component.VC. and end with .x86.x64.
The latest version is determined by string ordering.
In the current 17.4.1 version of the manifest, there is a dependency Microsoft.VisualStudio.Component.VC.Modules.x86.x64, so xwin determines that the latest CRT version is Modules.

Executing

xwin --accept-license --manifest-version=17 download

causes an error

Error: unable to find CRT headers item 'Microsoft.VC.Modules.CRT.Headers.base'

xwin can't find Microsoft.VC.{crt_version}.CRT.Headers.base package in the manifest because {crt_version} is Modules.

Potential problem with string ordering

String ordering might fail on some version numbers,
e.g. if the dependencies are

Microsoft.VisualStudio.Component.VC.15.9.x86.x64
Microsoft.VisualStudio.Component.VC.15.10.x86.x64

the latest version will be determined as 15.9.

xwin/src/lib.rs

Lines 522 to 526 in 5ece33c

let sdk = pkgs
.values()
.filter(|mi| mi.id.starts_with("Win10SDK_10."))
.max()
.context("unable to find latest Win10SDK version")?;

The latest SDK version is also determined by string ordering.

List of existing versions from currently available manifests

Might be useful to determine if changes from this pull request are needed.
Looks like recent "CRT version" is just a concatenation of 14.* real CRT version and manifest version (which matches Visual Studio version) and they usually increase simultaneously.
String ordering might work if there will be no major CRT version increase.

CRT versions

15.9.51 manifest (matching Microsoft.VisualStudio.Component.VC.Tools.*)

14.11
14.12
14.13
14.14
14.15

16.11.21 manifest (matching Microsoft.VisualStudio.Component.VC.*.x86.x64)

14.20
14.21
14.22
14.23
14.24
14.25
14.26
14.27
14.28.16.9
14.28
14.29.16.10

17.4.1 manifest (matching Microsoft.VisualStudio.Component.VC.*.x86.x64)

14.29.16.11
14.30.17.0
14.31.17.1
14.32.17.2
14.33.17.3
14.34.17.4

SDK versions (matching Win10SDK_*)

15.9.51 manifest

10.0.10586.212
10.0.14393.795
10.0.15063
10.0.16299
10.0.17134
10.0.17763

16.11.21 manifest

10.0.16299
10.0.17134
10.0.17763
10.0.18362
10.0.19041
10.0.20348

17.4.1 manifest

10.0.18362
10.0.19041
10.0.20348

The solution

Use versions crate to compare versions.
Real versions will take precedence over obvious non-versions because an alphanumeric is less than a numeric with rs-versions.

Why not other crates.

https://github.com/timvisee/version-compare.git
Only PartialOrd is implemented, .max() doesn't work, need to write .max_by(|a, b| a.compare(b).ord().unwrap()).

https://github.com/dtolnay/semver.git
Expects exactly 3 numbers separated by dots.

String ordering might fail on some version numbers, e.g. 15.9 vs 15.10
Real versions will take precedence over obvious non-versions
because an alphanumeric is less than a numeric with rs-versions.
Copy link
Owner

@Jake-Shadle Jake-Shadle left a comment

Choose a reason for hiding this comment

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

Seems reasonable, thanks for the detailed description!

@Jake-Shadle Jake-Shadle merged commit 765848b into Jake-Shadle:main Nov 30, 2022
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