Skip to content

Commit

Permalink
Fix ordering of BuildPlanCheck results
Browse files Browse the repository at this point in the history
The comment said that GT was better, but for partial and failing
snapshots, LT was returned for better results. The snapshot comparison
logic was using <=, so this worked for partial and failing snapshots,
but not BuildPlanCheckOk.
  • Loading branch information
mgsloan committed Jan 27, 2016
1 parent 3dd473b commit 8703722
Showing 1 changed file with 4 additions and 3 deletions.
7 changes: 4 additions & 3 deletions src/Stack/BuildPlan.hs
Original file line number Diff line number Diff line change
Expand Up @@ -658,10 +658,11 @@ data BuildPlanCheck =
-- | Compare 'BuildPlanCheck', where GT means a better plan.
compareBuildPlanCheck :: BuildPlanCheck -> BuildPlanCheck -> Ordering
compareBuildPlanCheck (BuildPlanCheckPartial _ e1) (BuildPlanCheckPartial _ e2) =
compare (Map.size e1) (Map.size e2)
-- Note: order of comparison flipped, since it's better to have fewer errors.
compare (Map.size e2) (Map.size e1)
compareBuildPlanCheck (BuildPlanCheckFail _ e1 _) (BuildPlanCheckFail _ e2 _) =
let numUserPkgs e = Map.size $ Map.unions (Map.elems (fmap deNeededBy e))
in compare (numUserPkgs e1) (numUserPkgs e2)
in compare (numUserPkgs e2) (numUserPkgs e1)
compareBuildPlanCheck BuildPlanCheckOk{} BuildPlanCheckOk{} = EQ
compareBuildPlanCheck BuildPlanCheckOk{} BuildPlanCheckPartial{} = GT
compareBuildPlanCheck BuildPlanCheckOk{} BuildPlanCheckFail{} = GT
Expand Down Expand Up @@ -738,7 +739,7 @@ selectBestSnapshot gpds snaps = do
Just old -> loop (Just (betterSnap old new)) rest

betterSnap (s1, r1) (s2, r2)
| compareBuildPlanCheck r1 r2 /= GT = (s1, r1)
| compareBuildPlanCheck r1 r2 /= LT = (s1, r1)
| otherwise = (s2, r2)

reportResult BuildPlanCheckOk {} snap = do
Expand Down

2 comments on commit 8703722

@mgsloan
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@harendra-kumar Is this correct?

@harendra-kumar
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comparison was wrong but it worked because we never compared Ok with Fail or Partial. We break out of the loop as soon as we find an Ok snapshot. The code is consistent now. Thanks!

Please sign in to comment.