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

refactor(versions): Refactor 'versions.ps1' #3721

Merged
merged 23 commits into from
Nov 13, 2021

Conversation

niheaven
Copy link
Member

@niheaven niheaven commented Oct 29, 2019

Reworking of #3333, #3578, #3648

Fix #2720, fix #3687, fix #4236, fix #4244, fix #4306

Get correct current version from apps\xxx\current\manifest.json, so works well after scoop reset or version roll-back. If NO_JUNCTIONS is set, use creating time of install.json to find last installed version as current version.

Add force-update config option that forces to update to manifest version (for version roll-back).

Now use '+' as post-release delimiter, and manifest maintainer should trim metadata in 'checkver.regex'. #3613

@chawyehsu
Copy link
Member

chawyehsu commented Oct 30, 2019

'+' is useful in so many cases (not only Flutter, but also JDK). I appreciate this work.

@niheaven
Copy link
Member Author

niheaven commented Oct 31, 2019

I've noticed many different usages of '+' sign outside semver's metadata, maybe a better sollution is deprecating the use of '+' in version, and trim metadata in checkver and replace '+' with '-' in other situations.

If parse versions with an uniform pattern, it's difficult to distinguish between metadata and build number.

The simplest way is trim metadata in checkver and treat '+' as '-'.

@Ash258
Copy link
Contributor

Ash258 commented Oct 31, 2019

You will confuse user when you manipulate with version name.

@niheaven
Copy link
Member Author

I mean, trimming metadata in checkver.regex by manifest maintainer, and treating '+' just as '-' in Compare-Version if '+' means post-release. This is more clear, for both maintainers and users, right?

@niheaven
Copy link
Member Author

niheaven commented Nov 1, 2019

KNOWN ISSUE: If app has only broken installed version dir (i.e. no vvvv\install.json), Get-InstalledVersion throw error. WILL FIX SOON.

@chawyehsu
Copy link
Member

chawyehsu commented Nov 4, 2019

I mean, trimming metadata in checkver.regex by manifest maintainer, and treating '+' just as '-' in Compare-Version if '+' means post-release. This is more clear, for both maintainers and users, right?

I guess it's ok to manipulate the version inside the Compare-Version function for better version comparison implementation (sounds like "mapping any version number to a SemVer version number"). But the version in the manifest should not be modified as it would confuse end-users. Better to keep the version in the manifest in its original format.

@niheaven
Copy link
Member Author

niheaven commented Nov 5, 2019

Due to https://semver.org/#spec-item-10

Build metadata MUST be ignored when determining version precedence. Thus two versions that differ only in the build metadata, have the same precedence.

So trimming metadata in checkver.regex is much better, user needn't update from 1.0+123 to 1.0+456, so they should both have 1.0 in version.

But if '+' means post-release likes Flutter, version 1.9.1+hotfix.5 should be updated to 1.9.1+hotfix.6, and we could have 1.9.1+hotfix.6 in version.

@chawyehsu
Copy link
Member

But if '+' means post-release likes Flutter, version 1.9.1+hotfix.5 should be updated to 1.9.1+hotfix.6, and we could have 1.9.1+hotfix.6 in version.

How about jdk-13.0.1+9.1_openj9-0.17.0, jdk-12.0.2+10?

@niheaven
Copy link
Member Author

niheaven commented Nov 8, 2019

Of cource jdk-13.0.1+9.1_openj9-0.17.0 > jdk-12.0.2+10. BTW, _openj9-0.17.0 should be omitted in version, I thought.

@chawyehsu
Copy link
Member

chawyehsu commented Nov 8, 2019

Of cource jdk-13.0.1+9.1_openj9-0.17.0 > jdk-12.0.2+10. BTW, _openj9-0.17.0 should be omitted in version, I thought.

I meant, should the +9.1 being trimmed, leaving the 13.0.1 in version?
https://github.com/lukesampson/scoop/pull/3721/files#diff-3bca6d668143a35c773f42dfd1c707d8R64-R75
Users will not get any update builds of JDK 13.0.1 if metadata got trimmed.

13.0.1+9.1 vs 13.0.1.

@niheaven
Copy link
Member Author

niheaven commented Nov 8, 2019

So I just want to discuss if we need trimming metadata in Compare-Version. I suggest trimming them in checkver.regex and leaving post-release tag in version (e.g. 13.0.1+9.1).

If above is preferred, I'll modify the function to adopt it.

@chawyehsu
Copy link
Member

chawyehsu commented Nov 14, 2019

So I just want to discuss if we need trimming metadata in Compare-Version. I suggest trimming them in checkver.regex and leaving post-release tag in version (e.g. 13.0.1+9.1).

If above is preferred, I'll modify the function to adopt it.

In those use cases that I know (all variant of JDK, and Flutter), all '+' are treated as post-release. No manifest I know is treating '+' as metadata, though it's a SemVer standard. Therefore I prefer leaving '+' as post-release tag in version. If one application is using '+' as metadata, manifest maintainers can trim it in checkver.regex by themselves.

But this should be documented in the wiki.

@niheaven
Copy link
Member Author

Now use '+' as post-release delimiter, and manifest maintainer should trim metadata in 'checkver.regex'. After merging, I'll update wiki.

@chawyehsu
Copy link
Member

And #3613 should be integrated into this pr.

lib/versions.ps1 Show resolved Hide resolved
lib/versions.ps1 Show resolved Hide resolved
lib/versions.ps1 Show resolved Hide resolved
test/Scoop-Versions.Tests.ps1 Outdated Show resolved Hide resolved
lib/versions.ps1 Show resolved Hide resolved
lib/versions.ps1 Show resolved Hide resolved
lib/versions.ps1 Show resolved Hide resolved
lib/versions.ps1 Outdated Show resolved Hide resolved
lib/versions.ps1 Outdated Show resolved Hide resolved
lib/versions.ps1 Outdated Show resolved Hide resolved
lib/versions.ps1 Outdated Show resolved Hide resolved
@chawyehsu
Copy link
Member

The pull request has conflicts now. @niheaven

@chawyehsu
Copy link
Member

So can this pull request be merged into develop branch?

@niheaven
Copy link
Member Author

This could be. @r15ch13 @Ash258

@niheaven
Copy link
Member Author

Ping @r15ch13

@Ash258
Copy link
Contributor

Ash258 commented May 17, 2020

Listing of nightly version manifest will be broken as you always return (installed_manifest ...).version which is exactly nightly and breaking stuff like versiondir, ....

Potential fix: Ash258@ddf8304

This fix will prevent scoop reset go to work as latest version reset as it will return always the link target. But it is not a major issue as you can use force update or just list all versions and then use the latest one manually

This PR presents breaking change.

Example scenario:

  1. Have multiple versions of application installed (go for example)
    • 1.14.3 is latest one available
  2. scoop reset go@1.14.2 -> everything OK
  3. scoop reset go@1.14.3 -> everything OK
  4. scoop reset go@1.14.2 -> everything OK
  5. scoop reset go
    • This PR will reset to 1.14.2
    • Scoop without this PR will reset to 1.14.3

niheaven and others added 2 commits November 10, 2021 15:40
Co-Authored-By: Jakub Čábera <cabera.jakub@gmail.com>
@niheaven niheaven changed the title fix(versions): Refactor 'versions.ps1' refactor(versions): Refactor 'versions.ps1' Nov 13, 2021
@niheaven niheaven merged commit 77d00d1 into ScoopInstaller:develop Nov 13, 2021
@niheaven niheaven deleted the fix-version-compare branch November 13, 2021 16:45
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.

4 participants