-
Notifications
You must be signed in to change notification settings - Fork 520
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
Fix pinning of packages not properly handled by the resolver #2307
Fix pinning of packages not properly handled by the resolver #2307
Conversation
…ndencies as well.
… to the resolution.
@@ -229,6 229,7 @@ type DependenciesFile(fileName,groups:Map<GroupName,DependenciesGroup>, textRepr | |||
RemoteDownload.DownloadSourceFiles(Path.GetDirectoryName fileName, groupName, force, remoteFiles) | |||
|
|||
// Step 1 Package resolution |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's call this something other than step 1
& step 2
, it's already confusing enough with all the other steps
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestions?
| Some d -> d.Settings | ||
| None -> p.Settings }) | ||
|> fun des -> Seq.append des runtimeDeps | ||
|> Seq.distinctBy (fun p -> p.Name) |> Set.ofSeq |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This part seems to be quite slow as it takes quite a while after Trying to find a valid resolution considering runtime dependencies...
has been printed until the second resolution starts...
src/Paket/Paket.fsproj
Outdated
<StartArguments>install</StartArguments> | ||
<StartWorkingDirectory>C:\PROJ\Paket\integrationtests\scenarios\i001145-excludes\temp</StartWorkingDirectory> | ||
<StartArguments>update</StartArguments> | ||
<StartWorkingDirectory>C:\PROJ\Paket\integrationtests\scenarios\i002294-pin-netstandard-1-6\temp</StartWorkingDirectory> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove the detritus
aside from some superficial cleanup LGTM |
…olution (for example to workaround problems or compare performance of the resolution algorithm with older versions of paket)
@@ -693,7 713,7 @@ let Resolve (getVersionsF, getPackageDetailsF, groupName:GroupName, globalStrate | |||
currentStep.CurrentResolution.Count | |||
(currentStep.CurrentResolution |> Seq.map (fun x -> sprintf "\n - %O, %O" x.Key x.Value.Version) |> String.Concat) | |||
currentStep.OpenRequirements.Count | |||
(currentStep.OpenRequirements |> Seq.map (fun x -> sprintf "\n - %O, %O" x.Parent x.VersionRequirement) |> String.Concat) | |||
(currentStep.OpenRequirements |> Seq.map (fun x -> sprintf "\n - %O, %O (from %O)" x.Name x.VersionRequirement x.Parent) |> String.Concat) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cloudRoutine what about this?
I got started by the test of @0x53A, therefore this was done on top of his branch and contains #2305.
Fixes #2294
Edit: Its actually OK to have this in the same PR because #2294 was the underlying issue the runtime solution didn't work out. So this actually first allows the workaround to "work" by applying the correct settings to the runtime resolution. At the same time it fixes #2294 itself (therefore a separate PR)