Paket: Prerelease packages won't used during resolving

[Stift]

Description

I have two packages A and B and the package A has a dependency on B.

My dependency file looks like
````
source Externals/NugetStore

nuget PackageA
nuget PackageB 1.0.11204-custom
````

With paket 4.8.8 the paket lock file looks like
NUGET remote: Externals/NugetStore PackageA (1.0.11250) PackageB (>= 1.0 < 2.0) PackageB (1.0.11204-custom)

But now with paket 5.2.5 I get the following output:
````
Paket version 5.2.5
Resolving packages for group Main:

• PackageB is pinned to 1.0.11204-custom
• PackageA 1.0.11250
  Incompatible dependency: PackageB >= 1.0 < 2.0 conflicts with resolved version 1.0.11204-custom
  Performance:
• Resolver: 356 milliseconds (1 runs)
  
  
  
  • Runtime: 292 milliseconds
  
  
  • Blocked (retrieving package details): 63 milliseconds (2 times)
  
  
  • Not Blocked (retrieving package versions): 2 times
  
  
• Disk IO: 42 milliseconds
• Runtime: 1 second
  Paket failed with:
  -> There was a version conflict during package resolution.
  Resolved packages:
  
  
  
  • PackageB 1.0.11204-custom
    
    Conflict detected:
  
  
  • Dependencies file requested package PackageA: >= 0
  
  
  • Available versions:
    
    
    
    
    
    • (1.0.11250, [Externals/NugetStore])
    
    
    
  
  

 Please try to relax some conditions or resolve the conflict manually (see http://fsprojects.github.io/Paket/nuget-dependencies.html#Use-exactly-this-version-constraint).

````

Repro steps

Use the Repro.zip

1. .paket\paket.bootstrapper.exe

2. .paket\paket.exe install

Expected behavior

Because the PackageB is pinned I would expect that this is taken as long as it fits into the constraints. To pin the package is kind of testing new prereleases. I wonder how this should be able without repackaging all packages.
A similar issue is described in #2459, but the recommendation that the package has to specify that it allows this as dependency does not seem valid to me. The definition of the version specifier in the nuspec file does not allow to specify more than just version numbers.

Actual behavior

Please provide a description of the actual behavior you observe.

Known workarounds

1. Switch back to paket 4.8.8
2. Pin all packages (anti-pattern)

[matthid]

Paket is correct that the combination is not compatible. This is because by default we assume nuget version ranges do not include prereleases. The reason because it doesn't work anymore is because of a bugfix in the resolver which had allowed invalid resolutions (like this one)

Ideally I think we need a syntax to tell paket to allow prereleases for transitives. I think a discussion about this is open somewhere.

[Stift]

I'm not sure whether I agree. Two things:

1. When I order semver versions according to the limited scope how nuget these implements, then an order would look like 1.0.0, 1.0.1, 1.0.2-beta, 1.0.2, etc
   According to this I would say that 1.0.2-beta is in range [1.0.0, 2.0.0)
2. In the example above the dependency is not a transitive dependency. It's a direct one, so the user defined what he wants to use and it still satisfies the constraint.

IMO this breaks a lot of workflows when testing betas. I wonder how any prerelease package can be used and tested in the future. Our current workflow was to add a dependency and start install + run tests.

Can you please point me to the discussion mentioned (didn't find any in the issue list here).

[matthid]

@Stift While technically you are of course correct (it is in the range) - we don't want to just resolve/pull in some pre-releases in the dependency tree by accident.

Previous discussions:

• https://github.com/fsprojects/Paket/issues/2459
• https://github.com/fsprojects/Paket/issues/2386
• https://github.com/fsprojects/Paket/issues/2326

So in essence: Use == for now

[Stift]

== works, thanks for that.
I still think that this can't be correct. Because a dependency definition in my paket.dependencies file should be treated equal regardless what version (and/or prerelease) I take as long as it complies with other constraints.
That while resolving automatically you should not resolve to a prerelease is understandable, but the constraint definition of the user should be top prio.

So from my above example I conclude the following what works:

• nuget PackageB 1.0.11204 :white_check_mark:
• nuget PackageB 1.0.11204-custom :red_circle:
• nuget PackageB prerelease :red_circle:

Would it be better to display a warning/comment, when the user uses the to last examples?

[matthid]

nuget PackageB prerelease

should work and resolve the latest stable specified by PackageA.

as it complies with other constraints.

That's the point. It doesn't in your case. The problem is that the semantics of nuget PackageA are that we do not allow prereleases in the resolution. See https://github.com/fsprojects/Paket/issues/2386#issuecomment-305818844 where I try to discuss syntax to allow that.

but the constraint definition of the user should be top prio.

Yes, but I think it is unexpected to extend the allowed ranges of one dependency based on another direct dependency.

[stendavid]

We're seeing the same problem. We currently resolved it by going back to paket 4.8.8, but it would be nice if at least packages specified as pre-release in package.dependencies were allowed as pre-release all the way.

Could it be so simple as ignoring pre-release when checking against resolved versions, with the thinking that if something dragged in a pre-relase version (e.g. the dependencies file) then we actually want it.

It seems to work in my case, but maybe it's too simplistic?

~~
let isPackageCompatible (dependencies:DependencySet) (package:ResolvedPackage) : bool =
dependencies
// exists any not matching stuff
|> Seq.exists (fun (name, requirement, restriction) ->
- if name = package.Name && not (requirement.IsInRange package.Version) then
+ if name = package.Name && not (requirement.IsInRange (package.Version,true)) then
tracefn " Incompatible dependency: %O %O conflicts with resolved version %O" name requirement package.Version
true
else false
)
|> not // then we are not compatible
~~

[matthid]

@zzdavid I think this also allows the case where one package pulled a prerelease via transitives but another one doesn't allow it.

PackageA 1.0 depends on PackageW 1.0-alpha1
PackageB 1.0 depends on PackageW ~> 1.0

Your change will probably pull an alpha of PackageW without warning, which I think is unexpected.

[matthid]

@zzdavid Maybe you can send a PR, mark is as WIP and we can look at the test-suite results...

[Stift]

@matthid nuget packageB prerelease doesn't work. And according to your statement before this is expected as paket is ignoring every prerelease setting. I think when paket sees the package constraint as only-allow-stable, this issue will never be solved.

What I don't understand: When I have in my dependency file only(!!) nuget packageB prerelease then this will work. When I add another package that depends on packageB (like in the example above) then it won't work. I still say packageB is my direct dependency (I manage this) and give me that version as long as it is in the range.

@zzdavid I see a problem with your approach, because every prerelease, would be accepted, isn't it? But give it a try.

[stendavid]

Aha, I see, IsInRange (package.Version,true) actually completely ignores the pre-release version? I missed that. But maybe adding another flag _allowPrerelease_ to IsInRange could work?

[matthid]

as long as it is in the range.

But it isn't from the point of view of paket. That's why it doesn't work.

nuget packageB prerelease doesn't work. And according to your statement before this is expected as paket is ignoring every prerelease setting.

I guess I'd need to take another look. Currently I think this should work as long as a matching stable package of packageB is available

[Stift]

@matthid I think we don't have to discuss this further. The positions are made, and I understand the current view of paket, I never said that paket should generally resolve prereleases, but pinned versions or prerelease setting by the users are different. I think this is a crucial point (for paket), and maybe has to be solved somewhere else. I close this issue for now as you say this is expected/wished and there will be no change. I have to see whether this still fits for me.

[matthid]

I close this issue for now as you say this is expected/wished and there will be no change.

I didn't say that. Just saying we need to be careful. I even argued similar to your position here: https://github.com/fsprojects/Paket/issues/2326
I also definitely agree that this is unexpected at the very least...

If we allow this case (again, I don't have any objections against that) we just need to ensure that other transitive resolutions are still forbidden. Also we need a clear "spec" what nuget PackageA means with regards to prereleases and other direct dependencies. Currently it is well defined (though a bit unexpected) I'd like to keep it that way. Unfortunately, I don't have any suggestions besides what I have already written in the other issues. But if somebody can up with something working we'd definitely accept it.

[forki]

we definetely need a way to override dependency settings in packages

== is a good start but not enough.

@Stift Would the following make sense for you?

nuget PackageB !>= 1.0.11204-custom

[matthid]

@forki but the problem is the PackageA line not the PackageB one?

[forki]

is it? I thought the issue is that package a defines a transitive dependency on package b but not as prerelease.

[matthid]

Yes, so the problem are that the deps of PackageA are not compatible with prereleases. That's why I suggested to change something on the PackageA line to allow prereleases (not sure about the syntax)

[forki]

I think this is not what people want to do here.
I always assumed people want to override the restrictions on specific packages.

[matthid]

btw. nuget PackageB !>= 1.0.11204-custom is already valid syntax afaik and changes transitives of PackageB to min resolution (https://fsprojects.github.io/Paket/nuget-dependencies.html#Min-modifier)

[forki]

yeah. that would be bad syntac then ;-)

[matthid]

@forki So it's fine to change resolution behavior of the nuget PackageA line depending on other lines?

[matthid]

Maybe we should just make it work without syntax change.

We do resolve prereleases when there is no other option, correct? So we probably should do the same here.

[forki]

@matthid it's not only about prereleases. prereleases are just a special case of the issue,

What I think is the following (syntax is up for discussion):

source Externals/NugetStore

nuget PackageA
nuget PackageB >= 1.0.11204-custom OVERRIDE

with this we would resolve like we do now, but when A gets drawn into the resolution we would not add the additional PackageB restriction (thee one from A's nuspec) into the open requirements.

This would allow users to patch every restriction in the system.

[Stift]

Maybe I'm blind, and don't understand everything, so pls forgive me.
We all agree, that prerelease is not transitive, and only should be used when requested specifically

But as I shown above decided on packageB I don't get any resolve. For me the normal pin ('=') or prerelease seem good. The '==' works currently, but only when we accept the current situation as default. As written in the docs, this should be used when a resolve cannot be relaxed.

The use case is often, that we test new packages before we put them to the wilderness (80+ devs). Therefore I can pin the version, resolve, run tests etc and then continue.

When I look at the code, I would assume, we have to change that before packageB can be resolved it has to be looked up (transported), what/whether is the pinned and what is the setting for that package.

IMO we don't need any additional syntax. We have everything.

[matthid]

@forki While this makes a lot of sense in some scenarios I'm not sure if that is what @Stift wants.
This way we wouldn't notice if PackageA updates the restrictions to something completely not compatible. Paket would still happily accept that (as it now completely ignores the nuspec).

it's not only about prereleases. prereleases are just a special case of the issue,

Can you give an example? Because currently I still think this is only some prerelease special because of how we have prerelease logic implemented. As @Stift correctly noted above it is in range of the semver version, it's just an implementation detail of paket that we don't allow it by default. I think all we want is that this special case with prereleases is handled better if we explicitly specify a prerelease version in the dependencies file (because users can't understand why the resolution is invalid, when it's completely valid according to semver)

[stendavid]

@matthid @Stift created a pull request with the suggestion from earlier. From what I can see it does work, as ignorePreRelease=true still uses pre-release for the actual comparison. Not sure I understand the whole discussion in this thread, but it could solve the original resolution problem. Could use a good look though :)

[forki]

ok forget my proposal for now. Maybe it's too much for this case.

other question: why do we say

 Incompatible dependency: PackageB >= 1.0 < 2.0 conflicts with resolved version 1.0.11204-custom

when it's actually in the range?

I think we should only exclude 1.0.0-custom and 2.0.0-custom

[matthid]

Like I said it's because PackageA (where the range is coming from) doesn't allow prereleases by default (at least that's just how it's implemented at the moment).

[matthid]

Or said in other words: When we see >= 1.0 < 2.0 anywhere in the code-base we always assume "without prereleases" by default. We are a bit careful when to use prereleases to not accidentally pull them somewhere...

[forki]

@matthid yes thought about it again. that is correct. otherwise we would always draw in prereleases

[Stift]

that is right, but my paket.dep file says you should pull it. Either by using a pin or by stating prerelease.

[forki]

@Stift yes. only talking about my general assumption.
currently looking at the concrete sample. give me a bit

[matthid]

We should also answer what we want to do with transitives of the prerelease?
What if we encounter the same conflict on transitives of direct and specified prerelease packages?

This will be fiddeling out through the resolver and we need to watch out to not pull prereleases on some unrelated parts of the dependency tree where we could have used stable ones...

That's why I said it is hard and we need to think about it...

[Stift]

For clarification I'll try to bring an example setup and expectations.

Assuming we have in our feed the following packages

• PackageA (1.0.11250)
• PackageB (1.0.11203)
• PackageB (1.0.11204-custom)
• PackageB (2.0.0)

and packageA has dep on packageB ~> 1

Given the paket.dependencies file

1. 
   nuget PackageA nuget PackageB
   gives paket.lock
   NUGET remote: Externals/NugetStore PackageA (1.0.11250) PackageB (>= 1.0 < 2.0) PackageB (1.0.11203) # because this this the latest stable
2. 
   nuget PackageA nuget PackageB prerelease
   gives paket.lock
   NUGET remote: Externals/NugetStore PackageA (1.0.11250) PackageB (>= 1.0 < 2.0) PackageB (1.0.11204-custom) # because this the latest in range and prereleases are allowed by paket.dependencies

1. nuget PackageA nuget PackageB 1.0.11204-custom
   gives paket.lock
   NUGET remote: Externals/NugetStore PackageA (1.0.11250) PackageB (>= 1.0 < 2.0) PackageB (1.0.11204-custom) # because this is requested by paket.dependencies and in range

2. nuget PackageA nuget PackageB == 2.0
   gives paket.lock
   NUGET remote: Externals/NugetStore PackageA (1.0.11250) PackageB (>= 1.0 < 2.0) PackageB (2.0) # because this is overriden by paket.dependencies (range check disabled)

[matthid]

@Stift can you add those to https://github.com/fsprojects/Paket/pull/2474 as unit tests for the resolver?

[forki]

I think I have a fix. but @Stift tests would help a lot

[stendavid]

@matthid Why don't you want to allow pre-release for transitive dependencies? It's more of a gray area than the dependencies file, but with dependencies like this
~~
PaketA 1.0-pre ->PaketB 1.0-pre
~~

I would be happy to get pre-release of PaketB pulled in.

The pull request doesn't touch the actual pulling in of pre-release versions. It's only saying "ok, we already resolved a package to pre-release, so let's trust that and allow it if it's in range". You could say only allowing pre-release packages from the dependency file is an extra security check, but it's also limiting.

[Stift]

@zzdavid transitivity can really lead to problematic situations where you can't overview the impact of such a decision. The case I designed above is IMO different, because I say the packge I defined explicitly should be allowed to be pulled from a prerelease stream. Often you switch to prerelease because bugs are fixed, but not yet in stable.

[matthid]

@zzdavid I personally have no problems with allowing prereleases for transitives as well, as long as we ensure it is limited to only the prereleases we actually need (stables should be preferred if possible) and as long as we don't pull prereleases in unrelated places...

regarding your change: Yes but we also need to ensure it works when the resolver decides to do things in different orders...

[Stift]

@forki @matthid I added the tests, pls see the commit for details

[stendavid]

@matthid @Stift I see. I guess sometimes getting pre-release and sometimes not depending on the order the resolver happens to choose is a bad thing...!

[Stift]

I tested all the behaviours and it seems to work.

Thanks for all the fruitful discussion, I'll now close that issue.