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

Add (optional) tracetree support into the cabal executable #3410

Merged
merged 2 commits into from May 10, 2016

Conversation

Projects
None yet
7 participants
@edsko
Copy link
Contributor

edsko commented May 7, 2016

This PR consists of two commits:

  • The first just contains a bunch of additional solver tests that I've used to write the (so far, draft) blog post about backjumping in the solver
  • The second add a new debug-tracetree configure flag. If enabled, it adds a dependency on my tracetree library, and has the solver spit out the various build trees in .JSON format. This is useful only for debugging, and we don't want to spit these out for very large search trees (i.e., most of the them), but it's nonetheless very helpful when we are debugging the solver or explain how the solver works. I used this to visualize the search trees in the previous blog post above the solver, but the tracetree library wasn't really mature enough to be released to Hackage. That is changed now, so having this available in the master branch (even if conditionally enabled by a configure flag) might be useful to other people working on the solver.

/cc @kosmikus , @grayjay .

GSimpleTree a =>
#endif
FilePath -- ^ Output file
-> (a -> a) -- ^ Function to summarize the tree before dumping

This comment has been minimized.

@23Skidoo

23Skidoo May 7, 2016

Member

Looks like you're only calling traceTree with id, so in principle you could get rid of this parameter.

-- | Replace all goal reasons with a dummy goal reason in the tree
--
-- This is useful for debugging (when experimenting with the impact of GRs)
_removeGR :: Tree QGoalReason -> Tree QGoalReason

This comment has been minimized.

@23Skidoo

23Skidoo May 7, 2016

Member

If this is not exported and not used anywhere, why are we not getting a Travis failure due to -Werror?

This comment has been minimized.

@23Skidoo

23Skidoo May 7, 2016

Member

OK, I get it, the underscore. Didn't realise it had the same effect at top level.

@23Skidoo

This comment has been minimized.

Copy link
Member

23Skidoo commented May 7, 2016

LGTM.

@23Skidoo 23Skidoo added this to the cabal-install 1.26 milestone May 7, 2016

@dcoutts

This comment has been minimized.

Copy link
Member

dcoutts commented May 7, 2016

LGTM too.

I don't know about the traceTree id param either. Is there some reason @edsko ?

@23Skidoo

This comment has been minimized.

Copy link
Member

23Skidoo commented May 7, 2016

My guess is that @edsko uses traceTree _removeGR in REPL.

@grayjay

This comment has been minimized.

Copy link
Collaborator

grayjay commented May 8, 2016

👍 This looks very useful. I had a hard time building the ttrender executable, though. I ended up lifting tracetree's upper bounds on bifunctors and optparse-applicative.

@kosmikus

This comment has been minimized.

Copy link
Contributor

kosmikus commented May 9, 2016

Yes, looks good.

@BardurArantsson

This comment has been minimized.

Copy link
Collaborator

BardurArantsson commented May 10, 2016

Would it not be preferable to avoid the CPP and just write the JSON directly? (I'm assuming the CPP is mostly to avoid the dependency in "production" builds.) Is that a large amount of work?

I'm mostly concerned about adding yet another code path to the Solver portion of cabal-install and adding large dependencies (even if behind a flag) which will make it hard to extricate at a later point.

@23Skidoo 23Skidoo merged commit 61c1d3d into haskell:master May 10, 2016

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@23Skidoo

This comment has been minimized.

Copy link
Member

23Skidoo commented May 10, 2016

Merged, thanks!

@edsko

This comment has been minimized.

Copy link
Contributor

edsko commented May 14, 2016

@BardurArantsson The CPP only happens at the very top-level (and yes, it's to avoid the dependency). I suppose we could write the JSON by hand but it would be kind of ugly; the tracetree library makes this patch rather small. A slightly better alternative to writing it by hand would be to use the canonical JSON representation offered by hackage-security, on which we now have a dependency anyway; but of course, if the goal is to have the solver "extractable" then probably that solver will eventually not have a dependency on hackage-security so I guess that doesn't help much. Either way, if we do extract this, then probably we want to extract this tracetree stuff with it, and if not, it will be easy to remove.

@edsko edsko deleted the edsko:pr/tracetree branch May 14, 2016

@edsko

This comment has been minimized.

Copy link
Contributor

edsko commented May 14, 2016

@grayjay I took a look at the upper bounds issue you mentioned; I've lifted the bounds on the two packages that you mentioned (they were the only ones that needed lifting, I think), but even then cabal still didn't find a solution; I had to manually specify the versions of some packages:

cabal install \
  -f ttrender \
  --only-dependencies \
  --constraint diagrams-lib==1.3.1.2 \
  --constraint semigroups==0.18.1 \
  --constraint linear==1.20.4

Not sure what's going on here; the versions I specified on the command line are all the latest, so I don't know why it wouldn't find those itself (--reorder-goals also did the trick, although I didn't check if it found the same solution).

@grayjay

This comment has been minimized.

Copy link
Collaborator

grayjay commented May 14, 2016

@edsko I first tried building cabal-install with both ttrender and debug-tracetree set to true, starting with the packages that come with GHC 7.10.3. That didn't work, because cairo's setup script doesn't compile with the latest Cabal. I tried to work around it by adding a custom setup with a constraint, but that prevented the solver from finding a solution quickly. One problem was that linear has a restrictive upper bound on transformers-compat. Then I built just tracetree +ttrender using Nixpkgs. I had to lift tracetree's upper bounds to work with those versions.

@ezyang ezyang modified the milestones: cabal-install 2.0, 2.0 (planned for next feature release) Sep 6, 2016

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