[GitHub] [maven] rfscholte commented on a change in pull request #342: [MNG-5760] Add --resume / -r switch

[GitBox <gi...@apache.org>]

rfscholte commented on a change in pull request #342:
URL: https://github.com/apache/maven/pull/342#discussion_r435782230



##########
File path: maven-core/src/main/java/org/apache/maven/execution/DefaultMavenExecutionResult.java
##########
@@ -108,4 +110,16 @@ public void addBuildSummary( BuildSummary summary )
     {
         buildSummaries.put( summary.getProject(), summary );
     }
+
+    @Override
+    public boolean canResume()
+    {
+        return canResume;
+    }
+
+    @Override
+    public void setCanResume()

Review comment:
       Our style is pretty defensive: setter should have an argument. I can imagine we will boolean method references instead of if-true statements.

##########
File path: maven-core/src/main/java/org/apache/maven/DefaultMaven.java
##########
@@ -99,6 +101,9 @@
     @Named( GraphBuilder.HINT )
     private GraphBuilder graphBuilder;
 
+    @Inject
+    private BuildResumptionManager buildResumptionManager;

Review comment:
       In general `Manager` and `Service` are buzzwords, if you don't know what to call it. Not sure it `BuildResumer` better is, but I'd like to avoid Manager.

##########
File path: maven-core/src/main/java/org/apache/maven/DefaultMaven.java
##########
@@ -349,6 +363,26 @@ private void afterSessionEnd( Collection<MavenProject> projects, MavenSession se
         }
     }
 
+    private void saveResumptionDataWhenApplicable( MavenExecutionResult result, MavenSession session )

Review comment:
       I would remove `WhenApplicable`, it is shorter and the condition doesn't matter (all code is conditional). And why not sync the method name, i.e. `persistResumptionData`

##########
File path: maven-core/src/main/java/org/apache/maven/execution/BuildResumptionManager.java
##########
@@ -0,0 +1,74 @@
+package org.apache.maven.execution;
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import org.apache.maven.project.MavenProject;
+
+import java.util.List;
+
+/**
+ * This class describes most of the logic needed for the --resume / -r feature. Its goal is to ensure newer
+ * builds of the same project that have the -r command-line flag skip successfully built projects during earlier
+ * invocations of Maven.
+ */
+public interface BuildResumptionManager
+{
+    /**
+     * Persists any data needed to resume the build at a later point in time, using a new Maven invocation. This method
+     * may also decide it is not needed or meaningful to persist such data, and return <code>false</code> to indicate
+     * so.
+     *
+     * @param result The result of the current Maven invocation.
+     * @param rootProject The root project that is being built.
+     * @return Whether any data was persisted.
+     */
+    boolean persistResumptionData( final MavenExecutionResult result, final MavenProject rootProject );

Review comment:
       `false` can have 2 meanings: IOException or nothing to persist. Both may result in different actions. I'm not if returning a boolean is useful, but I would prefer an IOException being thrown here. Might impact other methods as well.

##########
File path: maven-core/src/main/java/org/apache/maven/execution/DefaultBuildResumptionManager.java
##########
@@ -0,0 +1,306 @@
+package org.apache.maven.execution;
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import com.google.common.annotations.VisibleForTesting;

Review comment:
       no guava (is a necessary transitive dependency), no commons-lang (will be removed as we only use a few methods which are also available with plexus-utils)

##########
File path: maven-core/src/main/java/org/apache/maven/execution/BuildResumptionManager.java
##########
@@ -0,0 +1,74 @@
+package org.apache.maven.execution;
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import org.apache.maven.project.MavenProject;
+
+import java.util.List;
+
+/**
+ * This class describes most of the logic needed for the --resume / -r feature. Its goal is to ensure newer
+ * builds of the same project that have the -r command-line flag skip successfully built projects during earlier
+ * invocations of Maven.
+ */
+public interface BuildResumptionManager
+{
+    /**
+     * Persists any data needed to resume the build at a later point in time, using a new Maven invocation. This method
+     * may also decide it is not needed or meaningful to persist such data, and return <code>false</code> to indicate
+     * so.
+     *
+     * @param result The result of the current Maven invocation.
+     * @param rootProject The root project that is being built.
+     * @return Whether any data was persisted.
+     */
+    boolean persistResumptionData( final MavenExecutionResult result, final MavenProject rootProject );
+
+    /**
+     * Uses previously stored resumption data to enrich an existing execution request.
+     * @param request The execution request that will be enriched.
+     * @param rootProject The root project that is being built.
+     */
+    void applyResumptionData( final MavenExecutionRequest request, final MavenProject rootProject );
+
+    /**
+     * Removes previously stored resumption data.
+     * @param rootProject The root project that is being built.
+     */
+    void removeResumptionData( final MavenProject rootProject );
+
+    /**
+     * A helper method to determine the value to resume the build with {@code -rf} taking into account the edge case
+     *   where multiple modules in the reactor have the same artifactId.
+     * <p>
+     * {@code -rf :artifactId} will pick up the first module which matches, but when multiple modules in the reactor
+     *   have the same artifactId, effective failed module might be later in build reactor.
+     * This means that developer will either have to type groupId or wait for build execution of all modules which
+     *   were fine, but they are still before one which reported errors.
+     * <p>Then the returned value is {@code groupId:artifactId} when there is a name clash and
+     * {@code :artifactId} if there is no conflict.
+     *
+     * @param mavenProjects Maven projects which are part of build execution.
+     * @param failedProject Project which has failed.
+     * @return Value for -rf flag to resume build exactly from place where it failed ({@code :artifactId} in general
+     * and {@code groupId:artifactId} when there is a name clash).
+     */
+    String getResumeFromSelector( final List<MavenProject> mavenProjects, final MavenProject failedProject );

Review comment:
       Not sure if this method belongs here. The previous 3 are clearly related to the storage of the resumptionData, this one is not.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org