Fixing dependency versions when required versions differ #559
Conversation
| { | ||
| foreach (var dependency in set.Dependencies) | ||
| { | ||
| if (dependency.Id == id && dependency.VersionSpec.MinVersion < version) |
There was a problem hiding this comment.
the id compare should be case-insensitive
There was a problem hiding this comment.
The version check should use Satisfies, this isn't taking into account >= or if the min version is included in the range
|
If ProjectFactory is still using NuGet.Core.dll it should just go away. |
|
⌚ |
|
Pushed another version - this one gets rid of the Walker and just tries to figure it out on it's own. I didn't use Satisfies because it doesn't cover the case of a higher minimum. |
| { | ||
| found = true; | ||
|
|
||
| if (dependency.VersionSpec.MinVersion < projectPackage.Version || |
There was a problem hiding this comment.
does this work if MinVersion is null, I see the IsMinInclusive check afterwards
|
Does this need to go through the framework groups individually? It seems like it's possible to filter too much like this. VersionSpec is from nuget.core.dll is there any reason the dependencies come out of package builder as the old types? |
| { | ||
| // returns true if the dependency should be added to the package | ||
| // This happens if the dependency is not a dependency of a dependecy | ||
| // Or if the project dependency version is > the dependency's dependency version |
There was a problem hiding this comment.
Is this logic a bit too simplistic? Isn't what we really care about what version would actually get installed based on the requested version? For example, if a dependency A requests dependency B of version >=1.0.11, but there is no 1.0.11 we might get 1.0.12. In that scenario, if version 1.0.12 is what we currently have, then we don't need to include that dependency in the minimal set (even though it > the specified version).
There was a problem hiding this comment.
That's a tricky one, since feeds can change we should probably assume that the minimum version does exist somewhere and not try to fit the dependencies to the current state of the feeds.
|
I've pulled out a bunch of NuGet.Core references from ProjectFactory but with the same behavior. To address a question above, packing csproj projects only build for one target framework so there is no need to go through them individually. |
| var dependency = packagesAndDependencies[package.Id].Item2; | ||
| dependencies[dependency.Id] = new PackageDependency(dependency.Id, VersionRange.Parse(dependency.VersionSpec.ToString())); | ||
| var dependency = packagesAndDependencies[package.GetIdentity().Id].Item2; | ||
| dependencies[dependency.Id] = new PackageDependency(dependency.Id, VersionRange.Parse(dependency.VersionRange.ToString())); |
There was a problem hiding this comment.
PackageDependency should be immutable, does this need to make a copy of it?
|
⌚ |
|
|
Fixes NuGet/Home#759
@emgarten
Fixing pack so that dependencies are included with the indirectly referenced version is lower than the project's referenced version.