-
Notifications
You must be signed in to change notification settings - Fork 976
in-sources sbt-dependency-graph #5880
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
Conversation
…ated at another position
Conflicts: build.sbt
…r consumers as sbt `module-graph` task
…re too many nodes
…pendencies of a module
Optimize the screen space for dependencyBrowseGraph
Ref sbt/sbt-dependency-graph#178 This in-sources sbt-dependency-graph.
This comment has been minimized.
This comment has been minimized.
|
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. |
|
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. |
|
Not sure if there's a great way for turning plugins on or off. Metabuild gets cached so system properties do not always work. |
|
I was thinking putting it in it’s own project and requiring users to add it to manually to their plugins.sbt. |
|
Similar to |
|
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. |
|
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 The tasks supporting different output formats Ascii art, GraphML, DOT, and HTML take up majority of them.
|
|
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. |
|
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? |
|
I have no idea but in general setting counts seem to multiply much faster than I would naively expect across the board. |
|
Found it sbt/main/src/main/scala/sbt/plugins/DependencyGraphPlugin.scala Lines 76 to 77 in 54747b8
It's flatmapping across all configurations. |
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. |
|
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 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 |
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 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 |
Ref sbt#5880 This split the dependency-graph plugin into MiniDependencyTreePlugin and DependencyTreePlugin.
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. |
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 |
Ref sbt/sbt-dependency-graph#178
This in-sources sbt-dependency-graph so it can evolve together with sbt.
@dwijnand suggested:
To which I agreed: