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

Support versions with multiple "-" or "+" signs #478

Merged
merged 1 commit into from
Jan 18, 2021

Conversation

kojoru
Copy link
Contributor

@kojoru kojoru commented Jan 15, 2021

Currently if the version looks like 1.0.0-alpha-beta it will be treated like it has a PreRelease part equal to alpha.

The PreRelease part needs to be alpha-beta if you look at semver spec, that allows extra dash characters in the PreRelease part.

The BuildMetadata part has a similar issue, with the plus sign., i. e. for 1.0.0+123+321 it will be computed to 123 and not 123+321. Multiple pluses in version is not valid semver, and will probably cause an error when used with nuget. Despite that, it still feels like a similar issue should be fixed there.

In sum, for version 1.0.0-alpha-beta+123+321 this commit will set MinVerPreRelease to alpha-beta instead of current alpha and will set MinVerBuildMetadata to 123+321 instead of current 123.

Currently if the version looks like `1.0.0-alpha-beta` it will be treated like it has a `PreRelease` part equal to `alpha`.

The `PreRelease` part needs to be `alpha-beta` if you look at [semver spec](https://semver.org/#spec-item-9), that allows extra dash characters in the `PreRelease` part.

The `BuildMetadata` part has a similar issue, with the plus sign., i. e. for `1.0.0+123+321` it will be computed to `123` and not `123+321`. Multiple pluses in version is not valid semver, and will probably cause an error when used with nuget. Despite that, it still feels like a similar issue should be fixed there.

In sum, for version ``1.0.0-alpha-beta+123+321` this commit will set `MinVerPreRelease` to `alpha-beta` instead of current `alpha` and will set `MinVerBuildMetadata` to `123+321` instead of current `123`.
@kojoru
Copy link
Contributor Author

kojoru commented Jan 15, 2021

Note: I think it will work but haven't tested it, as I don't know how.

Split takes (char[], int), but funnily enough `-` (with backticks) is a char[] array (as opposed to '-' with apostrophes which is just a char) because the backtick symbol in MSBuild is a magical array creator. MSBuild is fun.

@adamralph
Copy link
Owner

@kojoru thanks for raising this PR. You're absolutely right about the pre-release identifiers! I can easily reproduce the problem:

image

However, I'm not sure that build metadata is allowed to contain a +. The build metadata section in SemVer says:

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

And if I try building a .NET project with build metadata containing a plus, the SDK returns an error:

C:\Program Files\dotnet\sdk\5.0.102\Sdks\NuGet.Build.Tasks.Pack\build\NuGet.Build.Tasks.Pack.targets(84,5): error MSB4018: System.ArgumentException: PackageVersion string specified '1.0.0-alpha-beta+123+321' is invalid. [C:\code\test\test.csproj]

@adamralph adamralph added this to the 2.5.0 milestone Jan 15, 2021
@kojoru
Copy link
Contributor Author

kojoru commented Jan 15, 2021

@adamralph no, the build metadata is not allowed to contain a second plus indeed. It just felt to me like adding the same clause there will lead to less surprises over what's happening, and overall a better UI for what is a user error either way.

@adamralph
Copy link
Owner

@kojoru yes, I agree. After I wrote my comment, I thought about how MinVer constructs versions. If it finds a version tag it does the absolute minimum required to split it into major, minor, pre-release, and build metadata and does not do any kind of validation of SemVer rules such as "only ASCII alphanumerics and hyphens [0-9A-Za-z-]". Similarly it will just use separately specified build metadata, or a specified default pre-release phase as-is. Ultimately, the version is controlled by the user and it's up to them whether they want to adhere to SemVer or not. And it's also up to the SDK to decide what it allows and what it does not. There's no point trying to pre-empt that in MinVer, because those rules may change over time.

When I get a moment, I'll add a test to this branch and then merge it. Thanks again!

@adamralph
Copy link
Owner

adamralph commented Jan 15, 2021

BTW, FWIW, this is a bug. I always intended it to work in the way you describe. I made a mistake by not limiting those two splits to two items only. This is the offending PR — https:/adamralph/minver/pull/166/files.

The code which parses the tags does it correctly —

var versionAndMeta = text.Split(new[] { '+' }, 2);
var numbersAndPre = versionAndMeta[0].Split(new[] { '-' }, 2);

This means that the version itself is correct. It is only the two output properties, MinVerPreRelease and MinVerBuildMetaData, which are incorrect.

@kojoru
Copy link
Contributor Author

kojoru commented Jan 18, 2021

@adamralph is there a way I can help with the tests?

@adamralph
Copy link
Owner

@kojoru thanks for offering. I'm currently in the middle of moving all the package tests out of the targets project and into a proper xUnit.net test library, so it's not really worth putting any effort into the current tests.

I hacked together a quick test locally on this branch and it works as you intended:

MinVer: [output] MinVerVersion=1.2.3-alpha.1-x+foo+bar.build.6
MinVer: [output] MinVerMajor=1
MinVer: [output] MinVerMinor=2
MinVer: [output] MinVerPatch=3
MinVer: [output] MinVerPreRelease=alpha.1-x
MinVer: [output] MinVerBuildMetadata=foo+bar.build.6
MinVer: [output] AssemblyVersion=1.0.0.0
MinVer: [output] FileVersion=1.2.3.0
MinVer: [output] PackageVersion=1.2.3-alpha.1-x+foo+bar.build.6
MinVer: [output] Version=1.2.3-alpha.1-x+foo+bar.build.6

That's good enough for me to know that this works, so I'm going ahead and merging.

@adamralph
Copy link
Owner

@kojoru I've released this in 2.5.0-alpha.1, if you want to give it a try.

@adamralph adamralph mentioned this pull request Jan 18, 2021
11 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants