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

PackageResolver Refactor #2039

Merged
merged 4 commits into from Nov 25, 2016

Conversation

Projects
None yet
2 participants
@cloudRoutine
Member

cloudRoutine commented Nov 23, 2016

Started working on this in an attempt to address #2030

I wasn't able to get the step function to the point where its tail recursive, but its much closer than it was before and it no longer relies on mutation across iterations.

I think it should be possible by adding a third case to

type private Stage =
    | Outer of conflictState : ConflictState
    | Inner of conflictState : ConflictState

and using it as an argument for step to direct it down one of three pathways for the start of a step, an outer loop, or an inner loop, although when I tried coding that up I couldn't get it to work.

I think this is the block of code that was causing the issues when I took that approach

                    match conflictState.Status with
                    | Resolution.Conflict(_,conflicts,_,_)
                        when
                            (Set.isEmpty conflicts |> not)
                            && nextStep.CurrentResolution.Count > 1
                            && not (conflicts |> Set.exists (fun r ->
                                r = currentRequirement
                                || r.Graph |> List.contains currentRequirement)) ->
                        let flags = StepFlags(flags.Ready,flags.UseUnlisted,flags.HasUnlisted,true,flags.FirstTrial)
                        stepLoop flags (Inner (conflictState))
                    | _ ->
                        stepLoop flags (Inner (conflictState))

but I'm not totally sure.

Show outdated Hide outdated src/Paket.Core/PackageResolver.fs
currentStep.ClosedRequirements
|> Set.union currentStep.OpenRequirements
|> Set.add lastPackageRequirement
|> Seq.filter (fun x -> x.Name = lastPackageRequirement.Name)
|> Seq.sortBy (fun x -> x.Parent)
|> Seq.filter ^ fun x -> x.Name = lastPackageRequirement.Name

This comment has been minimized.

@forki

forki Nov 23, 2016

Member

please don't

@forki

forki Nov 23, 2016

Member

please don't

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Nov 23, 2016

Member

this is really really hard to review- can you please split it in two PRs. one with the resolver change and one with all the cosmetical changes? thanks

Member

forki commented Nov 23, 2016

this is really really hard to review- can you please split it in two PRs. one with the resolver change and one with all the cosmetical changes? thanks

@cloudRoutine

This comment has been minimized.

Show comment
Hide comment
@cloudRoutine

cloudRoutine Nov 23, 2016

Member

@forki this fine?

Member

cloudRoutine commented Nov 23, 2016

@forki this fine?

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Nov 23, 2016

Member

it seems the new resolver is not resolving correctly tests like ##1254 are red. can you confirm?

Member

forki commented Nov 23, 2016

it seems the new resolver is not resolving correctly tests like ##1254 are red. can you confirm?

@cloudRoutine

This comment has been minimized.

Show comment
Hide comment
@cloudRoutine

cloudRoutine Nov 23, 2016

Member

a bug got reintroduced when I redid the commits

Member

cloudRoutine commented Nov 23, 2016

a bug got reintroduced when I redid the commits

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Nov 23, 2016

Member

#1883 install FSharp.Core from Chessie fails!?

Member

forki commented Nov 23, 2016

#1883 install FSharp.Core from Chessie fails!?

@cloudRoutine

This comment has been minimized.

Show comment
Hide comment
@cloudRoutine

cloudRoutine Nov 23, 2016

Member

I've been having issues running the integration tests on my machine


I know I can move the dir, but maybe the integration tests don't need to pull in so many netcore packages?

Member

cloudRoutine commented Nov 23, 2016

I've been having issues running the integration tests on my machine


I know I can move the dir, but maybe the integration tests don't need to pull in so many netcore packages?

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Nov 23, 2016

Member

yeah these new packages are fucking up everything. MS is clearly out of control here.

Member

forki commented Nov 23, 2016

yeah these new packages are fucking up everything. MS is clearly out of control here.

@cloudRoutine

This comment has been minimized.

Show comment
Hide comment
Member

cloudRoutine commented Nov 23, 2016

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Nov 23, 2016

Member

do you have VS open at that time? or something else that keeps an handle on the nupkg?

Member

forki commented Nov 23, 2016

do you have VS open at that time? or something else that keeps an handle on the nupkg?

@cloudRoutine

This comment has been minimized.

Show comment
Hide comment
@cloudRoutine

cloudRoutine Nov 23, 2016

Member

I had all of that stuff closed at the time. The first time I got those errors I thought that might be the source but when I tried again with everything off it still happened.

I get these errors when I run the tests just in VS too

Member

cloudRoutine commented Nov 23, 2016

I had all of that stuff closed at the time. The first time I got those errors I thought that might be the source but when I tried again with everything off it still happened.

I get these errors when I run the tests just in VS too

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Nov 23, 2016

Member

And from fake build?

Am 23.11.2016 16:50 schrieb "Jared Hester" notifications@github.com:

I had all of that stuff closed at the time. The first time I got those
errors I thought that might be the source but when I tried again with
everything off it still happened.

I get these errors when I run the tests just in VS too

https://camo.githubusercontent.com/941f9899d827833319c461cb4c1171afb00f673f/687474703a2f2f692e696d6775722e636f6d2f50717949665a442e706e67


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#2039 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AADgNDMhgrvmLy6PeGchRuyJk5e8gBuDks5rBGDQgaJpZM4K6Nx6
.

Member

forki commented Nov 23, 2016

And from fake build?

Am 23.11.2016 16:50 schrieb "Jared Hester" notifications@github.com:

I had all of that stuff closed at the time. The first time I got those
errors I thought that might be the source but when I tried again with
everything off it still happened.

I get these errors when I run the tests just in VS too

https://camo.githubusercontent.com/941f9899d827833319c461cb4c1171afb00f673f/687474703a2f2f692e696d6775722e636f6d2f50717949665a442e706e67


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#2039 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AADgNDMhgrvmLy6PeGchRuyJk5e8gBuDks5rBGDQgaJpZM4K6Nx6
.

@cloudRoutine

This comment has been minimized.

Show comment
Hide comment
@cloudRoutine

cloudRoutine Nov 23, 2016

Member

the log in the gist is from the FAKE build

Member

cloudRoutine commented Nov 23, 2016

the log in the gist is from the FAKE build

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Nov 23, 2016

Member

Ok can you please try to find out which tool is locking it? Sorry for all the work, but this whole .net standard conversion is a real mess.

Member

forki commented Nov 23, 2016

Ok can you please try to find out which tool is locking it? Sorry for all the work, but this whole .net standard conversion is a real mess.

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Nov 23, 2016

Member

#1977 seems to work on appveyor. (travis is expected to fail)

Member

forki commented Nov 23, 2016

#1977 seems to work on appveyor. (travis is expected to fail)

@cloudRoutine

This comment has been minimized.

Show comment
Hide comment
@cloudRoutine

cloudRoutine Nov 23, 2016

Member

The readme might need a note to install F# 3.0, without it #144 will fail with

But even after I installed F# 3.0

Member

cloudRoutine commented Nov 23, 2016

The readme might need a note to install F# 3.0, without it #144 will fail with

But even after I installed F# 3.0

@cloudRoutine

This comment has been minimized.

Show comment
Hide comment
@cloudRoutine

cloudRoutine Nov 23, 2016

Member

ok it seems like the error is coming from netstandard getting installed with chessie
when I run the contents of test #1883 in FSI

Member

cloudRoutine commented Nov 23, 2016

ok it seems like the error is coming from netstandard getting installed with chessie
when I run the contents of test #1883 in FSI

@cloudRoutine

This comment has been minimized.

Show comment
Hide comment
@cloudRoutine

cloudRoutine Nov 23, 2016

Member

A more detailed output

Member

cloudRoutine commented Nov 23, 2016

A more detailed output

let flags = StepFlags(flags.Ready,flags.UseUnlisted,hasUnlisted,flags.ForceBreak,flags.FirstTrial)
if exploredPackage.Unlisted && not flags.UseUnlisted then
tracefn " unlisted"

This comment has been minimized.

@cloudRoutine

cloudRoutine Nov 23, 2016

Member

now i'm pretty sure the bug is here, where I last made a change :facepalm:
which I'd had as

                    if exploredPackage.Unlisted && not flags.UseUnlisted then
                        tracefn "     unlisted"
                        stepLoop flags (Inner(conflictState))
                    else

before, but that caused an infinite loop printing unlisted

@forki it'd be really useful for whoever has to work on this next, if you or someone else with a deeper understanding of what's going on in this module could add some comments explaining the processes. I'd do it myself, but I don't think I have a firm enough grasp and I wouldn't want to put down something that's wrong

@cloudRoutine

cloudRoutine Nov 23, 2016

Member

now i'm pretty sure the bug is here, where I last made a change :facepalm:
which I'd had as

                    if exploredPackage.Unlisted && not flags.UseUnlisted then
                        tracefn "     unlisted"
                        stepLoop flags (Inner(conflictState))
                    else

before, but that caused an infinite loop printing unlisted

@forki it'd be really useful for whoever has to work on this next, if you or someone else with a deeper understanding of what's going on in this module could add some comments explaining the processes. I'd do it myself, but I don't think I have a firm enough grasp and I wouldn't want to put down something that's wrong

@cloudRoutine

This comment has been minimized.

Show comment
Hide comment
@cloudRoutine

cloudRoutine Nov 23, 2016

Member

@forki finally passed on appveyor 🎉

Member

cloudRoutine commented Nov 23, 2016

@forki finally passed on appveyor 🎉

@cloudRoutine

This comment has been minimized.

Show comment
Hide comment
@cloudRoutine

cloudRoutine Nov 24, 2016

Member

anything else I need to do for this?

Member

cloudRoutine commented Nov 24, 2016

anything else I need to do for this?

@forki forki merged commit e144fd8 into fsprojects:core3 Nov 25, 2016

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Nov 25, 2016

Member

thanks so much. let's what the real world thinks about it ;-)

Member

forki commented Nov 25, 2016

thanks so much. let's what the real world thinks about it ;-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment