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

Merged
merged 11 commits into from May 2, 2017

Conversation

Projects
None yet
4 participants
@matthid
Member

matthid commented Apr 30, 2017

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)

@matthid matthid requested a review from cloudRoutine Apr 30, 2017

Show outdated Hide outdated src/Paket.Core/PaketConfigFiles/DependenciesFile.fs
@@ -229,6 +229,7 @@ type DependenciesFile(fileName,groups:Map<GroupName,DependenciesGroup>, textRepr
RemoteDownload.DownloadSourceFiles(Path.GetDirectoryName fileName, groupName, force, remoteFiles)
// Step 1 Package resolution

This comment has been minimized.

@cloudRoutine

cloudRoutine Apr 30, 2017

Member

let's call this something other than step 1 & step 2, it's already confusing enough with all the other steps

@cloudRoutine

cloudRoutine Apr 30, 2017

Member

let's call this something other than step 1 & step 2, it's already confusing enough with all the other steps

This comment has been minimized.

@matthid

matthid Apr 30, 2017

Member

Suggestions?

@matthid

matthid Apr 30, 2017

Member

Suggestions?

| Some d -> d.Settings
| None -> p.Settings })
|> fun des -> Seq.append des runtimeDeps
|> Seq.distinctBy (fun p -> p.Name) |> Set.ofSeq

This comment has been minimized.

@matthid

matthid Apr 30, 2017

Member

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...

@matthid

matthid Apr 30, 2017

Member

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...

Show outdated Hide outdated src/Paket/Paket.fsproj
<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>

This comment has been minimized.

@cloudRoutine

cloudRoutine Apr 30, 2017

Member

remove the detritus

@cloudRoutine

cloudRoutine Apr 30, 2017

Member

remove the detritus

@cloudRoutine

This comment has been minimized.

Show comment
Hide comment
@cloudRoutine

cloudRoutine Apr 30, 2017

Member

aside from some superficial cleanup LGTM

Member

cloudRoutine commented Apr 30, 2017

aside from some superficial cleanup LGTM

matthid added some commits Apr 30, 2017

cleanup + add PAKET_DISABLE_RUNTIME_RESOLUTION to disable runtime res…
…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)

This comment has been minimized.

@matthid

matthid Apr 30, 2017

Member

@cloudRoutine what about this?

@matthid

matthid Apr 30, 2017

Member

@cloudRoutine what about this?

@matthid matthid changed the title from Fix resolver complicated pinning to Fix pinning of packages not properly handled by the resolver Apr 30, 2017

@forki forki merged commit 8765ffa into fsprojects:master May 2, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment