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 --external flag to `stack dot` #437

Merged
merged 3 commits into from Jul 1, 2015

Conversation

Projects
None yet
4 participants
@markus1189
Contributor

markus1189 commented Jun 27, 2015

When given --external, stack dot will include external dependencies.

By default it keeps the old behavior and only includes local packages and their interdependencies.
If --external is activated local packages will have the "dashed" line attribute so that one can distinguish local packages from external dependencies.

As an example, given a tree like this with two projects one and two:

├── [4.0K]  one
│   ├── [ 686]  one.cabal
│   └── [  46]  Setup.hs
├── [  78]  stack.yaml
└── [4.0K]  two
    ├── [  46]  Setup.hs
    └── [ 710]  two.cabal

stack dot will produce:
foo

while stack dot --external will produce:
foo

This would also fix #431.

@rvion

This comment has been minimized.

Contributor

rvion commented Jun 27, 2015

as discussed in #431, should we make --external the default (without command option) and come up with an other option name (like --local-only) for current behaviour ?

anyway, that's quite minor.
thanks @markus1189

@snoyberg

This comment has been minimized.

Contributor

snoyberg commented Jun 27, 2015

I'd rather have the current behavior as default, with flags to force it either direction. Then we can change the default in the future and people can keep backwards compatibility easily. Let me review this patch. I can make the addition of a --no-external or equivalent myself after review unless @markus1189 beats me to it :).

@snoyberg

This comment has been minimized.

Contributor

snoyberg commented Jun 27, 2015

I don't think this is working as intended: it currently only goes one level deep. For example, when run on the stack repo, it tells you all of the packages that stack depends on, but not the packages that they depend on as well. Am I correct in understanding that that was the desired behavior?

@snoyberg snoyberg added this to the 0.2.0.0 milestone Jun 27, 2015

@markus1189

This comment has been minimized.

Contributor

markus1189 commented Jun 27, 2015

I did not find an easy way to resolve the PackageName into something we can use to extract further dependencies without changing even more of the original version. So currently the intended behavior is that only one level is shown.

But I agree that it would be very cool to get a full dependency graph generated. Maybe that could be done as an extension though I don't know how much work it will be.

@snoyberg

This comment has been minimized.

Contributor

snoyberg commented Jun 27, 2015

I can probably either add that myself tomorrow or, if you'd prefer, add information to this issue explaining where to get the missing information.

@markus1189

This comment has been minimized.

Contributor

markus1189 commented Jun 27, 2015

If you want to explain it that's great because then I can give it a shot
myself :)
On Jun 27, 2015 11:24 PM, "Michael Snoyman" notifications@github.com
wrote:

I can probably either add that myself tomorrow or, if you'd prefer, add
information to this issue explaining where to get the missing information.


Reply to this email directly or view it on GitHub
#437 (comment)
.

@snoyberg

This comment has been minimized.

Contributor

snoyberg commented Jun 27, 2015

OK, I'll give more details tomorrow when I'm at my computer instead of on my phone. Probably the Stack.Build.Sources module is the right place to start.

@snoyberg

This comment has been minimized.

Contributor

snoyberg commented Jun 28, 2015

Alright, a bit more information:

  • loadSourceMap will give you a Map from PackageName to some information on package in question
  • For local packages, you can just grab the Package value with lpPackage like the code does now
  • For non-local packages, you'll want to use withLoadPackage in Stack.Build.

I hope that helps, let me know if you have any questions

@markus1189

This comment has been minimized.

Contributor

markus1189 commented Jun 28, 2015

Yes thanks that definitely helps. I think I will have something working soon, I'll let you know.

@markus1189 markus1189 force-pushed the markus1189:dot-command branch from c796c5b to 22f08dc Jun 29, 2015

@markus1189

This comment has been minimized.

Contributor

markus1189 commented Jun 29, 2015

Okay so I got something working.
I added two more flags, --[no-]include-base and --depth DEPTH

Usage: stack dot ([--external] | [--no-external]) ([--include-base] |
                 [--no-include-base]) [--depth DEPTH]
  Visualize your project's dependency graph using Graphviz dot

Available options:
  --external               Enable inclusion of external dependencies
  --no-external            Disable inclusion of external dependencies
  --include-base           Enable inclusion of dependencies on base
  --no-include-base        Disable inclusion of dependencies on base
  --depth DEPTH            Limit the depth of dependency resolution (Default: No
                           limit)

The reason for --include-base is that most (all?) dependencies will have a edge to base which makes the graph harder to read.

In addition --depth N limits the depth of the shown dependencies, as it will become very big for larger projects.

As an example take two cabal projects one and two

Dependencies for one:

build-depends:       base >=4.8 && <4.9, free

and for two

build-depends:       base >=4.8 && <4.9, free, mtl, transformers, one

stack dot gives
default

stack dot --external --depth 0 --no-include-base gives
ext0

stack dot --external --depth 1 --no-include-base gives
ext1

stack dot --external --depth 2 --no-include-base gives
onetwo

As an example why --no-include-base is handy, compare
stack dot --external --include-base
with_base
with stack dot --external --no-include-base
without_base

However I noticed that for packages that come with ghc it seems like I have to do something different to get the dependencies, as they are not in the SourceMap, is that correct?
That would be something I would like to change before this can be merged in addition to some tests so this is only a preview :)

@snoyberg

This comment has been minimized.

Contributor

snoyberg commented Jun 29, 2015

This is looking awesome. On my phone so I can't review properly, I'll provide feedback later.

@markus1189 markus1189 force-pushed the markus1189:dot-command branch from 22f08dc to 639addf Jun 29, 2015

@snoyberg

This comment has been minimized.

Contributor

snoyberg commented Jun 29, 2015

It looks like this doesn't build with GHC 7.8 due to lack of Applicative => Monad.

@markus1189 markus1189 force-pushed the markus1189:dot-command branch from 639addf to a9864e8 Jun 29, 2015

@markus1189

This comment has been minimized.

Contributor

markus1189 commented Jun 29, 2015

Yeah that trips me up every time because I currently only have ghc 7.10 installed :)

@snoyberg

This comment has been minimized.

Contributor

snoyberg commented Jun 29, 2015

You could just use stack setup ;)

@markus1189

This comment has been minimized.

Contributor

markus1189 commented Jun 29, 2015

Did I mention that my internet is SUPER slow? About 30 kB/s is the normal rate :P

@@ -87,7 +88,13 @@ mkBaseConfigOpts bopts = do
}

-- | Provide a function for loading package information from the package index
withLoadPackage :: M env m
withLoadPackage :: ( MonadIO m

This comment has been minimized.

@snoyberg

snoyberg Jun 29, 2015

Contributor

👍

@snoyberg

This comment has been minimized.

Contributor

snoyberg commented Jun 29, 2015

Ouch, I think I'd go crazy at that rate :(

The patch looks good, barring the 7.8 issues I'd be happy to merge it. I don't mind fixing those up myself, would you like me to?

And thanks again for the contributions, not sure if you noticed, but:

Achievement Unlocked: Commit Bit

@markus1189

This comment has been minimized.

Contributor

markus1189 commented Jun 29, 2015

The 7.8 issue should be fixed. There is still a problem with packages included with ghc, like template-haskell, ghc-prim and others. I guess I have to use something else to get those dependencies, any ideas?

And I would like to add some tests for the graph building before this is merged.

@snoyberg

This comment has been minimized.

Contributor

snoyberg commented Jun 29, 2015

I'm not sure how useful it is to include dependencies of wired-in packages (those that are tied into the compiler), since their dependencies are somewhat irrelevant (they can never be reinstalled). I'd say showing the ghc package as a leaf is a good call.

I'll hold off on the merge then.

@markus1189

This comment has been minimized.

Contributor

markus1189 commented Jun 29, 2015

Is there an easy way to check whether it is wired-in? Or is checking for an empty dependency set enough?

@snoyberg

This comment has been minimized.

Contributor

snoyberg commented Jun 29, 2015

It's not exactly the same thing, but I'd just trust that empty dependency set means it should be displayed as a leaf. This will apply to some non-wired-in packages that are in the global database as well if I'm not mistaken.

But to answer your question much more directly: look at Stack.Constraints.wiredInPackages.

Simplify and export `Stack.Build.withLoadPackage`
- replace type alias `M` with minimal set of required constraints

@markus1189 markus1189 force-pushed the markus1189:dot-command branch 4 times, most recently from d0aa685 to 98ea4de Jun 30, 2015

markus1189 added some commits Jun 29, 2015

Add external dependency visualization to stack dot
- new flag --[no-]external to include external dependencies
- new flag --[no-]include-base to toggle edges to base package
- new flag --depth to limit depth of external dependencies shown

@markus1189 markus1189 force-pushed the markus1189:dot-command branch from 98ea4de to 6b4ebf9 Jul 1, 2015

@markus1189

This comment has been minimized.

Contributor

markus1189 commented Jul 1, 2015

Okay I think this can be merged. Packages from Stack.Constants.wiredInPackages are drawn as rectangle nodes, though there are more leaf nodes if they are in the global package db I think.
I chose not to add a "ghc" node because the graph already is complicated enough for large projects and adding this pseudo node just hinders readability.

I also added some tests although not as many as I would like to have because for that I would need to also create LocalPackages which is a little unwieldy manually :)

I split the pr into 3 commits but if you want I can also just squash them into one.

Best,
Markus

mboes added a commit that referenced this pull request Jul 1, 2015

Merge pull request #437 from markus1189/dot-command
Add --external flag to `stack dot`

@mboes mboes merged commit bf086c6 into commercialhaskell:master Jul 1, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@mboes

This comment has been minimized.

Contributor

mboes commented Jul 1, 2015

Thanks!

@markus1189 markus1189 deleted the markus1189:dot-command branch Jul 1, 2015

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