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

Extended SemVer v2 compliance and reliability improvements #3030

Merged
merged 15 commits into from Feb 16, 2018

Conversation

Projects
None yet
5 participants
@viktor-svub
Contributor

viktor-svub commented Feb 7, 2018

  • updated Dependencies parser to accept semver2 twiddle-wakka constraints -- versions like 1.2.3-beta.4 ended up with invalid upper constraint (>= 1.2.3-beta.4 < 1.2.3-beta.0 (beta))
  • changed version comparison to be more obvious and to follow semver2 rules more closely
  • changed extended SemVer Build to bigint, as string builds resulted in "unique" versions, not properly resolvable or comparable to anything else
  • extended SemVer parser to accept well-formed semver2 strings without misinterpreting embedded numbers in alphanumeric segments -- which is now special-cased for for semver1 support
  • updated SemVer parser to interpret malformed versions as close as possible to correct ones -- until now, they were accepted as-is without warnings or errors, but could not be properly compared, e.g. version like 1.2.3.beta.4 was accepted on all paths, but considered neither prerelease, nor part of 1.2.3 range
  • changed transitive prerelease versions of prerelease packages to use Concrete name where possible -- with "diamond dependencies" in graph, having transitive dependencies on All caused resolver loops, and broke the expectation of using only the named prerelease, unless prerelease was specified explicitly
  • added more prerelease and its transitivity information to conflict report
  • added tests and new cases to cover previously undefined edge cases

viktor-svub added some commits Jan 22, 2018

let penultimateItem = Math.Max(parts.Length - 2, 0)
let promoted = parts |> promote penultimateItem
String.Join(".", promoted)
let twiddle (minimum:SemVerInfo) =

This comment has been minimized.

@viktor-svub

viktor-svub Feb 7, 2018

Contributor

changed to utilize existing SemVer parser, to handle multi segment prerelease ranges correctly
original code changed the last numeric segment and ended up with invalid upper constraint

| [] ->
versions
|> List.collect (function { PreRelease = Some x } -> [x.Name] | _ -> [])
|> List.distinct
|> function [] -> PreReleaseStatus.No | xs -> PreReleaseStatus.Concrete xs
| [x] when String.equalsIgnoreCase x "prerelease" -> PreReleaseStatus.All
| _ -> PreReleaseStatus.Concrete texts
| _ -> PreReleaseStatus.Concrete items

This comment has been minimized.

@viktor-svub

viktor-svub Feb 7, 2018

Contributor

assuming the filter for empty strings is necessary, we should use the filtered result

errorReport.AddLine (sprintf " Conflict detected:")
let getConflictMessage req =
let vr = formatVR req.VersionRequirement
let pr = formatPR hasPrereleases req.VersionRequirement
let pr = formatPR true req.VersionRequirement
let tp = if req.TransitivePrereleases then "*" else ""

This comment has been minimized.

@viktor-svub

viktor-svub Feb 7, 2018

Contributor

prerelease transitivity hint

let getName fromList =
match fromList with
| AlphaNumeric(a)::_ -> a
| _::AlphaNumeric(a)::_ -> a // fallback to 2nd

This comment has been minimized.

@viktor-svub

viktor-svub Feb 7, 2018

Contributor

fallback to 2nd place, for cases where users want to move from non-compliant versions like 1.2.3.4-beta1 to valid semver2 like 1.2.3-4.beta.1 to keep similar precedence rules

let notEmpty = StringSplitOptions.RemoveEmptyEntries
let name, values =
match str.Split([|'.'|],notEmpty) with

This comment has been minimized.

@viktor-svub

viktor-svub Feb 7, 2018

Contributor

heuristics: versions with dots in prerelease are semver2, which allows hyphen as part of identifiers;
only when having no dots we try to interpret the embedded numbers and separate "prerelease name" via regex

override x.ToString() = x.Origin
override x.GetHashCode() = hash x.Origin
member x.CompareTo(yobj) =

This comment has been minimized.

@viktor-svub

viktor-svub Feb 7, 2018

Contributor

changed to tail-recursive list comparison as zipOpt was slower, not debuggable, and keeping semver2 compliance was nontransparent

This comment has been minimized.

@baronfel

baronfel Feb 7, 2018

Contributor

Nice change, can we remove zipOpt now? I should never have written that thing.

@@ -96,19 +132,18 @@ type SemVerInfo =
/// The optional PreRelease version
PreRelease : PreRelease option
/// The optional build no.
Build : string
Build : bigint

This comment has been minimized.

@viktor-svub

viktor-svub Feb 7, 2018

Contributor

there was no case in unit or integration tests actually using string builds; though they could have been created by accident (we actually did)

This comment has been minimized.

@baronfel

baronfel Feb 7, 2018

Contributor

I think the compelling case for string here instead of bigint is the case of embedding commit hashes as the build metadata. The spec (section 10) says that build metadata can have the following form:

...Identifiers MUST comprise only ASCII alphanumerics and hyphen [0-9A-Za-z-]...

and so I think we must allow string here.

This comment has been minimized.

@viktor-svub

viktor-svub Feb 7, 2018

Contributor

As far as I understand it, build metadata is separate field, a semver compliant one, placed after a + sign, while the Build field is the 4th optional, non-semver, but NuGet supported, numeric segment, separated by dot, and having no meaningful comparison semantics for strings.

This comment has been minimized.

@baronfel

baronfel Feb 7, 2018

Contributor

ah, of course you are correct

@@ -125,40 +160,83 @@ type SemVerInfo =
member x.AsString
with get() = x.ToString()
member x.Equals(y) =
x.Major = y.Major && x.Minor = y.Minor && x.Patch = y.Patch && x.Build = y.Build && x.PreRelease = y.PreRelease

This comment has been minimized.

@viktor-svub

viktor-svub Feb 7, 2018

Contributor

comparing BuildMetaData is breaking semver2 compliance, and was not used anywhere

This comment has been minimized.

@baronfel

baronfel Feb 7, 2018

Contributor

good catch, that never should have been there

override x.GetHashCode() = hash (x.Minor, x.Minor, x.Patch, x.Build, x.PreRelease)
member x.CompareTo(y) =
let comparison =

This comment has been minimized.

@viktor-svub

viktor-svub Feb 7, 2018

Contributor

comparing each part only once and using the result is more transparent (and faster)

/// matches over list of the version fragments *and* delimiters
let major, minor, patch, revision, suffix =
match fragments with
| (Int M)::"."::(Int m)::"."::(Int p)::"."::(Big b)::tail -> M, m, p, b, tail

This comment has been minimized.

@viktor-svub

viktor-svub Feb 7, 2018

Contributor

match including delimiters allows to parse as much of valid version parts as possible, and processing the rest as prerelease segments

| GreaterThan v1, GreaterThan v2 when v1 < v2 -> true
| GreaterThan v1, Specific v2 when v1 < v2 -> true
| GreaterThan v1, Range(_, min2, max2, _) when v1 < min2 && v1 < max2 -> true
| Range(lower, min1, max1, upper), Specific v2 ->

This comment has been minimized.

@viktor-svub

viktor-svub Feb 7, 2018

Contributor

covering more cases allows faster resolver passes, and may prevent looping in some cases

| _ -> false
member this.GetPreReleaseStatus =

This comment has been minimized.

@viktor-svub

viktor-svub Feb 7, 2018

Contributor

unified prerelease status from the constraints' parts

@@ -161,20 +228,20 @@ type VersionRequirement =
static member Parse text =
if String.IsNullOrWhiteSpace text || text = "null" then VersionRequirement.AllReleases else
let prereleases = ref PreReleaseStatus.No
let prereleases = List<string>()

This comment has been minimized.

@viktor-svub

viktor-svub Feb 7, 2018

Contributor

packages with transitive dependencies like >= 1.0.1-beta.1 < 1.0.1 -- which are valid and can be produced automatically -- ended up with No prerelease status, as it was overwritten by the second part
also, allowing All for transitive dependencies of Concrete prerelease packages ended up with unresolvable graphs in some cases (diamond dependencies on named prerelease)

@baronfel

Great changes overall, there's a lot of clarifying code movement and hardening of some areas we were soft. Can you take a look at the few questions I had?

((minimum.AsString.Split '-').[0].Split '.')
|> Array.map isNumeric
|> Array.takeWhile (fun i -> i.IsSome)
|> Array.choose (fun i -> i)

This comment has been minimized.

@baronfel

baronfel Feb 7, 2018

Contributor

this could be ((minimum.AsString.Split '-').[0].Split '.') |> Array.choose isNumeric, because the choose function wants 't -> 'x option and isNumeric fits that requirement. That lets you not have to do the takeWhile and choose necessarily.

This comment has been minimized.

@viktor-svub

viktor-svub Feb 7, 2018

Contributor

thanks for the catch! I'll look into that

This comment has been minimized.

@viktor-svub

viktor-svub Feb 7, 2018

Contributor

I believe the shorter version has different results - the intent is full stop on first non-numeric segment, not simply taking all the numeric ones

| AlphaNumeric a, AlphaNumeric b -> compare a b
| Numeric a, Numeric b -> compare a b
| AlphaNumeric a, Numeric b -> 1
| Numeric a , AlphaNumeric b -> -1

This comment has been minimized.

@baronfel

baronfel Feb 7, 2018

Contributor

good move, much cleaner to make this logic part of the type itself 👍

override x.GetHashCode() = hash x
member x.Equals(y) =
match x, y with

This comment has been minimized.

@baronfel

baronfel Feb 7, 2018

Contributor

again, good thinking

override x.ToString() = x.Origin
override x.GetHashCode() = hash x.Origin
member x.CompareTo(yobj) =

This comment has been minimized.

@baronfel

baronfel Feb 7, 2018

Contributor

Nice change, can we remove zipOpt now? I should never have written that thing.

@@ -96,19 +132,18 @@ type SemVerInfo =
/// The optional PreRelease version
PreRelease : PreRelease option
/// The optional build no.
Build : string
Build : bigint

This comment has been minimized.

@baronfel

baronfel Feb 7, 2018

Contributor

I think the compelling case for string here instead of bigint is the case of embedding commit hashes as the build metadata. The spec (section 10) says that build metadata can have the following form:

...Identifiers MUST comprise only ASCII alphanumerics and hyphen [0-9A-Za-z-]...

and so I think we must allow string here.

@@ -125,40 +160,83 @@ type SemVerInfo =
member x.AsString
with get() = x.ToString()
member x.Equals(y) =
x.Major = y.Major && x.Minor = y.Minor && x.Patch = y.Patch && x.Build = y.Build && x.PreRelease = y.PreRelease

This comment has been minimized.

@baronfel

baronfel Feb 7, 2018

Contributor

good catch, that never should have been there

| _ -> false
override x.GetHashCode() = hash (x.Minor, x.Minor, x.Patch, x.PreRelease, x.Build)
override x.GetHashCode() = hash (x.Minor, x.Minor, x.Patch, x.Build, x.PreRelease)

This comment has been minimized.

@baronfel

baronfel Feb 7, 2018

Contributor

why the change in order here?

This comment has been minimized.

@viktor-svub

viktor-svub Feb 7, 2018

Contributor

Build is a number (or simple string) and may be part of release versions like 1.2.3.4 so the comparison preference should be kept regardless if there is PreRelease present or not; plus, this way it's faster :)

This comment has been minimized.

@baronfel

baronfel Feb 7, 2018

Contributor

yes, correct again :)

This comment has been minimized.

@BillHally

BillHally Feb 8, 2018

Is this deliberately not including x.Major in the hash (though it does include x.Minor twice)?

This comment has been minimized.

@viktor-svub

viktor-svub Feb 8, 2018

Contributor

oh my, that's a very bad typo, thanks for the catch :)

@matthid

This comment has been minimized.

Member

matthid commented Feb 7, 2018

This looks very good!

Just some words of warning: This is a delicate part of Paket with high potential of breaking stuff (especially since there are a lot of packages out there which are not actually semver compatible) .
Therefore I suggest that this at least needs a green CI (Just to explain: We have a long history of broken CI and ignoring them for PRs on some occasions, if the failures look unrelated :) )

Also feel free to send the same PR to FAKE as we have a Fake.Core.SemVer package with basically the same logic, and it would be nice to have the proper author on the history :)

If I find the time before this thing is merged I might review this more in-depth, thanks for sending this.

@viktor-svub

This comment has been minimized.

Contributor

viktor-svub commented Feb 7, 2018

Thank you! And I definitely agree this needs a thorough review & passing CI :)
I'm aware of the risks, as this was created to resolve our very real production issues, and mainly to bridge the gap between semver2 and non-semver cases, supporting more of both.

@forki

This comment has been minimized.

Member

forki commented Feb 8, 2018

I didn't yet find time to look at this, but for quite some time I wanted to to have an integration test which checks ALL packages on nuget and verify we can understand the version no.

I know it's a big thing to ask, but tbh it would increase my confidence into this change dramatically if you could try to come up with such a test.

@viktor-svub

This comment has been minimized.

Contributor

viktor-svub commented Feb 8, 2018

Actually that's a quite good idea, it may just take few more hours to implement :)
I'll do that separately, to see the tests' results before the changes, and merge into this branch/PR later.

@matthid

matthid approved these changes Feb 9, 2018

I played around a bit with the code and couldn't break it. That obviously doesn't mean the NuGet ecosystem can't :).

If this PR indeed changed
/// parse "1.2.3-alpha002" > parse "1.2.3-alpha1" // true
That is Okisch by me. This is merely a workaround from the early NuGet days. Nowadays people hopefully use SemVer (and not old prereleases).

@@ -177,55 +244,59 @@ module SemVer =
for s in version.Split([|'.'|]) do

This comment has been minimized.

@matthid

matthid Feb 9, 2018

Member

Can you change the comment of this function.
I'm not sure if this PR changed this but the line

/// parse "1.2.3-alpha002" > parse "1.2.3-alpha1" // true

is no longer correct.

This comment has been minimized.

@viktor-svub

viktor-svub Feb 10, 2018

Contributor

I'm conflicted about this :) the change is side effect of preferring SemVer2 ordering, but I really want to keep compatibility with SemVer1 workarounds -- not even M$ packages (netcore) use v2 format and rules in all cases, and there are many companies (incl. ours) having thousands of improperly versioned packages in their repos.
In this case, real-world semantics should win, I'd expect -- alpha002 should be bigger than alpha1, as the zero-padded version was created as workaround of broken ordering above alpha9, maybe after alpha1 already existed.
Luckily, the SemVer1/2 special case handler is one of the simpler parts of the code :)

This comment has been minimized.

@matthid

matthid Feb 10, 2018

Member

Yes I agree, but this is about PRERELEASE only. And SemVer2 has been out for some time now. You should not depend on PRERELEASEs too long you know :). Can you give examples of currently used packages where this particular instance is a problem?

Personally I would have no problem with releasing this and look where and if it hurts someone and if we cannot figure out a workaround we can still revert and look for a solution.

This comment has been minimized.

@matthid

matthid Feb 10, 2018

Member

I just verified that this PR did not change this behavior:

> oldParse "1.2.3-alpha002" > oldParse "1.2.3-alpha1";;
val it : bool = false

Therefore this change is already live and probably nothing to worry about.

This comment has been minimized.

@viktor-svub

viktor-svub Feb 10, 2018

Contributor

As much as I like there currently is a widely accepted semver2, it does not cover all of use cases, and for some companies, named "prereleases" are the majority of their packages, being produced & consumed by named feature/bugfix branches.

Similar approach can be seen in bigger companies, where there is simply no feasible fast release cycle (netcore could stay years in prerelease).

From my PoV, removing the old workaround of parsing numbers off alphanumeric segments makes transition no newer scheme harder -- I'd like to safely assume that 1.2.3-beta001 is less-than 1.2.3-beta.2 :)

matthid added a commit to fsharp/FAKE that referenced this pull request Feb 10, 2018

@forki

This comment has been minimized.

Member

forki commented Feb 10, 2018

Really really cool.

@forki

This comment has been minimized.

Member

forki commented Feb 10, 2018

Should we merge and release as prerelease for couple of days?

@matthid

This comment has been minimized.

Member

matthid commented Feb 10, 2018

@forki can you elaborate what you meant with the “ALL packages“ test and how it would work. I don’t think we can download all versions from nuget within a single test run. But we can generate a list of test-cases (but it will be way too long to review and the F# compiler will probably choke as well).
Or I just don’t get the idea...

@matthid

This comment has been minimized.

Member

matthid commented Feb 10, 2018

We can maybe filter out “uninteresting” versions via regex.

@viktor-svub

This comment has been minimized.

Contributor

viktor-svub commented Feb 10, 2018

As far as I understood and agreed on the "all of nuget" test idea, it should be possible to use the NuGetv3 Catalog to get all versions of all packages.
If they were in gallery order (need to confirm this), I could check our parsing & comparison is able to 1/ read all versions, 2/ return the latest release, and 3/ return the latest prerelease.
It will take me few days to do this in F# by myself; Do we perchance already have some helper in Paket or FAKE for catalog access?

@forki

This comment has been minimized.

Member

forki commented Feb 10, 2018

@matthid

This comment has been minimized.

Member

matthid commented Feb 10, 2018

Yes the only thing we need to watch out for is that afaik NuGet will respond with different results depending on the client and the feed :)

@@ -277,7 +282,8 @@ let private getCatalogPageDirectory(basePath:String,item:String) =
| _ -> Uri.EscapeDataString(item)
let cachePath = Path.Combine(basePath,hostPath)
let directory = new DirectoryInfo(cachePath)
if directory.Exists |> not then
if directory.Exists |> not then
traceWarnfn "Create \"%s\"" directory.FullName

This comment has been minimized.

@forki

forki Feb 12, 2018

Member

Is this warning expected?

@@ -277,7 +282,8 @@ let private getCatalogPageDirectory(basePath:String,item:String) =
| _ -> Uri.EscapeDataString(item)
let cachePath = Path.Combine(basePath,hostPath)
let directory = new DirectoryInfo(cachePath)
if directory.Exists |> not then
if directory.Exists |> not then
traceWarnfn "Create \"%s\"" directory.FullName

This comment has been minimized.

@forki

forki Feb 12, 2018

Member

Is this warning expected?

This comment has been minimized.

@viktor-svub

viktor-svub Feb 12, 2018

Contributor

it's not, as the page cache for the only code using this -- the public catalog integration test --
should be committed (it is now) and the warning will trigger only if called without the path existing

@forki

This comment has been minimized.

Member

forki commented Feb 12, 2018

@viktor-svub

This comment has been minimized.

Contributor

viktor-svub commented Feb 12, 2018

Oh, I see -- I can change that to detailed warning about re/building catalog cache from scratch and expecting very long operation/delay, would that be ok?

@forki

This comment has been minimized.

Member

forki commented Feb 12, 2018

@viktor-svub

This comment has been minimized.

Contributor

viktor-svub commented Feb 12, 2018

Catalog support was created just for the "all of nuget" integration test, but placed alongside the other
NuGet V3 "services" implementations -- this function is new and not used anywhere else.

Cursor-based implementation like https://docs.microsoft.com/en-us/nuget/api/catalog-resource#iterating-over-catalog-items would be only reliable and efficient if I used a compact and append-only representation on client side -- so I went for a more tolerant per-page cache.

Creating the page cache takes still more than an hour though, which is where the warning is appropriate -- missing directory means the cache will be created from scratch.

It seems this may not be a viable approach in the end -- while locally subsequent test runs take about half a minute, the CI servers do not exactly like it.

@viktor-svub

This comment has been minimized.

Contributor

viktor-svub commented Feb 12, 2018

Ok, the test overhead outgrew its potential usefulness, I'm removing it from this PR.
I will try to come up with lightweight version, in another branch, as mixing the changes got messy.

@viktor-svub

This comment has been minimized.

Contributor

viktor-svub commented Feb 13, 2018

AppVeyor RunIntegrationTestsNetCore failed with

1) System.Exception: Test failed on "test" "integrationtests/Paket.IntegrationTests.preview3/Paket.IntegrationTests.fsproj"
...
Command exited with code 42

https://ci.appveyor.com/project/paket/paket/build/0.0.1.88/job/bp3w37f1hb14rp6x#L2812

No specific test failed, and on local machine, the whole build seems to pass -- any ideas?

@forki

This comment has been minimized.

Member

forki commented Feb 13, 2018

yes have that locally as well. wonder what changed!? /cc @enricosada @matthid

viktor-svub added some commits Feb 14, 2018

@viktor-svub

This comment has been minimized.

Contributor

viktor-svub commented Feb 15, 2018

So far it seems that failed or successful test runs differ in the machines' performance -- the latest failed run has startup time -- and almost any subsequent measured operation -- 2-3 times longer than the successful ones, end ends with job timeout :/

@forki

This comment has been minimized.

Member

forki commented Feb 15, 2018

@enricosada ideas?

@viktor-svub

This comment has been minimized.

Contributor

viktor-svub commented Feb 15, 2018

@forki you were completely right about the public catalog verification :-) there was a regression and the latest commits (semver2 allows hyphens...) are the fix with new unit-tests

As for the verification -- this branch was merged into PR #3048 which passed:
20df660 shows the intentional changes (lines ending with !0.0.0 were not parse-able and interpreted as zero in the original requirement parser)

Do we still need to wait for the same verification to pass here?

@forki

This comment has been minimized.

Member

forki commented Feb 15, 2018

I'm fine with merging if @matthid and you are saying it's "probably good" ;-)

@viktor-svub

This comment has been minimized.

Contributor

viktor-svub commented Feb 15, 2018

As there is no further activity planned here at the moment,
unless you have any objections, feel free to merge anytime :)

@forki forki merged commit a4ee610 into fsprojects:master Feb 16, 2018

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@matthid

This comment has been minimized.

Member

matthid commented Feb 16, 2018

Yes it probably is fine, thanks @viktor-svub for going all the way with us :)

@forki

This comment has been minimized.

Member

forki commented Feb 19, 2018

image

^^ in the latest builds

@viktor-svub

This comment has been minimized.

Contributor

viktor-svub commented Feb 19, 2018

Seems like conflict with #3040 -- I should have probably rebased/merged it immediately after this was merged. Line 6778 is especially suspicious, as it does not look like a parser issue but rather an ordering one (which I did not encounter before) or the catalog being updated accidentally during the test.
Note that updating is done through an TestCase("...",Explicit=true) "test" and if the attribute was ignored in the CI build runner, it would create quite interesting results.

@forki

This comment has been minimized.

Member

forki commented Feb 19, 2018

any advice how to fix?

@viktor-svub

This comment has been minimized.

Contributor

viktor-svub commented Feb 19, 2018

After looking a bit, it rather looks like the error expected to appear before #3030.
It's a merge/rebase issue most likely, or I might gave accidentally pushed older version of the catalog, i.e. not adapted by the #3030 -- as the catalog is saved in the "current" ordering, to detect any change in parser/order without keeping their older versions around :)

@viktor-svub

This comment has been minimized.

Contributor

viktor-svub commented Feb 19, 2018

Currently it can be fixed semi-manually as the updating "test" #3030-9 update nuget catalog from public index does too much -- re-ordered version of current catalog without update from gallery needs only getCatalogCursor to setCatalogCursor -- there will be only changed line, no added, and the flat-text file can be diffed and reviewed manually to see if the ordering was broken or rather fixed.

@viktor-svub

This comment has been minimized.

Contributor

viktor-svub commented Feb 19, 2018

Note that when the Expected line contained an exclamation mark and the But was did not, it's likely a fix rather error, as ! delimits the original version string and its normalized form if they differ, and non-parsed versions were interpreted as 0.0.0 in constraints -- that's what I see in the picture.

@forki

This comment has been minimized.

Member

forki commented Feb 19, 2018

ok. please please send a PR ;-)
thank you!

@viktor-svub

This comment has been minimized.

Contributor

viktor-svub commented Feb 19, 2018

I can push updated catalog after I get to my devbox, as there was already supposed to be one -- not sure how the old ordering ended up in the updated PR :)

@viktor-svub

This comment has been minimized.

Contributor

viktor-svub commented Feb 19, 2018

here #3056 is the catalog update to correct ordering :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment