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

Error comparing versions #4998

Closed
Trenly opened this issue Nov 22, 2024 · 2 comments · Fixed by #5001
Closed

Error comparing versions #4998

Trenly opened this issue Nov 22, 2024 · 2 comments · Fixed by #5001
Labels
In-PR Issue related to a PR Issue-Bug It either shouldn't be doing this or needs an investigation.
Milestone

Comments

@Trenly
Copy link
Contributor

Trenly commented Nov 22, 2024

Brief description of your issue

See the discussion -

TL;DR

    RequireLessThan("1-rc", "1"); // Test case pases
    RequireLessThan("1.2-rc", "1.2"); // Test case passes
    RequireLessThan("1.0-rc", "1.0"); // Test case fails
@microsoft-github-policy-service microsoft-github-policy-service bot added the Needs-Triage Issue need to be triaged label Nov 22, 2024
@Trenly
Copy link
Contributor Author

Trenly commented Nov 22, 2024

[Policy] Issue Bug

@microsoft-github-policy-service microsoft-github-policy-service bot added Issue-Bug It either shouldn't be doing this or needs an investigation. and removed Needs-Triage Issue need to be triaged labels Nov 22, 2024
Trenly added a commit to Trenly/winget-cli that referenced this issue Nov 22, 2024
@Trenly
Copy link
Contributor Author

Trenly commented Nov 22, 2024

I've linked my branch that I've used for testing. The only test that is currently failing is the one mentioned in the initial comment.

Test results
PS E:\winget-cli\src\x64\Debug\AppInstallerCLITests> .\AppInstallerCLITests.exe version*
Filters: version*

~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
AppInstallerCLITests.exe is a Catch v2.11.0 host application.
Run with -? for options

-------------------------------------------------------------------------------
VersionCompare
-------------------------------------------------------------------------------
E:\winget-cli\src\AppInstallerCLITests\Versions.cpp(187)
...............................................................................

E:\winget-cli\src\AppInstallerCLITests\Versions.cpp(145): FAILED:
  REQUIRE( vA < vB )
with expansion:
  {?} < {?}

===============================================================================
test cases:  13 |  12 passed | 1 failed
assertions: 271 | 270 passed | 1 failed

Edit:

When a version is created, all trailing zeroes are trimmed off. This means that 22.0.0, when broken into parts, is actually stored as [22] and not as [22, 0, 0]. This means that it's hitting the condition where i >= other.m_parts.size() and is just breaking the for loop instead of actually trying to pad with zeroes when comparing

Edit: Confirmed by removing the trimming, which caused the sorting to be correct. This isn’t a real fix though. . .

@microsoft-github-policy-service microsoft-github-policy-service bot added the In-PR Issue related to a PR label Nov 22, 2024
Trenly added a commit to Trenly/winget-cli that referenced this issue Nov 25, 2024
<!-- To check a checkbox place an "x" between the brackets. e.g: [x] -->

- [x] I have signed the [Contributor License
Agreement](https://cla.opensource.microsoft.com/microsoft/winget-pkgs).
- [x] This pull request is related to an issue.
  - Resolves microsoft#4998 

When versions are created, any empty parts are trimmed off. This means
that `22.0.0` is stored as `[22]` in memory. This causes an issue when
comparing to a version that had additional parts, but should be sorted
lower than the version which is trimmed, such as `22.0.0-rc`. This is
due to the comparator seeing that `[22]` has no more parts to compare
after the first iteration, while `[22, 0, 0-rc]` does, causing it to hit
the condition where whichever version has more parts is considered to be
greater.

This PR updates the logic so that the version comparison effectively
contains as many empty parts as are needed to fully complete the
comparison. This retains the logic that if a part is not equal to the
part it is being compared against, the result of that comparison will be
returned but eliminates the need for checking if the versions have the
same number of parts since, effectively, they do; it's just that the
extra parts needed to complete the comparison are all empty.

Additional tests have been added to cover this corner case.

If possible, I would ask that this also be cherry-picked into any future
1.9 releases (assuming this PR passes review and merges)
###### Microsoft Reviewers: [Open in
CodeFlow](https://microsoft.github.io/open-pr/?codeflow=https://github.com/microsoft/winget-cli/pull/5001)
Trenly added a commit to Trenly/winget-cli that referenced this issue Nov 25, 2024
<!-- To check a checkbox place an "x" between the brackets. e.g: [x] -->

- [x] I have signed the [Contributor License
Agreement](https://cla.opensource.microsoft.com/microsoft/winget-pkgs).
- [x] This pull request is related to an issue.
  - Resolves microsoft#4998 

When versions are created, any empty parts are trimmed off. This means
that `22.0.0` is stored as `[22]` in memory. This causes an issue when
comparing to a version that had additional parts, but should be sorted
lower than the version which is trimmed, such as `22.0.0-rc`. This is
due to the comparator seeing that `[22]` has no more parts to compare
after the first iteration, while `[22, 0, 0-rc]` does, causing it to hit
the condition where whichever version has more parts is considered to be
greater.

This PR updates the logic so that the version comparison effectively
contains as many empty parts as are needed to fully complete the
comparison. This retains the logic that if a part is not equal to the
part it is being compared against, the result of that comparison will be
returned but eliminates the need for checking if the versions have the
same number of parts since, effectively, they do; it's just that the
extra parts needed to complete the comparison are all empty.

Additional tests have been added to cover this corner case.

If possible, I would ask that this also be cherry-picked into any future
1.9 releases (assuming this PR passes review and merges)
###### Microsoft Reviewers: [Open in
CodeFlow](https://microsoft.github.io/open-pr/?codeflow=https://github.com/microsoft/winget-cli/pull/5001)
@denelon denelon added this to the 1.10 Client milestone Dec 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
In-PR Issue related to a PR Issue-Bug It either shouldn't be doing this or needs an investigation.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants