Skip to content

Conversation

@eed3si9n
Copy link
Member

Ref sbt/sbt-dependency-graph#178

This in-sources sbt-dependency-graph so it can evolve together with sbt.

@dwijnand suggested:

I don't think it's worth the breaking change of changing groupId and package name. I think it's best to keep those things as is, and see if it can be merged into sbt/sbt.

To which I agreed:

@jrudolph Thanks for all your work. I've be happy to help transition sbt-dependency-graph to sbt organization, and bring the code base into sbt/sbt.

mdr and others added 30 commits June 26, 2012 08:54
Conflicts:
	build.sbt
@lightbend-cla-validator

This comment has been minimized.

@eed3si9n
Copy link
Member Author

We can dismiss that since the commits related to "com.github.mdr" %% "ascii-graphs" by mdr are no longer relevant - https://github.com/sbt/sbt-dependency-graph/commits?author=mdr.

@eed3si9n eed3si9n merged commit 602cf39 into sbt:develop Sep 22, 2020
@eed3si9n eed3si9n deleted the wip/dependencygraph branch September 22, 2020 02:56
@eatkins
Copy link
Contributor

eatkins commented Sep 22, 2020

Is there any chance we can turn this off by default? It adds a ton of settings to the build and slows down loading. It is super valuable when it's needed, but 99% of the time, it isn't used.

@eed3si9n
Copy link
Member Author

Not sure if there's a great way for turning plugins on or off. Metabuild gets cached so system properties do not always work.

@eatkins
Copy link
Contributor

eatkins commented Sep 22, 2020

I was thinking putting it in it’s own project and requiring users to add it to manually to their plugins.sbt.

@eed3si9n
Copy link
Member Author

Similar to inspect tree, I think dependencyTree is a useful enough task to be added to everyone's build out of box.
I'd be open to maybe splitting it up the plugin so some of the less used feature becomes an opt-in?

@dwijnand
Copy link
Member

I got the impression (but it was a while ago) that sbt-dependency-graph had some of its own data structures (behind keys) in an attempt to rationalise and perhaps somewhat protect from upstream (sbt/sbt) changes. Perhaps some of those can be culled down, integrating into the existing keys.

Also worth looking if any keys are needlessly in project-scope.

@eed3si9n
Copy link
Member Author

The keys are here https://github.com/sbt/sbt/blob/54747b88bb23dea7e59f296032a70b23c9aae14b/main/src/main/scala/sbt/internal/graph/DependencyGraphKeys.scala - 19 tasks and 8 settings. There's also some in-task scoping called asString and toFile which adds a few more.

The tasks supporting different output formats Ascii art, GraphML, DOT, and HTML take up majority of them.
There are also other features included like: ivyReport, dependencyList, dependencyStats, and whatDependsOn.

dependencyTreeIncludeScalaLibrary is already globally scoped.

@eatkins
Copy link
Contributor

eatkins commented Sep 22, 2020

In the akka project, after this change, the number of settings on my computer increased from 46483 to 54764. The average time to load the build increased from about 11.5 seconds to 12.25 seconds. The reason I care is that in eight years of using sbt, I think I've needed this functionality maybe 3 times. Making this a built-in autoplugin adds to the cold startup time for all users without providing any material benefit for the vast majority of users and there is no way to opt out. Even ignoring the cold startup time, it adds settings bloat to sbt which already has, in my opinion, too many settings and tasks visible in most projects which impacts tab completions and help.

Again, I am not disputing the value of the plugin. I support in-sourcing the plugin 100%. But I would be much happier with this change if it were an official, but standalone, plugin.

@eed3si9n
Copy link
Member Author

Let's say Akka has 40 subprojects and this adds 30 tasks. That's 1200. Where's another factor of x7 coming from to make it 8281? Streams?

@eatkins
Copy link
Contributor

eatkins commented Sep 22, 2020

I have no idea but in general setting counts seem to multiply much faster than I would naively expect across the board.

@eed3si9n
Copy link
Member Author

Found it

def reportSettings =
Seq(Compile, Test, IntegrationTest, Runtime, Provided, Optional).flatMap(ivyReportForConfig)

It's flatmapping across all configurations.

@dwijnand
Copy link
Member

In the akka project, after this change, the number of settings on my computer increased from 46483 to 54764. The average time to load the build increased from about 11.5 seconds to 12.25 seconds.

Sorry, but didn't we add a whole bunch of settings for watch/Glob things? How many and how much time did that add?

@eatkins
Copy link
Contributor

eatkins commented Sep 22, 2020

Sorry, but didn't we add a whole bunch of settings for watch/Glob things? How many and how much time did that add?

sbt 1.3.0 was faster to start up than 1.2.8 because I made a number of optimizations elsewhere that made up for the increased number of settings. I did in fact remove a number of settings that I had added because they increased the build load time and I wanted to ensure that load time did not increase and I delivered on that. I do performance testing of all of my significant work regardless of whether or not I report the results. Given how much work I have put into improving the overall performance in sbt, I think I have earned the right to voice my concerns. And I've been forceful in my tone because this was merged without any time for discussion on the eve of a major release.

Again, I think this is a great plugin but turning it on by default is not dissimilar to forcing all programs to run under gdb. It is a debugging tool that is not useful when you are debugging dependency resolution issues which most people are not doing most of the time. This is not a perfect analogy but it serves the point well enough. Also, even if there were no performance cost, I wouldn't want all of these settings and tasks added to my tab completions. There are already too many commands to fit on my terminal screen in many builds and this just makes that worse. The fact that in the past I have contributed to this problem myself is irrelevant to this discussion.

@dwijnand
Copy link
Member

I considered it relevant, but I meant no offence nor did I intend to imply there's no value your load time evaluations you reported above. I definitely agree to giving you a chance to review non-trivial changes before they land.

To me 11s to 12s doesn't change much, because 11s already means to me "don't load it more than absolutely necessary" - but that's just me, as a data point.

I've used sbt-dependency-graph a whole bunch over the years and have only used it less recently because it became less reliable with the 1.3/Coursier integration. Sounds like making it opt-in means enablePlugins on ever project. Which normally means editing the build file, which makes it impractical during bisects (which I consider plausible for dependency graph investigations). The alternative is to rewire the not-auto-triggering plugin into a local plugin (project/AutoDepGraphPlugin.scala) that isn't git-tracked. That's pretty advanced.

I guess there's two reasons (at least in my mind) for folding this in: keeping it stable and making it always available. It would be a shame to make that latter part inconveniently hard... You can't even set every ... it in a session... 🤔

@eatkins
Copy link
Contributor

eatkins commented Sep 23, 2020

Sounds like making it opt-in means enablePlugins on ever project.

I have been sloppy with my language. By turning it off by default, I mean making it a separate autoplugin that is not bundled in the sbt main classpath. The idea would be to make a plugins directory in the sbt project and publish new versions of the plugin with new versions of sbt but you would have to add something like enableSbtPlugin("org.scala-sbt", "sbt-dependency-graph", "1.4.0") to your ~/.sbt/1.0/plugins.sbt. That way power users like you can just leave it on all of the time.

Honestly, my reaction isn't so much about this specific issue but my escalating frustration with how slow sbt cold startup is. I agree that the difference 11-12 seconds doesn't matter but these kinds of changes add up. The mindset of not worrying about the impact of cold startup can then bleed out of sbt. Recently the bloop plugin, which is required by metals, added a feature where on startup it regenerates the bloop.json files. It causes the cold startup of all of my projects to take about 3-4 seconds longer. I feel and resent this change because I have to do cold startup frequently to test ui changes so it actually impacts my workflow.

My mindset has always been to optimize the sbt experience for the novice sbt user. At my last job, most of the of the people working on scala (it was a mixed-language company) never modified the build and were more or less just adding and modifying api endpoints to a CRUD application. This kind of user is generally more or less running sbt test (or if someone shows them, sbt ~test and never touches the advanced features. We were using sbt 0.13.18 and the slow performance of sbt batch mode was a noticeable turn off to people who didn't work with scala regularly which inhibited further adoption. The thin client was designed primarily for those users but even with the thin client, you can't avoid cold startup sometimes (I lump reload into cold startup).

eed3si9n added a commit to eed3si9n/sbt that referenced this pull request Sep 23, 2020
Ref sbt#5880

This split the dependency-graph plugin into MiniDependencyTreePlugin and DependencyTreePlugin.
@eed3si9n
Copy link
Member Author

Given how much work I have put into improving the overall performance in sbt, I think I have earned the right to voice my concerns. And I've been forceful in my tone because this was merged without any time for discussion on the eve of a major release.

Yes. In the hindsight rushing to merge this PR that includes my adaptation to enable by default was a mistake, and your concerns about loading performance is totally valid.

I just sent a PR to split the PR to split the plugin up a bit if that can be of a compromise between batteries-included and loading time.

@dwijnand
Copy link
Member

you would have to add something like enableSbtPlugin("org.scala-sbt", "sbt-dependency-graph", "1.4.0") to your ~/.sbt/1.0/plugins.sbt

It almost might as well be external then, as it would break if my build is using sbt 1.5 which includes the same kind of breakages that 1.3 brought to sbt-dependency-graph.

It would have to be more like enableDependencyGraph, but then it wouldn't be compatible to loading sbt 1.3 builds...

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.