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

VersionExtensions Regex does not support package ID ending with numbers #472

Open
CrazyOrange opened this issue Oct 27, 2015 · 9 comments
Open
Labels
bug:failing-test-would-be-nice Issues that would benefit from an automated test to confirm the problem help wanted Issues and features that are open to external contribution

Comments

@CrazyOrange
Copy link

Hi,

if you use NuGet Package Explorer and your nuget package id ends with a number updates won't work anymore (caused by changes in VersionExtensions.cs for prerelease support).

The NuGet Package Explorer uses a point to seperate the package id from the version number (at least if you don't change the filename).

ID: MyId123
Version: 1.1.0

The result would be "MyId123.1.1.0.nupkg"

The suffix regex will remove ".nupkg" and the version regex will match "123.1.1.0" instead of "1.1.0".

A workaround would be renaming your NuGet Package to "MyId123-1.1.0.nupkg".
The code should be changed or this naming issue should be noted somewhere.

@shiftkey shiftkey changed the title VersionExtensions Regex can cause trouble VersionExtensions Regex does not support package ID ending with numbers May 5, 2019
@shiftkey
Copy link
Contributor

shiftkey commented May 5, 2019

I believe this is still a problem based on testing what is currently shipped with 1.9.1:

static readonly Regex _versionRegex = new Regex(@"\d+(\.\d+){0,3}(-[A-Za-z][0-9A-Za-z-]*)?$", RegexOptions.Compiled);

Aside from the obvious workaround of changing the package name, it also doesn't look like we've got any tests to cover this regression. Is this something that you'd be interested in submitting @CrazyOrange?

@shiftkey shiftkey added help wanted Issues and features that are open to external contribution bug:failing-test-would-be-nice Issues that would benefit from an automated test to confirm the problem labels May 5, 2019
@MeikTranel
Copy link

MeikTranel commented May 21, 2019

Hey @shiftkey i took a look at this and maybe found a bigger issue at large? Did we ever write down which kind of SemVer we are supporting? Because the more widely used SemVer 2.0 does not support 4-part version numbers, but we're accepting them as compatible:

[InlineData("0000000000000000000000000000000000000000 MyCoolApp-1.2.3.4-full.nupkg 123", 1, 2, 3, 4, "", false)]
[InlineData("0000000000000000000000000000000000000000 MyCoolApp-1.2.3.4-delta.nupkg 123", 1, 2, 3, 4, "", true)]
[InlineData("0000000000000000000000000000000000000000 MyCoolApp-1.2.3.4-beta1.nupkg 123", 1, 2, 3, 4, "beta1", false)]
[InlineData("0000000000000000000000000000000000000000 MyCoolApp-1.2.3.4-beta1-full.nupkg 123", 1, 2, 3, 4, "beta1", false)]
[InlineData("0000000000000000000000000000000000000000 MyCoolApp-1.2.3.4-beta1-delta.nupkg 123", 1, 2, 3, 4, "beta1", true)]

This is a problem for this issue because to my knowledge this task can't be done without spelling out most of the semver ruleset in the regex. Even then we would never be able to make it actually compatible with semver 2.0. Take this:
PackageId: Zero.5
Version: 2.0
would result in a file: Zero.5.2.0-full.nupkg
There's simply no way to differentiate between version and packageid - given that the package explorer continues to save using dot separators. both parameters are valid inputs for nuget packages.
Am i seeing something wrong here?
If not... how would we go forward?

@MeikTranel
Copy link

I just checked specs again... not even semver 1.0 considers them compatible. but even then: There is no way to allow the dot separator while simultaneously supporting semver. There just is no way to support this. PackageIDs allowing packages like "p.123" to exist crosses us here.

@Thieum
Copy link
Contributor

Thieum commented May 21, 2019

See also #917 (and this comment too) - not sure the 4-part version is supported all the way.

@Thieum
Copy link
Contributor

Thieum commented May 21, 2019

@maxbrunsfeld Do you rememember why 4 digits release numbers were added to the requirements of #450 ?

@MeikTranel
Copy link

MeikTranel commented May 21, 2019

Just to clear up any confusion - there's two separate issues here:

  • clarification on what semver we support is needed to do this issue in the first place - this is why i linked the tests
  • after some thinking i came to the conclusion that packageids ending with ".12356789" are valid, but completely block us on supporting dot separated nupkg filenames

While the issue describes a packageid without a dot before the number, i think we have to think about not supporting the scenario at large - otherwise the next rightfully raised issue will torpedo any effort made here.

@Thieum
Copy link
Contributor

Thieum commented May 29, 2019

See also issue #1394 and PR #865 / #928

@maxbrunsfeld
Copy link
Contributor

Do you rememember why 4 digits release numbers were added to the requirements of #450?

As far as I can remember, the purpose of that PR was to allow Squirrel to work with semver version numbers with prerelease strings, like 1.2.3-beta0.

Maybe the versionRegex could be tweaked in some way to avoid this problem?

@MeikTranel
Copy link

In any case a decision has to be made:

  • Resolve this issue, while adding squirrel-specific non-nuget requirements to the Package ID
  • Refuse this issue, freezing existing squirrel-specific requirements to the Package ID
  • Refuse this issue, also drop support for dot-separated .nupkg filenames (essentially dropping support for nuget package explorer made packages)
  • Resolve this issue by dropping the filename as the source of truth for the package version - inspect .nuspec file inside the .nupkg archive to gain the version information instead

From a user perspective the last one of course seems like the best case scenario, but i must admit i do not grasp the architecture of squirrel enough to tell whether or not this would shoot us in the foot in the long run.
If y'all want to give it a try i could work on a PR, but i'm not sure whether this is worth the effort.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug:failing-test-would-be-nice Issues that would benefit from an automated test to confirm the problem help wanted Issues and features that are open to external contribution
Projects
None yet
Development

No branches or pull requests

5 participants