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
Prerelease packages won't used during resolving #2471
Comments
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. |
I'm not sure whether I agree. Two things:
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 Can you please point me to the discussion mentioned (didn't find any in the issue list here). |
@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:
So in essence: Use |
So from my above example I conclude the following what works:
Would it be better to display a warning/comment, when the user uses the to last examples? |
should work and resolve the latest stable specified by
That's the point. It doesn't in your case. The problem is that the semantics of
Yes, but I think it is unexpected to extend the allowed ranges of one dependency based on another direct dependency. |
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?
|
@zzDavid I think this also allows the case where one package pulled a prerelease via transitives but another one doesn't allow it.
Your change will probably pull an alpha of |
@zzDavid Maybe you can send a PR, mark is as WIP and we can look at the test-suite results... |
@matthid What I don't understand: When I have in my dependency file only(!!) @zzDavid I see a problem with your approach, because every prerelease, would be accepted, isn't it? But give it a try. |
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? |
But it isn't from the point of view of paket. That's why it doesn't work.
I guess I'd need to take another look. Currently I think this should work as long as a matching stable package of |
@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. |
I didn't say that. Just saying we need to be careful. I even argued similar to your position here: #2326 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 |
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?
|
@forki but the problem is the |
is it? I thought the issue is that package a defines a transitive dependency on package b but not as prerelease. |
Yes, so the problem are that the deps of |
I think this is not what people want to do here. |
btw. |
yeah. that would be bad syntac then ;-) |
@forki So it's fine to change resolution behavior of the |
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. |
@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):
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. |
Maybe I'm blind, and don't understand everything, so pls forgive me. 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. |
@forki While this makes a lot of sense in some scenarios I'm not sure if that is what @Stift wants.
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) |
@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 :) |
ok forget my proposal for now. Maybe it's too much for this case. other question: why do we say
when it's actually in the range? I think we should only exclude 1.0.0-custom and 2.0.0-custom |
Like I said it's because |
Or said in other words: When we see |
@matthid yes thought about it again. that is correct. otherwise we would always draw in prereleases |
that is right, but my paket.dep file says you should pull it. Either by using a pin or by stating prerelease. |
@Stift yes. only talking about my general assumption. |
We should also answer what we want to do with transitives of the prerelease? 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... |
For clarification I'll try to bring an example setup and expectations. Assuming we have in our feed the following packages
and packageA has dep on packageB ~> 1 Given the
gives paket.lock
gives paket.lock
gives paket.lock
gives paket.lock
|
I think I have a fix. but @Stift tests would help a lot |
@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
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. |
@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. |
@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... |
I tested all the behaviours and it seems to work. Thanks for all the fruitful discussion, I'll now close that issue. |
Description
I have two packages A and B and the package A has a dependency on B.
My dependency file looks like
With paket 4.8.8 the paket lock file looks like
But now with paket 5.2.5 I get the following output:
Repro steps
Use the Repro.zip
.paket\paket.bootstrapper.exe
.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
The text was updated successfully, but these errors were encountered: