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

[vcpkg] Add semantic handling for nuget_version with two-segment versions #1549

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

FrankXie05
Copy link
Contributor

Description:

This PR introduces a minor enhancement to handle the nuget_version field more effectively when exporting NuGet packages. Specifically:

  • If the provided nuget_version has only one period (.), such as 1.2, it will be automatically transformed into 1.2.0 to align with NuGet's semantic versioning.
  • Versions already adhering to full semantic versioning (e.g., 1.2.1) or other formats remain unchanged.

Reason for Change:

  • NuGet requires fully qualified semantic versions. While NuGet itself will raise an error for invalid version formats (e.g., 1.2.), this change preemptively adjusts common cases like 1.2 to avoid such errors and improve the user experience.

Changes Made:

  • Modified the nuget_version handling logic to append .0 if only one period is found in the version string.

Testing:

  • Manually tested scenarios with:
    • 1.2 → transformed to 1.2.0
    • 1.2.1 → remains unchanged
  • Invalid formats like 1.2. → left as-is, allowing NuGet to handle errors as expected.
    nuget

Impact:
This change ensures better alignment with NuGet's versioning requirements and reduces common user errors when specifying a version.

Fix vcpkg issue: microsoft/vcpkg#18543
@GreyCat Could you please test this PR? :)

cc @BillyONeal

@BillyONeal
Copy link
Member

Did NuGet emit a warning or something that we are suppressing that we should be reporting instead rather than trying to guess what the resulting file name will be?

Do we know we are resolving the situation in the same way NuGet does?

@FrankXie05
Copy link
Contributor Author

Warnings and behavior with NuGet: This change is not intended to suppress any warnings or errors emitted by NuGet. NuGet will still throw errors for invalid formats (e.g. 1.2.), and I will not cover or speculate on what NuGet will ultimately do in such cases. Instead, this PR handles a specific common scenario (1.2), which is a valid format, but inconsistent with NuGet's fully qualified semantic versioning.

Consistency with NuGet: The logic implemented here mimics NuGet's expected behavior by appending .0 to versions with only a period (e.g. 1.2 → 1.2.0). To confirm consistency, I have manually tested the results against NuGet, and the output filenames and behavior are consistent with NuGet's expectations. However, if there are any other edge cases we should investigate, I'd be happy to explore further.

@BillyONeal
Copy link
Member

Instead, this PR handles a specific common scenario (1.2), which is a valid format, but inconsistent with NuGet's fully qualified semantic versioning.

So the existing behavior is "if the user enters 1.2 that fails when we try to invoke NuGet?"

The logic implemented here mimics NuGet's expected behavior by appending .0 to versions with only a period (e.g. 1.2 → 1.2.0).

NuGet's command line appends the .0?

@FrankXie05
Copy link
Contributor Author

No, vcpkg will not fail if you pass nuget_version to 1.2.
vcpkg's current behavior is that the version number you pass in does not match the version number you actually pass in.
The reason for this is that nuget's semver semantics will automatically add .0 suffix to versions that match the semantics of X.Y.

image

@JavierMatosD JavierMatosD added the requires:discussion This PR requires discussion of the correct way forward label Jan 9, 2025
@JavierMatosD
Copy link
Contributor

This looks fine to me, but will consult the team.

@BillyONeal
Copy link
Member

No, vcpkg will not fail if you pass nuget_version to 1.2.

But would it fail for a user trying to use nuget.exe directly? I guess what I'm saying is that I think we should match nuget.exe's behavior here. If nuget.exe fails, then we should turn this into an error rather than trying to fix it. If nuget.exe fixes it up somehow, we need to make sure that the fixup we apply exactly matches what nuget.exe does. If we are fixing it up somewhere else in the tool, then we should move that fixup out here rather than implementing a different version of it again.

vcpkg's current behavior is that the version number you pass in does not match the version number you actually pass in.

OK, so the console output is incorrect. What if the user enters something like "1", do they get "1.0.0"?

The reason for this is that nuget's semver semantics will automatically add .0 suffix to versions that match the semantics of X.Y.

I guess, what I want to see before we make a change like this is an understanding of where the extra .0 came from. Did nuget.exe do that? Did we?

@BillyONeal
Copy link
Member

The logic implemented here mimics NuGet's expected behavior by appending .0 to versions with only a period (e.g. 1.2 → 1.2.0).

To be clear, is nuget doing that, or is somewhere else in vcpkg doing that?

@JavierMatosD JavierMatosD marked this pull request as draft January 9, 2025 23:08
@FrankXie05
Copy link
Contributor Author

But would it fail for a user trying to use nuget.exe directly? I guess what I'm saying is that I think we should match nuget.exe's behavior here. If nuget.exe fails, then we should turn this into an error rather than trying to fix it. If nuget.exe fixes it up somehow, we need to make sure that the fixup we apply exactly matches what nuget.exe does. If we are fixing it up somewhere else in the tool, then we should move that fixup out here rather than implementing a different version of it again.

According to the NuGet package version reference, NuGet supports four version segments (Major.Minor.Patch.Revision), and only requires the definition of the Major segment, while the other segments are optional and default to zero. Therefore, version numbers 1, 1.0, 1.0.0, and 1.0.0.0 are all considered equal.
https://learn-microsoft-com.translate.goog/en-us/nuget/concepts/package-versioning?tabs=semver20sort&_x_tr_sl=auto&_x_tr_tl=en&_x_tr_hl=zh-CN&_x_tr_pto=wapp#normalized-version-numbers

OK, so the console output is incorrect. What if the user enters something like "1", do they get "1.0.0"?

Yes, according to the above NuGet version processing rules, the user input 1 should be regarded as 1.0.0. vcpkg also needs to follow this rule.

I guess, what I want to see before we make a change like this is an understanding of where the extra .0 came from. Did nuget.exe do that? Did we?

According to the Semantic Versioning specification (SemVer), the standard version number format is X.Y.Z, where X, Y, and Z are non-negative integers, and leading zeros are prohibited. When some parts of the version number are omitted, the missing parts are usually assumed to be zero. For example, 1.2 can be considered 1.2.0, and 1 can be considered 1.0.0. This default completion logic is adopted in many package management tools, including NuGet.

image

https://semver.org/#spec-item-2

@FrankXie05
Copy link
Contributor Author

To be clear, is nuget doing that, or is somewhere else in vcpkg doing that?

This behavior is the norm for Nuget, and vcpkg needs to stay consistent.

For some other norms of Nuget, such as pre-release version identification(like: -alpha, -beta, -rc), we will not accept such version updates at present.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
requires:discussion This PR requires discussion of the correct way forward
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants