Add lenient mode to Version string parsing that doesn't require patch version #212

colincornaby · 2021-05-04T22:30:29Z

Git repos are commonly leaving off the patch segments of version strings in tags. This practice is also commonly repeated in guides on Git tagging. This commit makes the patch version optional to improve compatibility with these Git repositories in SPM.

Because of the possible downstream effects, I've made the lenient version parsing optional, and it defaults to off. Right now it only allows the patch version to be left off.

I considered trying to keep changes entirely in SPM without touching the Version type. This seemed like the most maintainable and performant option.

WowbaggersLiquidLunch · 2021-05-04T23:38:04Z

Sources/TSCUtility/Version.swift

@@ -119,11 +120,15 @@ public extension Version {
            .split(separator: ".", maxSplits: 2, omittingEmptySubsequences: false)
            .map(String.init).compactMap({ Int($0) }).filter({ $0 >= 0 })

-        guard requiredComponents.count == 3 else { return nil }
+        guard requiredComponents.count == 3 || (requiredComponents.count == 2 && lenient) else { return nil }


If we're doing this, would it be better (or make sense) if the rules are made even more lenient to allow for just a single major version number?

i.e.

guard requiredComponents.count == 3 || lenient else ...

My original intention was to make repositories that mix two and three component version styles more compatible. Supporting a single version component would make SPM support repositories that don't technically version semantically, which is a larger change. But that behavior can still map to semantic versions, and it would maximize compatibility with existing repos.

I tend to agree that from a conversion point of, tags are either 3 x.0.0 or 2 x.0 segment long. iow a single digit version tag would be pretty exceptional.

tomerd · 2021-07-26T18:22:29Z

@colincornaby thanks for this proposal. do you think package manifest should also allow x.0 to be parsed as x.0.0? If so, we would also need a change in SwiftPM itself: https://github.com/apple/swift-package-manager/blob/main/Sources/PackageDescription/Version.swift

Tests/TSCUtilityTests/VersionTests.swift

colincornaby · 2021-07-27T23:00:20Z

@tomerd - I had noticed that there is a package manifest facing aspect to this. I'll be honest, this is my first pull request to Swift, so I was unsure how large of changes the project is interested in through pull request. I also didn't know what existing opinions project opinions were on this.

Short answer is: No, but I'd be willing to make those changes if that's desired by others. My motivation was that we had started to implement SPM manifests in some of our C++ projects. But those teams hadn't always been adding the patch version where it was 0 to their tags (e.g. they used 3.4, 3.4.1, 3.4.2, 3.5). I wanted to be able to introduce SPM manifests into repos without retraining teams. A quick survey found a lot of Git repos also dropping the bug fix version when it was 0.

I do also have changes to the SPM repo that would integrate these changes into Git tagging, but they're waiting on these changes. If it's desired for the manifest to also support two digits I can propose those changes in a pull request as well.

tomerd · 2021-07-27T23:04:30Z

thanks @colincornaby makes sense. I think the proposed change is valuable at the git tag parsing level. @neonichu wdyt?

tomerd · 2021-07-27T23:04:39Z

@swift-ci please test

tomerd · 2021-07-28T16:48:10Z

@colincornaby CI is failing on something that looks like an issue with the tests?

/home/buildnode/jenkins/workspace/pr-swift-tools-support-core-linux/branch-main/swiftpm/Sources/SPMTestSupport/MockPackageContainer.swift:95:63: error: type 'Version' has no member 'init(string:)'
17:14:37         self._versions = dependencies.keys.compactMap(Version.init(string:)).sorted()
17:14:37                                                       ~~~~~~~ ^~~~~~~~~~~~~
17:14:37 /home/buildnode/jenkins/workspace/pr-swift-tools-support-core-linux/branch-main/swiftpm/Sources/SPMTestSupport/MockWorkspace.swift:123:25: error: generic parameter 'U' could not be inferred
17:14:37                 let v = version.flatMap(Version.init(string:))
17:14:37                         ^
17:14:37 Swift.Optional:6:28: note: in call to function 'flatMap'
17:14:37     @inlinable public func flatMap<U>(_ transform: (Wrapped) throws -> U?) rethrows -> U?
17:14:37                            ^
17:14:37 /home/buildnode/jenkins/workspace/pr-swift-tools-support-core-linux/branch-main/swiftpm/Sources/SPMTestSupport/MockWorkspace.swift:123:49: error: type 'Version' has no member 'init(string:)'
17:14:37                 let v = version.flatMap(Version.init(string:))
17:14:37                                         ~~~~~~~ ^~~~~~~~~~~~~

tomerd · 2021-07-28T16:50:28Z

Sources/TSCUtility/Version.swift

@@ -109,7 +109,8 @@ public extension Version {
    ///
    /// - Parameters:
    ///   - string: The string to parse.
-    init?(string: String) {
+    ///   - lenient: Boolean indicating if partial strings should be parsed. Infers that strings without an explicit patch version have a patch version of zero. Defaults to false.   
+    init?(string: String, lenient: Bool = false) {


since there are some dependencies in SwiftPM on the concrete ctor signature, lets structure this as:

init?(string: String) { self.init(string: string, lenient: false) } init?(string: String, lenient: Bool) { ... }

Do we really need the the lenient parameter? If the goal is to treat all x.y as x.y.0, then I don't think we can use strict parsing anywhere.

I'm happy to remove the lenient option if no one needs it as a solution.

imo its good to have this option for now so we can enable/disable it from SwiftPM side based on which tools version we are targeting - as this change could break existing packages. @neonichu wdyt?

I agree that the flag is useful.

imo its good to have this option for now so we can enable/disable it from SwiftPM side based on which tools version we are targeting - as this change could break existing packages.

This makes sense. I keep forgetting that SwiftPM needs to support previous versions.

Thanks for the feedback and pointing me to exactly what the test failure was. I've made the changes and run the in-module tests locally.

I keep forgetting that SwiftPM needs to support previous versions.

that's the hard part :D

tomerd · 2021-07-28T22:26:16Z

@swift-ci please test

colincornaby · 2021-07-29T03:43:14Z

Looks like the Linux build failed, but it's a module cache issue.

<unknown>:0: error: module file '/home/buildnode/jenkins/workspace/pr-swift-tools-support-core-linux/branch-main/build/buildbot_incremental/swiftpm-linux-x86_64/x86_64-unknown-linux-gnu/swift-driver/module-cache/2YIFRU0PHJR5U/CDispatch-SLYT5KF7B5CN.pcm' is out of date and needs to be rebuilt: signature mismatch

neonichu · 2021-07-29T16:38:29Z

@swift-ci please test

WowbaggersLiquidLunch · 2021-07-29T22:12:23Z

Sorry for asking this, but I wonder if it's OK to merge #214 first (after it's ready) and then rebase this PR on it?

These 2 PRs conflict with each other, so one of these has to be rebased after the other is merged. It seems that it's much easier to rebase this PR on #214 than the vice versa, because #214 changes a lot more lines and the logic. And since most of the parsing logic is changed in #214, if this PR is merged first, everything here would have to be reimplemented again in #214. If we end up deciding to merge #214 first, I can help with rebasing this PR.

tomerd · 2021-07-29T23:56:35Z

Sorry for asking this, but I wonder if it's OK to merge #214 first (after it's ready) and then rebase this PR on it?

These 2 PRs conflict with each other, so one of these has to be rebased after the other is merged. It seems that it's much easier to rebase this PR on #214 than the vice versa, because #214 changes a lot more lines and the logic. And since most of the parsing logic is changed in #214, if this PR is merged first, everything here would have to be reimplemented again in #214. If we end up deciding to merge #214 first, I can help with rebasing this PR

sgtm, assuming we can get #214 over the finish line soon

abertelrud · 2021-08-02T21:48:13Z

Sorry for asking this, but I wonder if it's OK to merge #214 first (after it's ready) and then rebase this PR on it?

These 2 PRs conflict with each other, so one of these has to be rebased after the other is merged. It seems that it's much easier to rebase this PR on #214 than the vice versa, because #214 changes a lot more lines and the logic. And since most of the parsing logic is changed in #214, if this PR is merged first, everything here would have to be reimplemented again in #214. If we end up deciding to merge #214 first, I can help with rebasing this PR

sgtm, assuming we can get #214 over the finish line soon

I think #214 looks good to me at this point, so that one can be merged if @neonichu agrees.

colincornaby · 2021-08-03T17:43:38Z

Following along - I'm fine also doing any cleanup work necessary after #214 gets merged in, or assisting others in that. I have changes in SPM waiting on this pull request to be merged, but no huge rush there.

tomerd · 2021-08-04T18:23:19Z

@colincornaby #214 is now merged, please rebase. thanks for your patience on this

WowbaggersLiquidLunch · 2021-08-04T18:34:34Z

@colincornaby Can I have access to this branch, to help with the rebase?

colincornaby · 2021-08-04T18:39:47Z

@WowbaggersLiquidLunch - I sent a collaboration invite over for my fork. I had started on the rebase, but in since you know the changes better I'll let you make the changes!

Added the `usesLenientParsing` parameter to `Version.init(versionString:) throws` and logic changes to enable parsing version strings without patch versions. Added the `usesLenientParsing: Bool` associated type to `VersionError.invalidVersionCoreIdentifiersCount`, so the error message can be specialised for both strict and lenient parsing.

WowbaggersLiquidLunch · 2021-08-05T02:05:07Z

Just force-pushed 19d1c37.

I took the liberty of squashing the 3 previous commits on this branch, so that it was easier to rebase. Hopefully this is ok. I also saved a copy of the pre-rebase branch here, just in case.

Changes as part of the rebase:

Instead of a new Version.init?(string:lenient), I added the usesLenientParsing parameter to Version.init(versionString:) throws, because #214 moved all the parsing logic to the latter initializer and deprecated the former. I changed the parameter name to follow the API guidelines more closely.

usesLenientParsing is also added to VersionError.invalidVersionCoreIdentifiersCount as an additional associated value, because the error message needs to know whether the minimum number of version core identifiers is 2 or 3.

Most of the additional lines of code are test cases.

tomerd · 2021-08-05T17:50:14Z

Sources/TSCUtility/Version.swift

    /// - Throws: A `VersionError` instance if the `versionString` doesn't follow [SemVer 2.0.0](https://semver.org).
-    public init(versionString: String) throws {
+    public init(versionString: String, usesLenientParsing: Bool = false) throws {


iirc we needed two constructors instead of one with default value because of downstream dependencies on the exact signatures (passed into map{})

I think the one that downstream dependencies use is init?(string: String). This initializer still exists (although marked as deprecated) and calls init?(_ versionString: String), which then calls init(versionString: versionString, usesLenientParsing: false) throws.

I have the change ready to commit, but also had the thought the downstream dependencies probably use the deprecated function. I could see the use in having a new version ready to cut over to, but that might get into project planning that I don't know the scope of.

thanks @colincornaby and @WowbaggersLiquidLunch - lets get this merged first, then @colincornaby you can submit a PR to SwiftPM to use the new methods where appropriate.

Sounds good! Thanks!

tomerd · 2021-08-05T18:06:04Z

@swift-ci please test

tomerd · 2021-08-05T20:43:45Z

thanks again thanks @colincornaby and @WowbaggersLiquidLunch

colincornaby requested review from abertelrud, aciidgh, friedbunny, neonichu and tomerd as code owners May 4, 2021 22:30

WowbaggersLiquidLunch reviewed May 4, 2021

View reviewed changes

tomerd assigned neonichu May 10, 2021

WowbaggersLiquidLunch mentioned this pull request May 13, 2021

[TSCUtility] Correct semantic version parsing and comparison #214

Merged

tomerd reviewed Jul 26, 2021

View reviewed changes

Tests/TSCUtilityTests/VersionTests.swift Outdated Show resolved Hide resolved

tomerd reviewed Jul 28, 2021

View reviewed changes

tomerd approved these changes Jul 29, 2021

View reviewed changes

WowbaggersLiquidLunch force-pushed the lenientVersionStringParsing branch from 28c2995 to 19d1c37 Compare August 5, 2021 02:04

tomerd reviewed Aug 5, 2021

View reviewed changes

tomerd merged commit 3cabb66 into swiftlang:main Aug 5, 2021

colincornaby mentioned this pull request Aug 5, 2021

Enable lenient parsing of Git version tags swiftlang/swift-package-manager#3649

Merged

Add lenient mode to Version string parsing that doesn't require patch version #212

Add lenient mode to Version string parsing that doesn't require patch version #212

Uh oh!

Conversation

colincornaby commented May 4, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tomerd commented Jul 26, 2021

Uh oh!

Uh oh!

colincornaby commented Jul 27, 2021

Uh oh!

tomerd commented Jul 27, 2021

Uh oh!

tomerd commented Jul 27, 2021

Uh oh!

tomerd commented Jul 28, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

WowbaggersLiquidLunch Jul 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tomerd commented Jul 28, 2021

Uh oh!

colincornaby commented Jul 29, 2021

Uh oh!

neonichu commented Jul 29, 2021

Uh oh!

WowbaggersLiquidLunch commented Jul 29, 2021

Uh oh!

tomerd commented Jul 29, 2021

Uh oh!

abertelrud commented Aug 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

colincornaby commented Aug 3, 2021

Uh oh!

tomerd commented Aug 4, 2021

Uh oh!

WowbaggersLiquidLunch commented Aug 4, 2021

Uh oh!

colincornaby commented Aug 4, 2021

Uh oh!

WowbaggersLiquidLunch commented Aug 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

WowbaggersLiquidLunch Aug 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tomerd commented Aug 5, 2021

Uh oh!

tomerd commented Aug 5, 2021

WowbaggersLiquidLunch Jul 28, 2021 •

edited

Loading

abertelrud commented Aug 2, 2021 •

edited

Loading

WowbaggersLiquidLunch commented Aug 5, 2021 •

edited

Loading

WowbaggersLiquidLunch Aug 5, 2021 •

edited

Loading