Maven-like rolling output when the build happens to be linear #271

ppalaga · 2020-12-15T22:31:37Z

ppalaga · 2020-12-15T22:35:25Z

daemon/src/main/java/org/mvndaemon/mvnd/daemon/Server.java

-                    session.getRequest().getDegreeOfConcurrency()));
+
+            final DependencyGraph smartGraph = DependencyGraph.fromMaven(session);
+            session.setProjectDependencyGraph(smartGraph);


@gnodet this is quite a rough way to do it. I was not able to persuade the guice container to prefer my org.apache.maven.graph.GraphBuilder impl. to the default one. So I gave up and did this replacement instead. I would not mind if you veto this and propose something more elegant.

If so, it may be better to not rewrite the graph and just keep the code as it is.
i.e. DependencyGraph and not tie it to ProjectDependencyGraph.

ppalaga · 2020-12-16T11:48:15Z

I am a bit more content with 385d77e:

Our custom SmartGraphBuilder is properly instantiated via Guice
The DependencyGraph now has a width field, that returns 1 if the graph is serial and Integer.MAX_VALUE otherwise. That's enough for the current feature. It can be enhanced later to return the proper width for other purposes.

gnodet

I am a bit more content with 385d77e:

Our custom SmartGraphBuilder is properly instantiated via Guice

The DependencyGraph now has a width field, that returns 1 if the graph is serial and Integer.MAX_VALUE otherwise. That's enough for the current feature. It can be enhanced later to return the proper width for other purposes.

Would it be possible to keep the DependencyGraph interface independent of maven ? The algorithm I have only depends on the upstream / downstream methods, so having to create MavenProject objects for nothing will make things harder to test.

See https://github.com/gnodet/mvnd/blob/dag-width/daemon/src/test/java/org/mvndaemon/mvnd/builder/DagWidthTest.java

gnodet · 2020-12-16T09:25:12Z

daemon/src/main/java/org/mvndaemon/mvnd/builder/DependencyGraph.java

-            public Stream<MavenProject> getDownstreamProjects(MavenProject project) {
-                return downstreams.get(project).stream();
-            }
+        final boolean serial = !projects.stream().anyMatch(p -> downstreams.get(p).size() > 1);


This check isn't correct imho. In order to use such a check, we'd have to remove dependencies that are redundant. For example
A depends on B
A depends on C
B depends on C
This is a very common scenario (C being the parent, with 2 children), but I think it will fail the check because C will have 2 downstream projects, yet, the build is linear (C -> B -> A).

Actually, is there any case where it returns 1 apart when building a single project ? It looks like the parent/child relationship will make this simple algorithm useless. Am I missing something ?

Yeah, it is not perfect, but it works in some simple cases at least and for me it is still an important improvement. As I said, a proper width computation can be added later.

I can easily port my computation algorithm, provided you keep the DependencyGraph<K> interface.

I've rebased on master. The computation can be done using the following lines: https://github.com/gnodet/mvnd/blob/d77d1f777b5bdba4697d8afaadef730813af81ce/daemon/src/main/java/org/mvndaemon/mvnd/builder/SmartBuilder.java#L200-L203

gnodet · 2020-12-16T09:27:27Z

daemon/src/main/java/org/mvndaemon/mvnd/daemon/Server.java

+
+            final int maxThreads = smartGraph.isSerial() ? 1 : session.getRequest().getDegreeOfConcurrency();
+            queue.add(new BuildStarted(getCurrentProject(session).getName(), smartGraph.getSortedProjects().size(),
+                    maxThreads));
        }


I have a branch pending that attempts at computing the maximum width of the graph, let me check its state.

gnodet · 2020-12-16T09:30:21Z

daemon/src/main/java/org/mvndaemon/mvnd/daemon/Server.java

-                    session.getRequest().getDegreeOfConcurrency()));
+
+            final DependencyGraph smartGraph = DependencyGraph.fromMaven(session);
+            session.setProjectDependencyGraph(smartGraph);


If so, it may be better to not rewrite the graph and just keep the code as it is.
i.e. DependencyGraph and not tie it to ProjectDependencyGraph.

gnodet · 2020-12-16T12:32:02Z

common/src/main/java/org/mvndaemon/mvnd/common/logging/TerminalOutput.java

@@ -173,6 +173,11 @@ private boolean doAccept(Message entry) {
            this.maxThreads = bs.getMaxThreads();
            final int maxThreadsDigits = (int) (Math.log10(maxThreads) + 1);
            this.threadsFormat = "%" + (maxThreadsDigits * 3 + 2) + "s";
+            if (maxThreads == 1) {


Should we use a configuration flag for that ?

You mean some users might not want this auto-rolling? I must say I like it like this. I would not want to add the flag unless somebody explicitly asks for it. Of course, if you personally want this, I'll add it immediately.

I'm fine with the auto-rolling behaviour. Let's advertise the change in the release notes and see if people want a switch. I've read users saying they do really like the buffered output a lot, but for linear builds it may be a bit different.

ppalaga · 2020-12-16T12:55:30Z

Would it be possible to keep the DependencyGraph interface independent of maven ? The algorithm I have only depends on the upstream / downstream methods, so having to create MavenProject objects for nothing will make things harder to test.

See https://github.com/gnodet/mvnd/blob/dag-width/daemon/src/test/java/org/mvndaemon/mvnd/builder/DagWidthTest.java

Yeah, that's a valid point. Let me try that.

ppalaga · 2020-12-16T13:16:04Z

2708c12 keeps the DependencyGraph interface independent of Maven.

ppalaga · 2020-12-16T13:54:27Z

80b66e2: adapted your DagWidth implementation

gnodet · 2020-12-16T13:56:00Z

daemon/src/test/java/org/mvndaemon/mvnd/builder/DependencyGraphTest.java

@@ -29,7 +29,7 @@ public void testRules() {
        MavenProject a = newProject("a"), b = newProject("b"), c = newProject("c");


It may be better to rename the class altogether, given we're testing the SmartProjectDependencyGraph

gnodet · 2020-12-16T14:00:50Z

daemon/src/main/java/org/mvndaemon/mvnd/builder/SmartGraphBuilder.java

+            return Result.success(smartGraph);
+        }
+    }
+}


Overall, I'm a bit skeptic on the benefits of overriding the graph builder to wrap the existing graph by delegating to the standard graph builder, as we have to cast it later to actually access the correct instance.
It may be easier to simply wrap it when needed. If we need to avoid wrapping it multiple times (not sure that's actually the case), we can always cache it in the session / execution context somewhere.
The problem is that it ties us to maven internals (which I'd rather avoid as much as possible) with no real benefits imho.

it ties us to maven internals

That's a valid point

It may be easier to simply wrap it when needed. If we need to avoid wrapping it multiple times (not sure that's actually the case),

Yes, we do. The graph is needed on two locations at least.

we can always cache it in the session / execution context somewhere.

Yeah, storing it somewhere in the session was my first thought. I have not found any general purpose map in the session or in any of its associated classes. Do we have any better option than system properties, ThreadLocal or a static variable?

MavenSession.getExecutionRequest().getData() or MavenSession.getRepositorySession().getData() or MavenSession.getRepositorySession().getCache().

Given we want to avoid stale informations, the former may be the better as it won't outlive the current build.

At the moment, it's only used once. I suppose it's used in an additional call to compute the width ?

gnodet · 2020-12-16T14:42:58Z

@ppalaga Wouldn't it be easier to just include the changes on the client (when there's a single thread or theres's only a single project) and defer the server side stuff for 0.3.0 ?

ppalaga · 2020-12-16T17:11:55Z

c3ac2f6: DependencyGraph wraps ProjectDependencyGraph and stores it in session.getRequest().getData()

If this iteration is not good enough, I do not mind postponing this.

gnodet · 2020-12-16T19:09:15Z

c3ac2f6: DependencyGraph wraps ProjectDependencyGraph and stores it in session.getRequest().getData()

If this iteration is not good enough, I do not mind postponing this.

Ok, let me check asap.

gnodet · 2020-12-16T19:31:49Z

@ppalaga Could you just add the following change gnodet@b1d9bd5 ?

Otherwise looks good !

ppalaga · 2020-12-16T19:36:16Z

Could you just add the following change gnodet@b1d9bd5 ?

Accepted in 1c5e086

ppalaga changed the title ~~Maven-like rolling output when the build happens to be linear #269~~ Maven-like rolling output when the build happens to be linear Dec 15, 2020

ppalaga commented Dec 15, 2020

View reviewed changes

ppalaga force-pushed the i269 branch from 12b2328 to 385d77e Compare December 16, 2020 11:42

gnodet reviewed Dec 16, 2020

View reviewed changes

ppalaga force-pushed the i269 branch from 385d77e to 2708c12 Compare December 16, 2020 13:15

ppalaga force-pushed the i269 branch from fe963d5 to 80b66e2 Compare December 16, 2020 13:55

gnodet reviewed Dec 16, 2020

View reviewed changes

ppalaga force-pushed the i269 branch from 80b66e2 to 22023e4 Compare December 16, 2020 14:02

ppalaga force-pushed the i269 branch 2 times, most recently from bd596e2 to c3ac2f6 Compare December 16, 2020 17:10

Maven-like rolling output when the build happens to be linear apache#269

1c5e086

ppalaga force-pushed the i269 branch from c3ac2f6 to 1c5e086 Compare December 16, 2020 19:34

ppalaga merged commit f2cdcf9 into apache:master Dec 16, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Maven-like rolling output when the build happens to be linear #271

Maven-like rolling output when the build happens to be linear #271

ppalaga commented Dec 15, 2020 •

edited

Loading

ppalaga Dec 15, 2020

gnodet Dec 16, 2020

ppalaga commented Dec 16, 2020

gnodet left a comment •

edited

Loading

gnodet Dec 16, 2020

gnodet Dec 16, 2020

ppalaga Dec 16, 2020

gnodet Dec 16, 2020

gnodet Dec 16, 2020

gnodet Dec 16, 2020

gnodet Dec 16, 2020

gnodet Dec 16, 2020

ppalaga Dec 16, 2020

gnodet Dec 16, 2020

ppalaga commented Dec 16, 2020

ppalaga commented Dec 16, 2020

ppalaga commented Dec 16, 2020 •

edited

Loading

gnodet Dec 16, 2020

gnodet Dec 16, 2020

ppalaga Dec 16, 2020

gnodet Dec 16, 2020

gnodet commented Dec 16, 2020

ppalaga commented Dec 16, 2020

gnodet commented Dec 16, 2020

gnodet commented Dec 16, 2020 •

edited

Loading

ppalaga commented Dec 16, 2020

		@@ -29,7 +29,7 @@ public void testRules() {
		MavenProject a = newProject("a"), b = newProject("b"), c = newProject("c");

Maven-like rolling output when the build happens to be linear #271

Maven-like rolling output when the build happens to be linear #271

Conversation

ppalaga commented Dec 15, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ppalaga commented Dec 16, 2020

gnodet left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ppalaga commented Dec 16, 2020

ppalaga commented Dec 16, 2020

ppalaga commented Dec 16, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnodet commented Dec 16, 2020

ppalaga commented Dec 16, 2020

gnodet commented Dec 16, 2020

gnodet commented Dec 16, 2020 • edited Loading

ppalaga commented Dec 16, 2020

ppalaga commented Dec 15, 2020 •

edited

Loading

gnodet left a comment •

edited

Loading

ppalaga commented Dec 16, 2020 •

edited

Loading

gnodet commented Dec 16, 2020 •

edited

Loading