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

NU1608 warning is not evaluating prerelease versions combined with exclusive upper bounds properly #6434

Open
bording opened this issue Jan 13, 2018 · 28 comments
Labels
Area:ErrorHandling Functionality:Restore Pipeline:Backlog Priority:2 Type:DCR
Milestone

Comments

@bording
Copy link

bording commented Jan 13, 2018

The NU1608 warning that was added in NuGet/NuGet.Client#1678 that displays when a package is outside of a dependency constraint is not working correctly when prerelease versions are combined with exclusive upper bounds.

The scenario:

  • PackageA version 1.0.0 has a dependency on PackageB defined as [1.0.0, 2.0.0)
  • The following versions of PackageB exist: 1.0.0, 2.0.0-beta1, 2.0.0
  • A project has the following package references:
    • <PackageReference Include="PackageA" Version="1.0.0" />
    • <PackageReference Include="PackageB" Version="2.0.0" />

In this case, an NU1608 warning is displayed as expected.

However, if the project's package references are instead:

  • <PackageReference Include="PackageA" Version="1.0.0" />
  • <PackageReference Include="PackageB" Version="2.0.0-beta1" />

No warning is displayed.

Given that PackageA's constraint on PackageB is [1.0.0, 2.0.0), any package with a major version > 1 should fall outside of that version range constraint, regardless if it's a prerelease package or not.

Based on that, referencing PackageB 2.0.0-beta1 should also cause an NU1608 warning.

NuGet product used: VS UI
VS version: 2017 15.5.3

cc: @adamralph

@nkolev92 nkolev92 added Functionality:Restore Area:ErrorHandling labels Jan 13, 2018
@nkolev92
Copy link
Member

nkolev92 commented Jan 13, 2018

I think what it's doing is correct.
2.0.0 > 2.0.0-beta1...so that puts beta1 in the range.

//cc
@emgarten

@bording
Copy link
Author

bording commented Jan 13, 2018

I think what it's doing is correct.
2.0.0 > 2.0.0-beta1...so that puts beta1 in the range.

When you consider the reason for putting an upper bound on a version range, I totally disagree.

If you're following SemVer, you're trying to avoid breaking changes. Picking up a prerelease version of a new major completely defeats the point of putting the upper bound there.

Moving to version 2 of PackageB means I have breaking changes. That means that that 2.0.0-beta1 also has breaking changes.

If I've declared that PackageA does not work with 2.0 of PackageB, then I want consumers of PackageA to see a warning letting them know that they're violating the constraint.

@nkolev92
Copy link
Member

nkolev92 commented Jan 13, 2018

SemVer only declares the ordering.

As I mentioned earlier, so 2.0.0 > 2.0.0.-beta1
https://semver.org/#spec-item-11

So the way NuGet implements the ranges is correct.
It's not intuitive but semver compliant.

The way to go here would be something like marking the upper bound as "2.0.0-0" as that'd be the lowest possible prerelease version of 2.0.0. @emgarten can correct me if I'm wrong here.

@nkolev92
Copy link
Member

nkolev92 commented Jan 13, 2018

Basically, think of it this way.

1.0.0 < 2.0.0-beta1 <2.0.0

And you're saying 1.0.0 and higher works, and lower than 2.0.0 works

@bording
Copy link
Author

bording commented Jan 13, 2018

The way to go here would be something like marking the upper bound as "2.0.0-" as that'd be the lowest possible prerelease version of 2.0.0. @emgarten can correct me if I'm wrong here.

Setting the range to [1.0.0, 2.0.0-) in my PackageB project to get to try to get the package to build with that dependency does not work:

C:\Program Files\dotnet\sdk\2.1.4\NuGet.targets(103,5): error : '[1.0.0, 2.0.0-)' is not a valid version string. 

Basically, think of it this way.

1.0.0 < 2.0.0-beta1 <2.0.0

And you're saying 1.0.0 and higher works, and lower than 2.0.0 works

While I understand what you're saying, and from a general sorting order you are correct, I feel like falling back on that to explain this behavior is largely missing the point of what specifying version ranges is for.

If I was specifying an inclusive upper bound, then I totally think that prerelease versions of 2.0.0 should be included. But if I've specified an exclusive upper bound, then I'm declaring that the major version of the package should never be 2.

@emgarten
Copy link
Member

emgarten commented Jan 13, 2018

2.0.0-0 would be the lowest valid pre-release version.

I get that this is confusing since you intend to limit the version to 1.x.

Ranges in NuGet have always worked this way and I think it would be a major breaking change to the most basic part of NuGet to change this behavior. I am not sure that this could or should be changed.

@bording
Copy link
Author

bording commented Jan 13, 2018

Ranges in NuGet have always worked this way and I think it would be a major breaking change to the most basic part of NuGet to change this behavior. I am not sure that this could or should be changed.

Then that means there are a whole bunch of people out there handling version ranges on packages incorrectly. I suspect this behavior would be quite the surprise to most people, in fact.

I totally get the whole "can't make breaking changes" thing, but this definitely feels like a bug.

At least the 2.0.0-0 workaround appears to work. Is that guaranteed to block all prerelease versions?

@nkolev92
Copy link
Member

nkolev92 commented Jan 14, 2018

I disagree with saying that this is a bug.
That's just how SemVer is, and NuGet is SemVer compliant.

It's definitely not the most intuitive thing, I agree, but having a value (2.0.0-beta1) mean one thing as an exact version, but another thing in a version range will be even more confusing.

@SimonCropp
Copy link

SimonCropp commented Jan 14, 2018

may "technically" not be a bug. but it is so outside the majority of peoples understanding and usage that it should change.

@abatishchev
Copy link

abatishchev commented Jan 14, 2018

So without changing anything in the existing behavior, how to specify 2.x) where 2.x is either release, prerelease, anything? In other words "no packages from the 2.x branch can be used" (including 2.0-rc-final, surprise!).

@adamralph
Copy link

adamralph commented Jan 14, 2018

I think this may be a regression. IIRC, earlier versions of NuGet (before <PackageReference>) did exhibit the desired behaviour, i.e. [1.0.0, 2.0.0) would exclude any 2.0.0 pre-release versions. I was involved in a discussion a few years ago about this very topic, and I'm all but certain it was agreed to implement to exclusive upper bounds in that way. Unfortunately I can't find the conversation (it may be hidden away somewhere in codeplex).

But more importantly, that is the desired behaviour. No-one will ever want to specify "use any 1.x versions, but no 2.x versions, unless they are pre-release". That scenario is plain silly.

Note that this is not a conversation about SemVer. SemVer is doing it's own thing and communicating the version info correctly. This is about how to use SemVer compliant version numbers in the context of version ranges, and the only sensible thing to do is to exclude any pre-release versions of the exclusive upper bound.

@mauroservienti
Copy link

mauroservienti commented Jan 14, 2018

Nuget documentation states:

Pre-release versions are not included when resolving version ranges. Pre-release versions are included when using a wildcard (*). The version range [1.0,2.0], for example, does not include 2.0-beta, but the wildcard notation 2.0-* does. See issue 912 for further discussion on pre-release wildcards.

That to me means that this is a bug.

@kbaley
Copy link

kbaley commented Jan 14, 2018

SemVer talks about ordering but I don't think it talks specifically about how to implement ranges. I.e. it says 2.0.0-beta1 < 2.0.0 but it doesn't say something like [1.0.0, 2.0.0) should be defined as 1.0.0 <= x < 2.0.0. That's Nuget's implementation of a specific syntax, isn't it?

For reference, npm has special handling of pre-releases: https://docs.npmjs.com/misc/semver

@rrelyea
Copy link
Contributor

rrelyea commented Jan 18, 2018

@nkolev92 and I talked this through.
I'm interested to know if @adamralph is right about a regression.

I also told him that I'd like to understand how we treat a few cases:
given that with [1.0.0, 2.0.0) with 2.0.0-b1 in the feeds, will take 2.0.0-b1 w/ no warning.
I'm interested to know what we do with: [1.0.0,2.0.0) with 2.0.0-b1 and 2.0.0 in the feeds.

@nkolev92 will try to squeeze some more investigation/analysis to this sprint or soon thereafter.

@nkolev92 nkolev92 added this to the Backlog milestone Jan 18, 2018
@nkolev92 nkolev92 self-assigned this Jan 18, 2018
@emgarten
Copy link
Member

emgarten commented Jan 18, 2018

I did some digging on this. At a low level VersionRange/VersionSpec behavior has never changed, [1.0.0, 2.0.0) has always allowed 2.0.0-alpha and I've verified this all the way back to NuGet 2.2.0

What has changed is that stable packages now allow pre-release dependencies in some additional scenarios. Here is an example:

  1. A 1.0.0 -> B [1.0.0, 2.0.0)
  2. A 1.0.0-alpha -> B [1.0.0, 2.0.0)

For packages.config scenario one does not allow 2.0.0-alpha, purely because it doesn't allow any pre-release packages in the graph by default since A 1.0.0 is stable.

In scenario two when the parent package is pre-release, or if the user opts in to pre-release packages, or if a higher level package is being installed that is pre-release then pre-release is allowed everywhere and 2.0.0-alpha will be added.

Example of this in NuGet 2.2.0:

nuget.2.2.0.exe install a -Output D:\tmp\versiontest\output\ -Source d:\tmp\versiontest\source -Pre
Attempting to resolve dependency 'b (≥ 1.0.0 && < 2.0.0)'.
Successfully installed 'b 2.0.0-alpha'.
Successfully installed 'a 1.0.0'.

For PackageReference stable packages have always been able to resolve pre-release dependencies by default. Which is likely the source of confusion here around how this works and what is allowed.

History on allowing pre-release dependencies

  • DNU originally allowed stable packages to depend on pre-release packages. We attempted to remove this but it was a breaking changes for packages that had already shipped as stable that depended on pre-release packages.
  • There was a long period of time where System.* packages were pre-release only, making it very hard to ship anything without pre-release versions.
  • Pack originally enforced that stable packages depend on only stable packages, but this check was removed after community feedback to make things easier. See: NuGet pack should not fail for release packages with prerelease development dependency #1365

packages.config

Summary

This is very painful, but I do not see this as a regression. Since version ranges have always worked this way in NuGet changing it would be a major breaking change.

A better way to expression version ranges would be to put them in terms of the lower bound only like npm. Example: ^1.0.0 instead of having to write in < 2.0.0. This would however break older clients that did not understand the syntax.

@abatishchev
Copy link

abatishchev commented Jan 18, 2018

This is very painful, but I do not see this as a regression

Does this mean that the implementation was always different from the documentation?

@emgarten
Copy link
Member

emgarten commented Jan 18, 2018

The documentation mentioned by @mauroservienti was a recent addition meant to explain that 1.* does not include 1.1.0-beta since there have been a lot of questions on around this. It is incorrect and will be updated.

@304NotModified
Copy link

304NotModified commented May 9, 2018

At least the 2.0.0-0 workaround appears to work.

Please add this to the docs until this issue is proper resolved!

@304NotModified
Copy link

304NotModified commented May 9, 2018

it looks like the -0 trick is not accepted by nuget.org? :(

(publish from AppVeyor) - https://ci.appveyor.com/project/nlog/nlog-framework-logging/build/1.0.634

Deploying using NuGet provider
Publishing NLog.Extensions.Logging.1.0.1.nupkg to https://www.nuget.org/api/v2/package...
Error publishing package. NuGet server returned 400: The NuGet package contains an invalid .nuspec file. The errors encountered were: 'The package manifest contains an invalid Version: '5.0.0-0'', 'The package manifest contains an invalid Version: '5.0.0-0'', 'The package manifest contains an invalid Version: '5.0.0-0'', 'The package manifest contains an invalid Version: '5.0.0-0'', 'The package manifest contains an invalid Version: '5.0.0-0''. Correct the errors and try again.

@304NotModified
Copy link

304NotModified commented May 10, 2018

also not working on manual upload. What am I missing?

image

airbreather added a commit to NetTopologySuite/NetTopologySuite that referenced this issue Feb 26, 2019
This won't make NTS 1.x *completely* fail to work with GeoAPI 2.x, but it will ensure that someone doesn't *accidentally* restore GeoAPI 2.x, and it lets the system raise NU1608 if they manually update anyway.

Working around NuGet/Home#6434 by putting "2.0.0-0" instead of just a bare "2" or something.

Fixes #283
@airbreather
Copy link

airbreather commented Feb 28, 2019

also not working on manual upload. What am I missing?

I'm getting the same error. Our .nuspec file is here. The package itself builds properly, and our CI (which pushes to MyGet.org) had no problems whatsoever* getting it there. nuget.org appears to be in the wrong?

*edit: the link takes you to 1.15.2-pre017; to verify, I opened the package file in 7-zip and confirmed that the included .nuspec did have the "2.0.0-0" upper-bounds specified.

image

@nkolev92
Copy link
Member

nkolev92 commented Feb 28, 2019

I have created an issue on Gallery side. Please follow that for further updates on the nuget.org issue NuGet/NuGetGallery#6950.

Edit

See #6434 (comment).

Follow NuGet/NuGetGallery#6948 instead.

@airbreather
Copy link

airbreather commented Feb 28, 2019

I have created an issue on Gallery side. Please follow that for further updates on the nuget.org issue NuGet/NuGetGallery#6950.

Heh, I'd reported it too shortly after my comment... NuGet/NuGetGallery#6948

@bording
Copy link
Author

bording commented Feb 28, 2019

I still think that NuGet's behavior here is wrong, and we shouldn't have to resort to the sort of tricks that aren't working on the Gallery to try and work around NuGet's broken behavior.

@nkolev92
Copy link
Member

nkolev92 commented Feb 28, 2019

I'll refer to @emgarten's investigation. #6434 (comment).
Changing this in any manner will have so many ripple effects.

Is it intuitive? No.
But this is how it always worked.

@304NotModified
Copy link

304NotModified commented Feb 28, 2019

But this is how it always worked

This isn't very constructive....

@airbreather
Copy link

airbreather commented Feb 28, 2019

I still think that NuGet's behavior here is wrong

I agree + disagree... IMO the defect is in the requirements, rather than the design or implementation, which I'm guessing why we're seeing responses that indicate that this is all working as intended.

We have a need to be able to specify "this dependency can only be satisfied by a version of that package whose major version is less than X", because according to SemVer, a major version could come with breaking changes, and so when everybody's following the rules and paying attention to the feedback from the system, this should be enough to stave off DLL Hell.

The nearest solution that we are provided with are "version range strings" that use SemVer precedence semantics for determining inclusion / exclusion.

The mismatch between our need and the existing solution seems to be the cause of all this frustration, because the "obvious" / "intuitive" way to try to use the "version range string" tool to meet our need has a glaring flaw (namely, we are guided towards things like "[1.0, 2.0)", which unexpectedly accepts a dependency with version "2.0.0-pre001").

IMO, the ideal solution would be to specifically and narrowly change the semantics: if a version range string a) has an exclusive upper-bound that b) does not specify a prerelease tag, then consider that version range to exclude a prerelease version of a version that would be excluded by the upper-bound if the prerelease tag were removed. Perhaps this is my bias showing, but it's difficult for me to imagine a package with a dependency version range of "[1.0, 2.0)" that would break if NuGet stopped accepting "2.0.0-pre001".

I'd also be perfectly content with any other straightforward means of conveying the intent.

I'm also not going to complain too much about having to write "2.0.0-0". It's something I only need to learn once, and it has the benefit of actually doing what I want.

we shouldn't have to resort to [...] tricks

100% agreed

@SeanFeldman
Copy link

SeanFeldman commented Oct 25, 2019

When a range is defined as [ X.0.0, Y.0.0 ), the expectation is not to include anything from Y.* version. That would include pre-releases as well. The way it functions right now it utterly confusing and broken.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area:ErrorHandling Functionality:Restore Pipeline:Backlog Priority:2 Type:DCR
Projects
None yet
Development

No branches or pull requests