Skip to content

Don't rely on parameter order when creating VersionRange - #5213

Merged
JohnMcPMS merged 5 commits into
microsoft:masterfrom
Trenly:VersionRange
Feb 19, 2025
Merged

Don't rely on parameter order when creating VersionRange#5213
JohnMcPMS merged 5 commits into
microsoft:masterfrom
Trenly:VersionRange

Conversation

@Trenly

@Trenly Kaleb Luedtke (Trenly) commented Feb 16, 2025

Copy link
Copy Markdown
Contributor

I don't know whether or not this will resolve the above issue, but I also don't think it can hurt?

Instead of relying on the parameters to be in the correct order for minimum and maximum when creating a VersionRange, the code can instead just put them in the right order.


Microsoft Reviewers: Open in CodeFlow

@Trenly
Kaleb Luedtke (Trenly) requested a review from a team as a code owner February 16, 2025 20:58
@JohnMcPMS

Copy link
Copy Markdown
Member

The thing I worry about with this is code that would have failed previously will now get through and may continue on with a flawed view of the range.

@Trenly

Copy link
Copy Markdown
Contributor Author

The thing I worry about with this is code that would have failed previously will now get through and may continue on with a flawed view of the range.

That was my concern as well, but I wondered if it truly mattered? Consumers of the range should be checking if a version falls within the range, or using other methods of the VersionRange class. So, in theory, as long as the class maps everything properly, it should be transparent to the consumer.

However, theory really only goes so far, so conpletely understand if you'd prefer to close this

I'd be interested to know how the values in the index were flip-flopped between the max and min

@JohnMcPMS

Copy link
Copy Markdown
Member

I'd be interested to know how the values in the index were flip-flopped between the max and min

That is the real root issue (other than our continued discussions on the whether version ranges make any sense at all 😄).

@yao-msft

yao-msft commented Feb 19, 2025

Copy link
Copy Markdown
Contributor

I'd be interested to know how the values in the index were flip-flopped between the max and min

That is the real root issue (other than our continued discussions on the whether version ranges make any sense at all 😄).

That's probably due to this change #5001 where the version comparison was fixed to correctly compare versions. It's likely the index was created with an old wingetutil and the new client has fixed code. I think a rebuild of the index (with latest wingetutil) will fix the index.

@Trenly

Copy link
Copy Markdown
Contributor Author

I'd be interested to know how the values in the index were flip-flopped between the max and min

That is the real root issue (other than our continued discussions on the whether version ranges make any sense at all 😄).

That's probably due to this change #5001 where the version comparison was fixed to correctly compare versions. It's likely the index was created with an old wingetutil and the new client has fixed code. I think a rebuild of the index (with latest wingetutil) will fix the index.

Ah, that makes sense; The old code would have parsed it as 6.1 instead of 6.1.0, and in that case would have returned the incorrect version comparison

@JohnMcPMS

Copy link
Copy Markdown
Member

Given that this is the root cause, I'm inclined to say that the best resolution likely is to accept a change along these lines. If we do rebuild, or in fact if a similar situation arises after this change makes it to the service, then <1.10 clients will fail similarly. The good thing is that while I am concerned about the potential for some code to make a mistake that is harder to diagnose after this change, we can simply review current usage to see if that is happening.

@JohnMcPMS

Copy link
Copy Markdown
Member

On review, I didn't see any concerning usage.

Also, GH copilot will write the commit message for you when you edit here. Fun.

@JohnMcPMS

Copy link
Copy Markdown
Member

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@JohnMcPMS
JohnMcPMS merged commit af81f1d into microsoft:master Feb 19, 2025
@Trenly
Kaleb Luedtke (Trenly) deleted the VersionRange branch February 19, 2025 21:32
JohnMcPMS pushed a commit to JohnMcPMS/winget-cli that referenced this pull request Feb 19, 2025
)

Instead of relying on the parameters to be in the correct order for
minimum and maximum when creating a VersionRange, the code can instead
just put them in the right order.
JohnMcPMS added a commit that referenced this pull request Feb 19, 2025
Instead of relying on the parameters to be in the correct order for
minimum and maximum when creating a VersionRange, the code can instead
just put them in the right order.

CP from #5213
Kaleb Luedtke (Trenly) added a commit to Trenly/winget-cli that referenced this pull request Feb 19, 2025
)

Instead of relying on the parameters to be in the correct order for
minimum and maximum when creating a VersionRange, the code can instead
just put them in the right order.
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.

3 participants