You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@maven.apache.org by GitBox <gi...@apache.org> on 2020/05/04 15:09:58 UTC
[GitHub] [maven] mthmulders opened a new pull request #342: [MNG-5760] Add --resume / -r switch
mthmulders opened a new pull request #342:
URL: https://github.com/apache/maven/pull/342
This pull request adds the `--resume` / `-r` switch. It works like `--resume-from` / `-rf`, but does more than that:
1. It takes no arguments. Instead, when a build fails, it creates `target/resume.properties` which is read when the `--resume` / `-r` switch is specified.
1. It is smarter. When other modules later in the failed build succeeded, they will be excluded from the next (resumed) build. This only happens if there are no dependencies between the former and the latter.
## Status
This pull request is a work-in-progress. Not all functionality is finished. We're working on accompanying integration tests.
## Pull request checklist
- [X] There is a [JIRA issue](https://issues.apache.org/jira/browse/MNG-5760).
- [X] Each commit in the pull request has a meaningful subject line and body.
- [X] The pull request title is correctly formatted.
- [X] The pull request description is detailed enough to understand what the pull request does, how, and why.
- [ ] Run `mvn clean verify` to make sure basic checks pass. A more thorough check will be performed on your pull request automatically.
- [ ] You have run the [Core IT][core-its] successfully.
- [X] In any other case, please file an [Apache Individual Contributor License Agreement](https://www.apache.org/licenses/icla.pdf).
**ICLA has been signed by @MartinKanters and myself. CCLA has been signed by our employer, Info Support.**
[core-its]: https://maven.apache.org/core-its/core-it-suite/
----------------------------------------------------------------
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
[GitHub] [maven] mthmulders commented on a change in pull request #342: [MNG-5760] Add --resume / -r switch
Posted by GitBox <gi...@apache.org>.
mthmulders commented on a change in pull request #342:
URL: https://github.com/apache/maven/pull/342#discussion_r429747671
##########
File path: maven-core/src/main/java/org/apache/maven/execution/DefaultBuildResumptionManager.java
##########
@@ -0,0 +1,308 @@
+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;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.maven.lifecycle.LifecycleExecutionException;
+import org.apache.maven.model.Dependency;
+import org.apache.maven.project.MavenProject;
+import org.codehaus.plexus.logging.Logger;
+
+import javax.inject.Inject;
+import javax.inject.Named;
+import javax.inject.Singleton;
+import java.io.IOException;
+import java.io.Reader;
+import java.io.Writer;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.Paths;
+import java.util.Arrays;
+import java.util.List;
+import java.util.Objects;
+import java.util.Optional;
+import java.util.Properties;
+import java.util.stream.Collectors;
+
+import static java.util.Comparator.comparing;
+
+/**
+ * This implementation of {@link BuildResumptionManager} persists information in a properties file. The file is stored
+ * in the build output directory under the Maven execution root.
+ */
+@Named
+@Singleton
+public class DefaultBuildResumptionManager implements BuildResumptionManager
+{
+ private static final String RESUME_PROPERTIES_FILENAME = "resume.properties";
+ private static final String RESUME_FROM_PROPERTY = "resumeFrom";
+ private static final String EXCLUDED_PROJECTS_PROPERTY = "excludedProjects";
+ private static final String PROPERTY_DELIMITER = ", ";
+
+ @Inject
+ private Logger logger;
Review comment:
I think we use String concat a few times for building a log message. We should also use SLF4J's parameter feature (`"{} happened"`)
----------------------------------------------------------------
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
[GitHub] [maven] MartinKanters commented on a change in pull request #342: [MNG-5760] Add --resume / -r switch
Posted by GitBox <gi...@apache.org>.
MartinKanters commented on a change in pull request #342:
URL: https://github.com/apache/maven/pull/342#discussion_r436094305
##########
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:
I was also thinking about BuildResumer last time you mentioned that you didn't like the Manager part. I'll make the change, but if others have better names, please suggest them.
----------------------------------------------------------------
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
[GitHub] [maven] michael-o commented on a change in pull request #342: [MNG-5760] Add --resume / -r switch
Posted by GitBox <gi...@apache.org>.
michael-o commented on a change in pull request #342:
URL: https://github.com/apache/maven/pull/342#discussion_r429572926
##########
File path: maven-core/src/main/java/org/apache/maven/execution/DefaultBuildResumptionManager.java
##########
@@ -0,0 +1,308 @@
+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;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.maven.lifecycle.LifecycleExecutionException;
+import org.apache.maven.model.Dependency;
+import org.apache.maven.project.MavenProject;
+import org.codehaus.plexus.logging.Logger;
+
+import javax.inject.Inject;
+import javax.inject.Named;
+import javax.inject.Singleton;
+import java.io.IOException;
+import java.io.Reader;
+import java.io.Writer;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.Paths;
+import java.util.Arrays;
+import java.util.List;
+import java.util.Objects;
+import java.util.Optional;
+import java.util.Properties;
+import java.util.stream.Collectors;
+
+import static java.util.Comparator.comparing;
+
+/**
+ * This implementation of {@link BuildResumptionManager} persists information in a properties file. The file is stored
+ * in the build output directory under the Maven execution root.
+ */
+@Named
+@Singleton
+public class DefaultBuildResumptionManager implements BuildResumptionManager
+{
+ private static final String RESUME_PROPERTIES_FILENAME = "resume.properties";
+ private static final String RESUME_FROM_PROPERTY = "resumeFrom";
+ private static final String EXCLUDED_PROJECTS_PROPERTY = "excludedProjects";
+ private static final String PROPERTY_DELIMITER = ", ";
+
+ @Inject
+ private Logger logger;
Review comment:
Don't use the Plexus logger. You can use SLF4J directly.
##########
File path: maven-core/src/main/java/org/apache/maven/execution/DefaultBuildResumptionManager.java
##########
@@ -0,0 +1,308 @@
+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;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.maven.lifecycle.LifecycleExecutionException;
+import org.apache.maven.model.Dependency;
+import org.apache.maven.project.MavenProject;
+import org.codehaus.plexus.logging.Logger;
+
+import javax.inject.Inject;
+import javax.inject.Named;
+import javax.inject.Singleton;
+import java.io.IOException;
+import java.io.Reader;
+import java.io.Writer;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.Paths;
+import java.util.Arrays;
+import java.util.List;
+import java.util.Objects;
+import java.util.Optional;
+import java.util.Properties;
+import java.util.stream.Collectors;
+
+import static java.util.Comparator.comparing;
+
+/**
+ * This implementation of {@link BuildResumptionManager} persists information in a properties file. The file is stored
+ * in the build output directory under the Maven execution root.
+ */
+@Named
+@Singleton
+public class DefaultBuildResumptionManager implements BuildResumptionManager
+{
+ private static final String RESUME_PROPERTIES_FILENAME = "resume.properties";
+ private static final String RESUME_FROM_PROPERTY = "resumeFrom";
+ private static final String EXCLUDED_PROJECTS_PROPERTY = "excludedProjects";
+ private static final String PROPERTY_DELIMITER = ", ";
+
+ @Inject
+ private Logger logger;
+
+ @Override
+ public boolean persistResumptionData( MavenExecutionResult result, MavenProject rootProject )
+ {
+ Properties properties = determineResumptionProperties( result );
+
+ if ( properties.isEmpty() )
+ {
+ logger.debug( "Will not create " + RESUME_PROPERTIES_FILENAME + " file: nothing to resume from" );
+ return false;
+ }
+
+ return writeResumptionFile( rootProject, properties );
+ }
+
+ @Override
+ public void applyResumptionData( MavenExecutionRequest request, MavenProject rootProject )
+ {
+ Properties properties = loadResumptionFile( rootProject.getBuild().getDirectory() );
+ applyResumptionProperties( request, properties );
+ }
+
+ @Override
+ public void removeResumptionData( MavenProject rootProject )
+ {
+ Path resumeProperties = Paths.get( rootProject.getBuild().getDirectory(), RESUME_PROPERTIES_FILENAME );
+ try
+ {
+ Files.deleteIfExists( resumeProperties );
+ }
+ catch ( IOException e )
+ {
+ logger.warn( "Could not delete " + RESUME_PROPERTIES_FILENAME + " file. ", e );
+ }
+ }
+
+ @Override
+ public String getResumeFromSelector( List<MavenProject> mavenProjects, MavenProject failedProject )
+ {
+ boolean hasOverlappingArtifactId = mavenProjects.stream()
+ .filter( project -> failedProject.getArtifactId().equals( project.getArtifactId() ) )
+ .count() > 1;
+
+ if ( hasOverlappingArtifactId )
+ {
+ return failedProject.getGroupId() + ":" + failedProject.getArtifactId();
+ }
+
+ return ":" + failedProject.getArtifactId();
+ }
+
+ @VisibleForTesting
+ Properties determineResumptionProperties( MavenExecutionResult result )
+ {
+ Properties properties = new Properties();
+
+ List<MavenProject> failedProjects = getFailedProjectsInOrder( result );
+ if ( !failedProjects.isEmpty() )
+ {
+ MavenProject resumeFromProject = failedProjects.get( 0 );
+ Optional<String> resumeFrom = getResumeFrom( result, resumeFromProject );
+ Optional<String> projectsToSkip = determineProjectsToSkip( result, failedProjects, resumeFromProject );
+
+ resumeFrom.ifPresent( value -> properties.setProperty( RESUME_FROM_PROPERTY, value ) );
+ projectsToSkip.ifPresent( value -> properties.setProperty( EXCLUDED_PROJECTS_PROPERTY, value ) );
+ }
+ else
+ {
+ logger.warn( "Could not create " + RESUME_PROPERTIES_FILENAME + " file: no failed projects found" );
+ }
+
+ return properties;
+ }
+
+ private List<MavenProject> getFailedProjectsInOrder( MavenExecutionResult result )
+ {
+ List<MavenProject> sortedProjects = result.getTopologicallySortedProjects();
+
+ return result.getExceptions().stream()
+ .filter( LifecycleExecutionException.class::isInstance )
+ .map( LifecycleExecutionException.class::cast )
+ .map( LifecycleExecutionException::getProject )
+ .sorted( comparing( sortedProjects::indexOf ) )
+ .collect( Collectors.toList() );
+ }
+
+ /**
+ * Determine the project where the next build can be resumed from.
+ * If the failed project is the first project of the build,
+ * it does not make sense to use --resume-from, so the result will be empty.
+ * @param result The result of the Maven build.
+ * @param failedProject The first failed project of the build.
+ * @return An optional containing the resume-from suggestion.
+ */
+ private Optional<String> getResumeFrom( MavenExecutionResult result, MavenProject failedProject )
+ {
+ List<MavenProject> allSortedProjects = result.getTopologicallySortedProjects();
+ if ( !allSortedProjects.get( 0 ).equals( failedProject ) )
+ {
+ return Optional.of( String.format( "%s:%s", failedProject.getGroupId(), failedProject.getArtifactId() ) );
Review comment:
Inconsistent with `getResumeFromSelector()`. I see no real benefit of `format()` for this case. Static concat will be replaced with a builder at compile time anyway.
##########
File path: maven-core/src/main/java/org/apache/maven/execution/DefaultBuildResumptionManager.java
##########
@@ -0,0 +1,308 @@
+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;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.maven.lifecycle.LifecycleExecutionException;
+import org.apache.maven.model.Dependency;
+import org.apache.maven.project.MavenProject;
+import org.codehaus.plexus.logging.Logger;
+
+import javax.inject.Inject;
+import javax.inject.Named;
+import javax.inject.Singleton;
+import java.io.IOException;
+import java.io.Reader;
+import java.io.Writer;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.Paths;
+import java.util.Arrays;
+import java.util.List;
+import java.util.Objects;
+import java.util.Optional;
+import java.util.Properties;
+import java.util.stream.Collectors;
+
+import static java.util.Comparator.comparing;
+
+/**
+ * This implementation of {@link BuildResumptionManager} persists information in a properties file. The file is stored
+ * in the build output directory under the Maven execution root.
+ */
+@Named
+@Singleton
+public class DefaultBuildResumptionManager implements BuildResumptionManager
+{
+ private static final String RESUME_PROPERTIES_FILENAME = "resume.properties";
+ private static final String RESUME_FROM_PROPERTY = "resumeFrom";
+ private static final String EXCLUDED_PROJECTS_PROPERTY = "excludedProjects";
+ private static final String PROPERTY_DELIMITER = ", ";
+
+ @Inject
+ private Logger logger;
+
+ @Override
+ public boolean persistResumptionData( MavenExecutionResult result, MavenProject rootProject )
+ {
+ Properties properties = determineResumptionProperties( result );
+
+ if ( properties.isEmpty() )
+ {
+ logger.debug( "Will not create " + RESUME_PROPERTIES_FILENAME + " file: nothing to resume from" );
+ return false;
+ }
+
+ return writeResumptionFile( rootProject, properties );
+ }
+
+ @Override
+ public void applyResumptionData( MavenExecutionRequest request, MavenProject rootProject )
+ {
+ Properties properties = loadResumptionFile( rootProject.getBuild().getDirectory() );
+ applyResumptionProperties( request, properties );
+ }
+
+ @Override
+ public void removeResumptionData( MavenProject rootProject )
+ {
+ Path resumeProperties = Paths.get( rootProject.getBuild().getDirectory(), RESUME_PROPERTIES_FILENAME );
+ try
+ {
+ Files.deleteIfExists( resumeProperties );
+ }
+ catch ( IOException e )
+ {
+ logger.warn( "Could not delete " + RESUME_PROPERTIES_FILENAME + " file. ", e );
+ }
+ }
+
+ @Override
+ public String getResumeFromSelector( List<MavenProject> mavenProjects, MavenProject failedProject )
+ {
+ boolean hasOverlappingArtifactId = mavenProjects.stream()
+ .filter( project -> failedProject.getArtifactId().equals( project.getArtifactId() ) )
+ .count() > 1;
+
+ if ( hasOverlappingArtifactId )
+ {
+ return failedProject.getGroupId() + ":" + failedProject.getArtifactId();
+ }
+
+ return ":" + failedProject.getArtifactId();
+ }
+
+ @VisibleForTesting
+ Properties determineResumptionProperties( MavenExecutionResult result )
+ {
+ Properties properties = new Properties();
+
+ List<MavenProject> failedProjects = getFailedProjectsInOrder( result );
+ if ( !failedProjects.isEmpty() )
+ {
+ MavenProject resumeFromProject = failedProjects.get( 0 );
+ Optional<String> resumeFrom = getResumeFrom( result, resumeFromProject );
+ Optional<String> projectsToSkip = determineProjectsToSkip( result, failedProjects, resumeFromProject );
+
+ resumeFrom.ifPresent( value -> properties.setProperty( RESUME_FROM_PROPERTY, value ) );
+ projectsToSkip.ifPresent( value -> properties.setProperty( EXCLUDED_PROJECTS_PROPERTY, value ) );
+ }
+ else
+ {
+ logger.warn( "Could not create " + RESUME_PROPERTIES_FILENAME + " file: no failed projects found" );
+ }
+
+ return properties;
+ }
+
+ private List<MavenProject> getFailedProjectsInOrder( MavenExecutionResult result )
+ {
+ List<MavenProject> sortedProjects = result.getTopologicallySortedProjects();
+
+ return result.getExceptions().stream()
+ .filter( LifecycleExecutionException.class::isInstance )
+ .map( LifecycleExecutionException.class::cast )
+ .map( LifecycleExecutionException::getProject )
+ .sorted( comparing( sortedProjects::indexOf ) )
+ .collect( Collectors.toList() );
+ }
+
+ /**
+ * Determine the project where the next build can be resumed from.
+ * If the failed project is the first project of the build,
+ * it does not make sense to use --resume-from, so the result will be empty.
+ * @param result The result of the Maven build.
+ * @param failedProject The first failed project of the build.
+ * @return An optional containing the resume-from suggestion.
+ */
+ private Optional<String> getResumeFrom( MavenExecutionResult result, MavenProject failedProject )
+ {
+ List<MavenProject> allSortedProjects = result.getTopologicallySortedProjects();
+ if ( !allSortedProjects.get( 0 ).equals( failedProject ) )
+ {
+ return Optional.of( String.format( "%s:%s", failedProject.getGroupId(), failedProject.getArtifactId() ) );
+ }
+
+ return Optional.empty();
+ }
+
+ /**
+ * Projects after the first failed project could have succeeded by using -T or --fail-at-end.
+ * These projects can be skipped from later builds.
+ * This is not the case these projects are dependent on one of the failed projects.
+ * @param result The result of the Maven build.
+ * @param failedProjects The list of failed projects in the build.
+ * @param resumeFromProject The project where the build will be resumed with in the next run.
+ * @return An optional containing a comma separated list of projects which can be skipped,
+ * or an empty optional if no projects can be skipped.
+ */
+ private Optional<String> determineProjectsToSkip( MavenExecutionResult result, List<MavenProject> failedProjects,
+ MavenProject resumeFromProject )
+ {
+ List<MavenProject> allProjects = result.getTopologicallySortedProjects();
+ int resumeFromProjectIndex = allProjects.indexOf( resumeFromProject );
+ List<MavenProject> remainingProjects = allProjects.subList( resumeFromProjectIndex + 1, allProjects.size() );
+
+ List<GroupArtifactPair> failedProjectsGAList = failedProjects.stream()
+ .map( GroupArtifactPair::new )
+ .collect( Collectors.toList() );
+
+ String projectsToSkip = remainingProjects.stream()
+ .filter( project -> result.getBuildSummary( project ) instanceof BuildSuccess )
+ .filter( project -> hasNoDependencyOnProjects( project, failedProjectsGAList ) )
+ .map( project -> String.format( "%s:%s", project.getGroupId(), project.getArtifactId() ) )
+ .collect( Collectors.joining( PROPERTY_DELIMITER ) );
+
+ if ( !StringUtils.isEmpty( projectsToSkip ) )
+ {
+ return Optional.of( projectsToSkip );
+ }
+
+ return Optional.empty();
+ }
+
+ private boolean hasNoDependencyOnProjects( MavenProject project, List<GroupArtifactPair> projectsGAs )
+ {
+ return project.getDependencies().stream()
+ .map( GroupArtifactPair::new )
+ .noneMatch( projectsGAs::contains );
+ }
+
+ private boolean writeResumptionFile( MavenProject rootProject, Properties properties )
+ {
+ Path resumeProperties = Paths.get( rootProject.getBuild().getDirectory(), RESUME_PROPERTIES_FILENAME );
+ try
+ {
+ Files.createDirectories( resumeProperties.getParent() );
Review comment:
Does this one assume that we are in a phase/goal where Maven hasn't created `target` yet?
##########
File path: maven-core/src/main/java/org/apache/maven/execution/DefaultBuildResumptionManager.java
##########
@@ -0,0 +1,308 @@
+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;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.maven.lifecycle.LifecycleExecutionException;
+import org.apache.maven.model.Dependency;
+import org.apache.maven.project.MavenProject;
+import org.codehaus.plexus.logging.Logger;
+
+import javax.inject.Inject;
+import javax.inject.Named;
+import javax.inject.Singleton;
+import java.io.IOException;
+import java.io.Reader;
+import java.io.Writer;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.Paths;
+import java.util.Arrays;
+import java.util.List;
+import java.util.Objects;
+import java.util.Optional;
+import java.util.Properties;
+import java.util.stream.Collectors;
+
+import static java.util.Comparator.comparing;
+
+/**
+ * This implementation of {@link BuildResumptionManager} persists information in a properties file. The file is stored
+ * in the build output directory under the Maven execution root.
+ */
+@Named
+@Singleton
+public class DefaultBuildResumptionManager implements BuildResumptionManager
+{
+ private static final String RESUME_PROPERTIES_FILENAME = "resume.properties";
+ private static final String RESUME_FROM_PROPERTY = "resumeFrom";
+ private static final String EXCLUDED_PROJECTS_PROPERTY = "excludedProjects";
+ private static final String PROPERTY_DELIMITER = ", ";
+
+ @Inject
+ private Logger logger;
+
+ @Override
+ public boolean persistResumptionData( MavenExecutionResult result, MavenProject rootProject )
+ {
+ Properties properties = determineResumptionProperties( result );
+
+ if ( properties.isEmpty() )
+ {
+ logger.debug( "Will not create " + RESUME_PROPERTIES_FILENAME + " file: nothing to resume from" );
+ return false;
+ }
+
+ return writeResumptionFile( rootProject, properties );
+ }
+
+ @Override
+ public void applyResumptionData( MavenExecutionRequest request, MavenProject rootProject )
+ {
+ Properties properties = loadResumptionFile( rootProject.getBuild().getDirectory() );
+ applyResumptionProperties( request, properties );
+ }
+
+ @Override
+ public void removeResumptionData( MavenProject rootProject )
+ {
+ Path resumeProperties = Paths.get( rootProject.getBuild().getDirectory(), RESUME_PROPERTIES_FILENAME );
+ try
+ {
+ Files.deleteIfExists( resumeProperties );
+ }
+ catch ( IOException e )
+ {
+ logger.warn( "Could not delete " + RESUME_PROPERTIES_FILENAME + " file. ", e );
+ }
+ }
+
+ @Override
+ public String getResumeFromSelector( List<MavenProject> mavenProjects, MavenProject failedProject )
+ {
+ boolean hasOverlappingArtifactId = mavenProjects.stream()
+ .filter( project -> failedProject.getArtifactId().equals( project.getArtifactId() ) )
+ .count() > 1;
+
+ if ( hasOverlappingArtifactId )
+ {
+ return failedProject.getGroupId() + ":" + failedProject.getArtifactId();
+ }
+
+ return ":" + failedProject.getArtifactId();
+ }
+
+ @VisibleForTesting
+ Properties determineResumptionProperties( MavenExecutionResult result )
+ {
+ Properties properties = new Properties();
+
+ List<MavenProject> failedProjects = getFailedProjectsInOrder( result );
+ if ( !failedProjects.isEmpty() )
+ {
+ MavenProject resumeFromProject = failedProjects.get( 0 );
+ Optional<String> resumeFrom = getResumeFrom( result, resumeFromProject );
+ Optional<String> projectsToSkip = determineProjectsToSkip( result, failedProjects, resumeFromProject );
+
+ resumeFrom.ifPresent( value -> properties.setProperty( RESUME_FROM_PROPERTY, value ) );
+ projectsToSkip.ifPresent( value -> properties.setProperty( EXCLUDED_PROJECTS_PROPERTY, value ) );
+ }
+ else
+ {
+ logger.warn( "Could not create " + RESUME_PROPERTIES_FILENAME + " file: no failed projects found" );
+ }
+
+ return properties;
+ }
+
+ private List<MavenProject> getFailedProjectsInOrder( MavenExecutionResult result )
+ {
+ List<MavenProject> sortedProjects = result.getTopologicallySortedProjects();
+
+ return result.getExceptions().stream()
+ .filter( LifecycleExecutionException.class::isInstance )
+ .map( LifecycleExecutionException.class::cast )
+ .map( LifecycleExecutionException::getProject )
+ .sorted( comparing( sortedProjects::indexOf ) )
+ .collect( Collectors.toList() );
+ }
+
+ /**
+ * Determine the project where the next build can be resumed from.
+ * If the failed project is the first project of the build,
+ * it does not make sense to use --resume-from, so the result will be empty.
+ * @param result The result of the Maven build.
+ * @param failedProject The first failed project of the build.
+ * @return An optional containing the resume-from suggestion.
+ */
+ private Optional<String> getResumeFrom( MavenExecutionResult result, MavenProject failedProject )
+ {
+ List<MavenProject> allSortedProjects = result.getTopologicallySortedProjects();
+ if ( !allSortedProjects.get( 0 ).equals( failedProject ) )
+ {
+ return Optional.of( String.format( "%s:%s", failedProject.getGroupId(), failedProject.getArtifactId() ) );
+ }
+
+ return Optional.empty();
+ }
+
+ /**
+ * Projects after the first failed project could have succeeded by using -T or --fail-at-end.
+ * These projects can be skipped from later builds.
+ * This is not the case these projects are dependent on one of the failed projects.
+ * @param result The result of the Maven build.
+ * @param failedProjects The list of failed projects in the build.
+ * @param resumeFromProject The project where the build will be resumed with in the next run.
+ * @return An optional containing a comma separated list of projects which can be skipped,
+ * or an empty optional if no projects can be skipped.
+ */
+ private Optional<String> determineProjectsToSkip( MavenExecutionResult result, List<MavenProject> failedProjects,
+ MavenProject resumeFromProject )
+ {
+ List<MavenProject> allProjects = result.getTopologicallySortedProjects();
+ int resumeFromProjectIndex = allProjects.indexOf( resumeFromProject );
+ List<MavenProject> remainingProjects = allProjects.subList( resumeFromProjectIndex + 1, allProjects.size() );
+
+ List<GroupArtifactPair> failedProjectsGAList = failedProjects.stream()
+ .map( GroupArtifactPair::new )
+ .collect( Collectors.toList() );
+
+ String projectsToSkip = remainingProjects.stream()
+ .filter( project -> result.getBuildSummary( project ) instanceof BuildSuccess )
+ .filter( project -> hasNoDependencyOnProjects( project, failedProjectsGAList ) )
+ .map( project -> String.format( "%s:%s", project.getGroupId(), project.getArtifactId() ) )
+ .collect( Collectors.joining( PROPERTY_DELIMITER ) );
+
+ if ( !StringUtils.isEmpty( projectsToSkip ) )
+ {
+ return Optional.of( projectsToSkip );
+ }
+
+ return Optional.empty();
+ }
+
+ private boolean hasNoDependencyOnProjects( MavenProject project, List<GroupArtifactPair> projectsGAs )
+ {
+ return project.getDependencies().stream()
+ .map( GroupArtifactPair::new )
+ .noneMatch( projectsGAs::contains );
+ }
+
+ private boolean writeResumptionFile( MavenProject rootProject, Properties properties )
+ {
+ Path resumeProperties = Paths.get( rootProject.getBuild().getDirectory(), RESUME_PROPERTIES_FILENAME );
+ try
+ {
+ Files.createDirectories( resumeProperties.getParent() );
+ try ( Writer writer = Files.newBufferedWriter( resumeProperties ) )
+ {
+ properties.store( writer, null );
+ }
+ }
+ catch ( IOException e )
+ {
+ logger.warn( "Could not create " + RESUME_PROPERTIES_FILENAME + " file. ", e );
+ return false;
+ }
+
+ return true;
+ }
+
+ private Properties loadResumptionFile( String rootBuildDirectory )
Review comment:
The signature is inconsistent with `writeResumptionFile()`. once you pass the string directly, on the other one you pass the Maven project. Any reason for this?
----------------------------------------------------------------
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
[GitHub] [maven] MartinKanters commented on a change in pull request #342: [MNG-5760] Add --resume / -r switch
Posted by GitBox <gi...@apache.org>.
MartinKanters commented on a change in pull request #342:
URL: https://github.com/apache/maven/pull/342#discussion_r436106166
##########
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:
We were doubting about this as well, but still put it here, since it is related to the resumption of any following build. On the contrary it is only loosely related to the `-r` feature (only when that failed persisting the file, `getResumeFromSelector` will be called)
@mthmulders or others, what do you think?
----------------------------------------------------------------
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
[GitHub] [maven] MartinKanters commented on a change in pull request #342: [MNG-5760] Add --resume / -r switch
Posted by GitBox <gi...@apache.org>.
MartinKanters commented on a change in pull request #342:
URL: https://github.com/apache/maven/pull/342#discussion_r436122276
##########
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:
Will add it, it sounds like a good idea. We can also simplify the if-true statement in `DefaultMaven#persistResumptionData` then
----------------------------------------------------------------
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
[GitHub] [maven] MartinKanters commented on a change in pull request #342: [MNG-5760] Add --resume / -r switch
Posted by GitBox <gi...@apache.org>.
MartinKanters commented on a change in pull request #342:
URL: https://github.com/apache/maven/pull/342#discussion_r436106166
##########
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:
We were doubting about this as well, but still put it here, since it is related to the resumption of any following build. On the contrary it is only related to the -r feature loosely (only when that failed persisting the file, `getResumeFromSelector` will be called)
@mthmulders or others, what do you think?
----------------------------------------------------------------
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
[GitHub] [maven] MartinKanters commented on a change in pull request #342: [MNG-5760] Add --resume / -r switch
Posted by GitBox <gi...@apache.org>.
MartinKanters commented on a change in pull request #342:
URL: https://github.com/apache/maven/pull/342#discussion_r436104556
##########
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 agree, will do.
----------------------------------------------------------------
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
[GitHub] [maven] mthmulders commented on a change in pull request #342: [MNG-5760] Add --resume / -r switch
Posted by GitBox <gi...@apache.org>.
mthmulders commented on a change in pull request #342:
URL: https://github.com/apache/maven/pull/342#discussion_r436133279
##########
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:
Following the "single responsibility principle", on second thought I'd say it doesn't belong here. Let's make the `BuildResumptionManager` class responsible for just storing and retrieving resume-related data - not for user-interface texts such as the "-rf hint".
----------------------------------------------------------------
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
[GitHub] [maven] MartinKanters commented on a change in pull request #342: [MNG-5760] Add --resume / -r switch
Posted by GitBox <gi...@apache.org>.
MartinKanters commented on a change in pull request #342:
URL: https://github.com/apache/maven/pull/342#discussion_r436112546
##########
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:
I agree that it false has 2 meanings and that the calling method would like to act differently upon it. But an IOException will leak the internal logic to the outside (that it is using filesystem for storing this data) and it might potentially give problems later if we would get a different implementation. I'm thinking of creating a specific Exception, wrapping the IOException.
----------------------------------------------------------------
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
[GitHub] [maven] MartinKanters commented on a change in pull request #342: [MNG-5760] Add --resume / -r switch
Posted by GitBox <gi...@apache.org>.
MartinKanters commented on a change in pull request #342:
URL: https://github.com/apache/maven/pull/342#discussion_r436099407
##########
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:
That being said, "resumer" is not a valid English word, I think.
We could also go for `BuildResumption`?
----------------------------------------------------------------
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
[GitHub] [maven] rfscholte commented on a change in pull request #342: [MNG-5760] Add --resume / -r switch
Posted by 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
[GitHub] [maven] MartinKanters commented on pull request #342: [MNG-5760] Add --resume / -r switch
Posted by GitBox <gi...@apache.org>.
MartinKanters commented on pull request #342:
URL: https://github.com/apache/maven/pull/342#issuecomment-633401173
> how about a simple `canResume()` (who knows if there will be other requirements to resume)?
Sounds good, Robert.
----------------------------------------------------------------
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
[GitHub] [maven] mthmulders commented on a change in pull request #342: [MNG-5760] Add --resume / -r switch
Posted by GitBox <gi...@apache.org>.
mthmulders commented on a change in pull request #342:
URL: https://github.com/apache/maven/pull/342#discussion_r429940070
##########
File path: maven-core/src/main/java/org/apache/maven/DefaultMaven.java
##########
@@ -366,20 +366,21 @@ private void afterSessionEnd( Collection<MavenProject> projects, MavenSession se
private void saveResumptionDataWhenApplicable( MavenExecutionResult result, MavenSession session )
{
- List<LifecycleExecutionException> lifecycleExecutionExceptions = result.getExceptions().stream()
+ long lifecycleExecutionExceptionCount = result.getExceptions().stream()
.filter( LifecycleExecutionException.class::isInstance )
- .map( LifecycleExecutionException.class::cast )
- .collect( Collectors.toList() );
+ .count();
Review comment:
Ah yes, of course, that makes perfect sense!
----------------------------------------------------------------
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
[GitHub] [maven] MartinKanters commented on a change in pull request #342: [MNG-5760] Add --resume / -r switch
Posted by GitBox <gi...@apache.org>.
MartinKanters commented on a change in pull request #342:
URL: https://github.com/apache/maven/pull/342#discussion_r436109778
##########
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:
Right, sounds like a good reason to remove it indeed. I will replace it with a comment then.
----------------------------------------------------------------
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
[GitHub] [maven] rfscholte commented on pull request #342: [MNG-5760] Add --resume / -r switch
Posted by GitBox <gi...@apache.org>.
rfscholte commented on pull request #342:
URL: https://github.com/apache/maven/pull/342#issuecomment-646999247
Fixed in https://github.com/apache/maven/commit/658ad90b3850131e4a73fd6cca2ead30f6e5f213
----------------------------------------------------------------
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
[GitHub] [maven] MartinKanters commented on a change in pull request #342: [MNG-5760] Add --resume / -r switch
Posted by GitBox <gi...@apache.org>.
MartinKanters commented on a change in pull request #342:
URL: https://github.com/apache/maven/pull/342#discussion_r436252389
##########
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:
Okay, I agree that it seems only valid to let the BuildResumer class be focused on the new resume feature. I have moved it back to the MavenCli class.
----------------------------------------------------------------
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
[GitHub] [maven] MartinKanters commented on a change in pull request #342: [MNG-5760] Add --resume / -r switch
Posted by GitBox <gi...@apache.org>.
MartinKanters commented on a change in pull request #342:
URL: https://github.com/apache/maven/pull/342#discussion_r429751000
##########
File path: maven-core/src/main/java/org/apache/maven/execution/DefaultBuildResumptionManager.java
##########
@@ -0,0 +1,308 @@
+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;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.maven.lifecycle.LifecycleExecutionException;
+import org.apache.maven.model.Dependency;
+import org.apache.maven.project.MavenProject;
+import org.codehaus.plexus.logging.Logger;
+
+import javax.inject.Inject;
+import javax.inject.Named;
+import javax.inject.Singleton;
+import java.io.IOException;
+import java.io.Reader;
+import java.io.Writer;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.Paths;
+import java.util.Arrays;
+import java.util.List;
+import java.util.Objects;
+import java.util.Optional;
+import java.util.Properties;
+import java.util.stream.Collectors;
+
+import static java.util.Comparator.comparing;
+
+/**
+ * This implementation of {@link BuildResumptionManager} persists information in a properties file. The file is stored
+ * in the build output directory under the Maven execution root.
+ */
+@Named
+@Singleton
+public class DefaultBuildResumptionManager implements BuildResumptionManager
+{
+ private static final String RESUME_PROPERTIES_FILENAME = "resume.properties";
+ private static final String RESUME_FROM_PROPERTY = "resumeFrom";
+ private static final String EXCLUDED_PROJECTS_PROPERTY = "excludedProjects";
+ private static final String PROPERTY_DELIMITER = ", ";
+
+ @Inject
+ private Logger logger;
+
+ @Override
+ public boolean persistResumptionData( MavenExecutionResult result, MavenProject rootProject )
+ {
+ Properties properties = determineResumptionProperties( result );
+
+ if ( properties.isEmpty() )
+ {
+ logger.debug( "Will not create " + RESUME_PROPERTIES_FILENAME + " file: nothing to resume from" );
+ return false;
+ }
+
+ return writeResumptionFile( rootProject, properties );
+ }
+
+ @Override
+ public void applyResumptionData( MavenExecutionRequest request, MavenProject rootProject )
+ {
+ Properties properties = loadResumptionFile( rootProject.getBuild().getDirectory() );
+ applyResumptionProperties( request, properties );
+ }
+
+ @Override
+ public void removeResumptionData( MavenProject rootProject )
+ {
+ Path resumeProperties = Paths.get( rootProject.getBuild().getDirectory(), RESUME_PROPERTIES_FILENAME );
+ try
+ {
+ Files.deleteIfExists( resumeProperties );
+ }
+ catch ( IOException e )
+ {
+ logger.warn( "Could not delete " + RESUME_PROPERTIES_FILENAME + " file. ", e );
+ }
+ }
+
+ @Override
+ public String getResumeFromSelector( List<MavenProject> mavenProjects, MavenProject failedProject )
+ {
+ boolean hasOverlappingArtifactId = mavenProjects.stream()
+ .filter( project -> failedProject.getArtifactId().equals( project.getArtifactId() ) )
+ .count() > 1;
+
+ if ( hasOverlappingArtifactId )
+ {
+ return failedProject.getGroupId() + ":" + failedProject.getArtifactId();
+ }
+
+ return ":" + failedProject.getArtifactId();
+ }
+
+ @VisibleForTesting
+ Properties determineResumptionProperties( MavenExecutionResult result )
+ {
+ Properties properties = new Properties();
+
+ List<MavenProject> failedProjects = getFailedProjectsInOrder( result );
+ if ( !failedProjects.isEmpty() )
+ {
+ MavenProject resumeFromProject = failedProjects.get( 0 );
+ Optional<String> resumeFrom = getResumeFrom( result, resumeFromProject );
+ Optional<String> projectsToSkip = determineProjectsToSkip( result, failedProjects, resumeFromProject );
+
+ resumeFrom.ifPresent( value -> properties.setProperty( RESUME_FROM_PROPERTY, value ) );
+ projectsToSkip.ifPresent( value -> properties.setProperty( EXCLUDED_PROJECTS_PROPERTY, value ) );
+ }
+ else
+ {
+ logger.warn( "Could not create " + RESUME_PROPERTIES_FILENAME + " file: no failed projects found" );
+ }
+
+ return properties;
+ }
+
+ private List<MavenProject> getFailedProjectsInOrder( MavenExecutionResult result )
+ {
+ List<MavenProject> sortedProjects = result.getTopologicallySortedProjects();
+
+ return result.getExceptions().stream()
+ .filter( LifecycleExecutionException.class::isInstance )
+ .map( LifecycleExecutionException.class::cast )
+ .map( LifecycleExecutionException::getProject )
+ .sorted( comparing( sortedProjects::indexOf ) )
+ .collect( Collectors.toList() );
+ }
+
+ /**
+ * Determine the project where the next build can be resumed from.
+ * If the failed project is the first project of the build,
+ * it does not make sense to use --resume-from, so the result will be empty.
+ * @param result The result of the Maven build.
+ * @param failedProject The first failed project of the build.
+ * @return An optional containing the resume-from suggestion.
+ */
+ private Optional<String> getResumeFrom( MavenExecutionResult result, MavenProject failedProject )
+ {
+ List<MavenProject> allSortedProjects = result.getTopologicallySortedProjects();
+ if ( !allSortedProjects.get( 0 ).equals( failedProject ) )
+ {
+ return Optional.of( String.format( "%s:%s", failedProject.getGroupId(), failedProject.getArtifactId() ) );
+ }
+
+ return Optional.empty();
+ }
+
+ /**
+ * Projects after the first failed project could have succeeded by using -T or --fail-at-end.
+ * These projects can be skipped from later builds.
+ * This is not the case these projects are dependent on one of the failed projects.
+ * @param result The result of the Maven build.
+ * @param failedProjects The list of failed projects in the build.
+ * @param resumeFromProject The project where the build will be resumed with in the next run.
+ * @return An optional containing a comma separated list of projects which can be skipped,
+ * or an empty optional if no projects can be skipped.
+ */
+ private Optional<String> determineProjectsToSkip( MavenExecutionResult result, List<MavenProject> failedProjects,
+ MavenProject resumeFromProject )
+ {
+ List<MavenProject> allProjects = result.getTopologicallySortedProjects();
+ int resumeFromProjectIndex = allProjects.indexOf( resumeFromProject );
+ List<MavenProject> remainingProjects = allProjects.subList( resumeFromProjectIndex + 1, allProjects.size() );
+
+ List<GroupArtifactPair> failedProjectsGAList = failedProjects.stream()
+ .map( GroupArtifactPair::new )
+ .collect( Collectors.toList() );
+
+ String projectsToSkip = remainingProjects.stream()
+ .filter( project -> result.getBuildSummary( project ) instanceof BuildSuccess )
+ .filter( project -> hasNoDependencyOnProjects( project, failedProjectsGAList ) )
+ .map( project -> String.format( "%s:%s", project.getGroupId(), project.getArtifactId() ) )
+ .collect( Collectors.joining( PROPERTY_DELIMITER ) );
+
+ if ( !StringUtils.isEmpty( projectsToSkip ) )
+ {
+ return Optional.of( projectsToSkip );
+ }
+
+ return Optional.empty();
+ }
+
+ private boolean hasNoDependencyOnProjects( MavenProject project, List<GroupArtifactPair> projectsGAs )
+ {
+ return project.getDependencies().stream()
+ .map( GroupArtifactPair::new )
+ .noneMatch( projectsGAs::contains );
+ }
+
+ private boolean writeResumptionFile( MavenProject rootProject, Properties properties )
+ {
+ Path resumeProperties = Paths.get( rootProject.getBuild().getDirectory(), RESUME_PROPERTIES_FILENAME );
+ try
+ {
+ Files.createDirectories( resumeProperties.getParent() );
Review comment:
Yes, in the case of our [integration test project](https://github.com/apache/maven-integration-testing/pull/61/files), we execute `mvn test` on a multi-module project. (root + 3 children). The root project will not have a target folder generated at the end of the build.
----------------------------------------------------------------
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
[GitHub] [maven] rfscholte commented on a change in pull request #342: [MNG-5760] Add --resume / -r switch
Posted by GitBox <gi...@apache.org>.
rfscholte commented on a change in pull request #342:
URL: https://github.com/apache/maven/pull/342#discussion_r429806085
##########
File path: maven-core/src/main/java/org/apache/maven/execution/DefaultBuildResumptionManager.java
##########
@@ -0,0 +1,308 @@
+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;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.maven.lifecycle.LifecycleExecutionException;
+import org.apache.maven.model.Dependency;
+import org.apache.maven.project.MavenProject;
+import org.codehaus.plexus.logging.Logger;
+
+import javax.inject.Inject;
+import javax.inject.Named;
+import javax.inject.Singleton;
+import java.io.IOException;
+import java.io.Reader;
+import java.io.Writer;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.Paths;
+import java.util.Arrays;
+import java.util.List;
+import java.util.Objects;
+import java.util.Optional;
+import java.util.Properties;
+import java.util.stream.Collectors;
+
+import static java.util.Comparator.comparing;
+
+/**
+ * This implementation of {@link BuildResumptionManager} persists information in a properties file. The file is stored
+ * in the build output directory under the Maven execution root.
+ */
+@Named
+@Singleton
+public class DefaultBuildResumptionManager implements BuildResumptionManager
+{
+ private static final String RESUME_PROPERTIES_FILENAME = "resume.properties";
+ private static final String RESUME_FROM_PROPERTY = "resumeFrom";
+ private static final String EXCLUDED_PROJECTS_PROPERTY = "excludedProjects";
+ private static final String PROPERTY_DELIMITER = ", ";
+
+ @Inject
+ private Logger logger;
+
+ @Override
+ public boolean persistResumptionData( MavenExecutionResult result, MavenProject rootProject )
+ {
+ Properties properties = determineResumptionProperties( result );
+
+ if ( properties.isEmpty() )
+ {
+ logger.debug( "Will not create " + RESUME_PROPERTIES_FILENAME + " file: nothing to resume from" );
+ return false;
+ }
+
+ return writeResumptionFile( rootProject, properties );
+ }
+
+ @Override
+ public void applyResumptionData( MavenExecutionRequest request, MavenProject rootProject )
+ {
+ Properties properties = loadResumptionFile( rootProject.getBuild().getDirectory() );
+ applyResumptionProperties( request, properties );
+ }
+
+ @Override
+ public void removeResumptionData( MavenProject rootProject )
+ {
+ Path resumeProperties = Paths.get( rootProject.getBuild().getDirectory(), RESUME_PROPERTIES_FILENAME );
+ try
+ {
+ Files.deleteIfExists( resumeProperties );
+ }
+ catch ( IOException e )
+ {
+ logger.warn( "Could not delete " + RESUME_PROPERTIES_FILENAME + " file. ", e );
+ }
+ }
+
+ @Override
+ public String getResumeFromSelector( List<MavenProject> mavenProjects, MavenProject failedProject )
+ {
+ boolean hasOverlappingArtifactId = mavenProjects.stream()
+ .filter( project -> failedProject.getArtifactId().equals( project.getArtifactId() ) )
+ .count() > 1;
+
+ if ( hasOverlappingArtifactId )
+ {
+ return failedProject.getGroupId() + ":" + failedProject.getArtifactId();
+ }
+
+ return ":" + failedProject.getArtifactId();
+ }
+
+ @VisibleForTesting
+ Properties determineResumptionProperties( MavenExecutionResult result )
+ {
+ Properties properties = new Properties();
+
+ List<MavenProject> failedProjects = getFailedProjectsInOrder( result );
+ if ( !failedProjects.isEmpty() )
+ {
+ MavenProject resumeFromProject = failedProjects.get( 0 );
+ Optional<String> resumeFrom = getResumeFrom( result, resumeFromProject );
+ Optional<String> projectsToSkip = determineProjectsToSkip( result, failedProjects, resumeFromProject );
+
+ resumeFrom.ifPresent( value -> properties.setProperty( RESUME_FROM_PROPERTY, value ) );
+ projectsToSkip.ifPresent( value -> properties.setProperty( EXCLUDED_PROJECTS_PROPERTY, value ) );
+ }
+ else
+ {
+ logger.warn( "Could not create " + RESUME_PROPERTIES_FILENAME + " file: no failed projects found" );
+ }
+
+ return properties;
+ }
+
+ private List<MavenProject> getFailedProjectsInOrder( MavenExecutionResult result )
+ {
+ List<MavenProject> sortedProjects = result.getTopologicallySortedProjects();
+
+ return result.getExceptions().stream()
+ .filter( LifecycleExecutionException.class::isInstance )
+ .map( LifecycleExecutionException.class::cast )
+ .map( LifecycleExecutionException::getProject )
+ .sorted( comparing( sortedProjects::indexOf ) )
+ .collect( Collectors.toList() );
+ }
+
+ /**
+ * Determine the project where the next build can be resumed from.
+ * If the failed project is the first project of the build,
+ * it does not make sense to use --resume-from, so the result will be empty.
+ * @param result The result of the Maven build.
+ * @param failedProject The first failed project of the build.
+ * @return An optional containing the resume-from suggestion.
+ */
+ private Optional<String> getResumeFrom( MavenExecutionResult result, MavenProject failedProject )
+ {
+ List<MavenProject> allSortedProjects = result.getTopologicallySortedProjects();
+ if ( !allSortedProjects.get( 0 ).equals( failedProject ) )
+ {
+ return Optional.of( String.format( "%s:%s", failedProject.getGroupId(), failedProject.getArtifactId() ) );
+ }
+
+ return Optional.empty();
+ }
+
+ /**
+ * Projects after the first failed project could have succeeded by using -T or --fail-at-end.
+ * These projects can be skipped from later builds.
+ * This is not the case these projects are dependent on one of the failed projects.
+ * @param result The result of the Maven build.
+ * @param failedProjects The list of failed projects in the build.
+ * @param resumeFromProject The project where the build will be resumed with in the next run.
+ * @return An optional containing a comma separated list of projects which can be skipped,
+ * or an empty optional if no projects can be skipped.
+ */
+ private Optional<String> determineProjectsToSkip( MavenExecutionResult result, List<MavenProject> failedProjects,
+ MavenProject resumeFromProject )
+ {
+ List<MavenProject> allProjects = result.getTopologicallySortedProjects();
+ int resumeFromProjectIndex = allProjects.indexOf( resumeFromProject );
+ List<MavenProject> remainingProjects = allProjects.subList( resumeFromProjectIndex + 1, allProjects.size() );
+
+ List<GroupArtifactPair> failedProjectsGAList = failedProjects.stream()
+ .map( GroupArtifactPair::new )
+ .collect( Collectors.toList() );
+
+ String projectsToSkip = remainingProjects.stream()
+ .filter( project -> result.getBuildSummary( project ) instanceof BuildSuccess )
+ .filter( project -> hasNoDependencyOnProjects( project, failedProjectsGAList ) )
+ .map( project -> String.format( "%s:%s", project.getGroupId(), project.getArtifactId() ) )
+ .collect( Collectors.joining( PROPERTY_DELIMITER ) );
+
+ if ( !StringUtils.isEmpty( projectsToSkip ) )
+ {
+ return Optional.of( projectsToSkip );
+ }
+
+ return Optional.empty();
+ }
+
+ private boolean hasNoDependencyOnProjects( MavenProject project, List<GroupArtifactPair> projectsGAs )
+ {
+ return project.getDependencies().stream()
+ .map( GroupArtifactPair::new )
+ .noneMatch( projectsGAs::contains );
+ }
+
+ private boolean writeResumptionFile( MavenProject rootProject, Properties properties )
+ {
+ Path resumeProperties = Paths.get( rootProject.getBuild().getDirectory(), RESUME_PROPERTIES_FILENAME );
+ try
+ {
+ Files.createDirectories( resumeProperties.getParent() );
+ try ( Writer writer = Files.newBufferedWriter( resumeProperties ) )
+ {
+ properties.store( writer, null );
+ }
+ }
+ catch ( IOException e )
+ {
+ logger.warn( "Could not create " + RESUME_PROPERTIES_FILENAME + " file. ", e );
+ return false;
+ }
+
+ return true;
+ }
+
+ private Properties loadResumptionFile( String rootBuildDirectory )
Review comment:
In general, if you know a String is actually something else (like a `Path`), please pass it as the dedicated instance.
----------------------------------------------------------------
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
[GitHub] [maven] MartinKanters commented on a change in pull request #342: [MNG-5760] Add --resume / -r switch
Posted by GitBox <gi...@apache.org>.
MartinKanters commented on a change in pull request #342:
URL: https://github.com/apache/maven/pull/342#discussion_r429748772
##########
File path: maven-core/src/main/java/org/apache/maven/DefaultMaven.java
##########
@@ -366,20 +366,21 @@ private void afterSessionEnd( Collection<MavenProject> projects, MavenSession se
private void saveResumptionDataWhenApplicable( MavenExecutionResult result, MavenSession session )
{
- List<LifecycleExecutionException> lifecycleExecutionExceptions = result.getExceptions().stream()
+ long lifecycleExecutionExceptionCount = result.getExceptions().stream()
.filter( LifecycleExecutionException.class::isInstance )
- .map( LifecycleExecutionException.class::cast )
- .collect( Collectors.toList() );
+ .count();
Review comment:
We think this is the most readable way of counting those specific exceptions. Rewriting it in a for-loop could potentially save some resources (if javac does not already optimize the stream for us). We usually go for easy-to-read code first, and optimize when needed.
That said, I am not sure if the Maven codebase has a different strategy? Maybe @rfscholte or someone else can chip in?
----------------------------------------------------------------
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
[GitHub] [maven] MartinKanters commented on a change in pull request #342: [MNG-5760] Add --resume / -r switch
Posted by GitBox <gi...@apache.org>.
MartinKanters commented on a change in pull request #342:
URL: https://github.com/apache/maven/pull/342#discussion_r429748977
##########
File path: maven-core/src/main/java/org/apache/maven/execution/DefaultBuildResumptionManager.java
##########
@@ -0,0 +1,308 @@
+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;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.maven.lifecycle.LifecycleExecutionException;
+import org.apache.maven.model.Dependency;
+import org.apache.maven.project.MavenProject;
+import org.codehaus.plexus.logging.Logger;
+
+import javax.inject.Inject;
+import javax.inject.Named;
+import javax.inject.Singleton;
+import java.io.IOException;
+import java.io.Reader;
+import java.io.Writer;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.Paths;
+import java.util.Arrays;
+import java.util.List;
+import java.util.Objects;
+import java.util.Optional;
+import java.util.Properties;
+import java.util.stream.Collectors;
+
+import static java.util.Comparator.comparing;
+
+/**
+ * This implementation of {@link BuildResumptionManager} persists information in a properties file. The file is stored
+ * in the build output directory under the Maven execution root.
+ */
+@Named
+@Singleton
+public class DefaultBuildResumptionManager implements BuildResumptionManager
+{
+ private static final String RESUME_PROPERTIES_FILENAME = "resume.properties";
+ private static final String RESUME_FROM_PROPERTY = "resumeFrom";
+ private static final String EXCLUDED_PROJECTS_PROPERTY = "excludedProjects";
+ private static final String PROPERTY_DELIMITER = ", ";
+
+ @Inject
+ private Logger logger;
Review comment:
Sounds like a good refactor indeed!
----------------------------------------------------------------
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
[GitHub] [maven] MartinKanters edited a comment on pull request #342: [MNG-5760] Add --resume / -r switch
Posted by GitBox <gi...@apache.org>.
MartinKanters edited a comment on pull request #342:
URL: https://github.com/apache/maven/pull/342#issuecomment-633401173
> how about a simple `canResume()` (who knows if there will be other requirements to resume)?
Sounds good, Robert. We will change this name, indeed.
----------------------------------------------------------------
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
[GitHub] [maven] rfscholte closed pull request #342: [MNG-5760] Add --resume / -r switch
Posted by GitBox <gi...@apache.org>.
rfscholte closed pull request #342:
URL: https://github.com/apache/maven/pull/342
----------------------------------------------------------------
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
[GitHub] [maven] MartinKanters commented on a change in pull request #342: [MNG-5760] Add --resume / -r switch
Posted by GitBox <gi...@apache.org>.
MartinKanters commented on a change in pull request #342:
URL: https://github.com/apache/maven/pull/342#discussion_r429750167
##########
File path: maven-core/src/main/java/org/apache/maven/execution/DefaultBuildResumptionManager.java
##########
@@ -0,0 +1,308 @@
+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;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.maven.lifecycle.LifecycleExecutionException;
+import org.apache.maven.model.Dependency;
+import org.apache.maven.project.MavenProject;
+import org.codehaus.plexus.logging.Logger;
+
+import javax.inject.Inject;
+import javax.inject.Named;
+import javax.inject.Singleton;
+import java.io.IOException;
+import java.io.Reader;
+import java.io.Writer;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.Paths;
+import java.util.Arrays;
+import java.util.List;
+import java.util.Objects;
+import java.util.Optional;
+import java.util.Properties;
+import java.util.stream.Collectors;
+
+import static java.util.Comparator.comparing;
+
+/**
+ * This implementation of {@link BuildResumptionManager} persists information in a properties file. The file is stored
+ * in the build output directory under the Maven execution root.
+ */
+@Named
+@Singleton
+public class DefaultBuildResumptionManager implements BuildResumptionManager
+{
+ private static final String RESUME_PROPERTIES_FILENAME = "resume.properties";
+ private static final String RESUME_FROM_PROPERTY = "resumeFrom";
+ private static final String EXCLUDED_PROJECTS_PROPERTY = "excludedProjects";
+ private static final String PROPERTY_DELIMITER = ", ";
+
+ @Inject
+ private Logger logger;
+
+ @Override
+ public boolean persistResumptionData( MavenExecutionResult result, MavenProject rootProject )
+ {
+ Properties properties = determineResumptionProperties( result );
+
+ if ( properties.isEmpty() )
+ {
+ logger.debug( "Will not create " + RESUME_PROPERTIES_FILENAME + " file: nothing to resume from" );
+ return false;
+ }
+
+ return writeResumptionFile( rootProject, properties );
+ }
+
+ @Override
+ public void applyResumptionData( MavenExecutionRequest request, MavenProject rootProject )
+ {
+ Properties properties = loadResumptionFile( rootProject.getBuild().getDirectory() );
+ applyResumptionProperties( request, properties );
+ }
+
+ @Override
+ public void removeResumptionData( MavenProject rootProject )
+ {
+ Path resumeProperties = Paths.get( rootProject.getBuild().getDirectory(), RESUME_PROPERTIES_FILENAME );
+ try
+ {
+ Files.deleteIfExists( resumeProperties );
+ }
+ catch ( IOException e )
+ {
+ logger.warn( "Could not delete " + RESUME_PROPERTIES_FILENAME + " file. ", e );
+ }
+ }
+
+ @Override
+ public String getResumeFromSelector( List<MavenProject> mavenProjects, MavenProject failedProject )
+ {
+ boolean hasOverlappingArtifactId = mavenProjects.stream()
+ .filter( project -> failedProject.getArtifactId().equals( project.getArtifactId() ) )
+ .count() > 1;
+
+ if ( hasOverlappingArtifactId )
+ {
+ return failedProject.getGroupId() + ":" + failedProject.getArtifactId();
+ }
+
+ return ":" + failedProject.getArtifactId();
+ }
+
+ @VisibleForTesting
+ Properties determineResumptionProperties( MavenExecutionResult result )
+ {
+ Properties properties = new Properties();
+
+ List<MavenProject> failedProjects = getFailedProjectsInOrder( result );
+ if ( !failedProjects.isEmpty() )
+ {
+ MavenProject resumeFromProject = failedProjects.get( 0 );
+ Optional<String> resumeFrom = getResumeFrom( result, resumeFromProject );
+ Optional<String> projectsToSkip = determineProjectsToSkip( result, failedProjects, resumeFromProject );
+
+ resumeFrom.ifPresent( value -> properties.setProperty( RESUME_FROM_PROPERTY, value ) );
+ projectsToSkip.ifPresent( value -> properties.setProperty( EXCLUDED_PROJECTS_PROPERTY, value ) );
+ }
+ else
+ {
+ logger.warn( "Could not create " + RESUME_PROPERTIES_FILENAME + " file: no failed projects found" );
+ }
+
+ return properties;
+ }
+
+ private List<MavenProject> getFailedProjectsInOrder( MavenExecutionResult result )
+ {
+ List<MavenProject> sortedProjects = result.getTopologicallySortedProjects();
+
+ return result.getExceptions().stream()
+ .filter( LifecycleExecutionException.class::isInstance )
+ .map( LifecycleExecutionException.class::cast )
+ .map( LifecycleExecutionException::getProject )
+ .sorted( comparing( sortedProjects::indexOf ) )
+ .collect( Collectors.toList() );
+ }
+
+ /**
+ * Determine the project where the next build can be resumed from.
+ * If the failed project is the first project of the build,
+ * it does not make sense to use --resume-from, so the result will be empty.
+ * @param result The result of the Maven build.
+ * @param failedProject The first failed project of the build.
+ * @return An optional containing the resume-from suggestion.
+ */
+ private Optional<String> getResumeFrom( MavenExecutionResult result, MavenProject failedProject )
+ {
+ List<MavenProject> allSortedProjects = result.getTopologicallySortedProjects();
+ if ( !allSortedProjects.get( 0 ).equals( failedProject ) )
+ {
+ return Optional.of( String.format( "%s:%s", failedProject.getGroupId(), failedProject.getArtifactId() ) );
Review comment:
I'm fine to replace format with string concat, I don't think it matters much in the sake of readability.
The inconsistency of `getResumeFromSelector()` is on purpose. I think that method was designed in order for the user to get the most short-hand hint to "resume-from" the build with. In this case the user does not have to see this selector at all, so we can use the full unique key (group+artifact id). The added benefit is that we do not have to calculate whether an artifact-id alone is unique in the whole project.
##########
File path: maven-core/src/main/java/org/apache/maven/execution/DefaultBuildResumptionManager.java
##########
@@ -0,0 +1,308 @@
+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;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.maven.lifecycle.LifecycleExecutionException;
+import org.apache.maven.model.Dependency;
+import org.apache.maven.project.MavenProject;
+import org.codehaus.plexus.logging.Logger;
+
+import javax.inject.Inject;
+import javax.inject.Named;
+import javax.inject.Singleton;
+import java.io.IOException;
+import java.io.Reader;
+import java.io.Writer;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.Paths;
+import java.util.Arrays;
+import java.util.List;
+import java.util.Objects;
+import java.util.Optional;
+import java.util.Properties;
+import java.util.stream.Collectors;
+
+import static java.util.Comparator.comparing;
+
+/**
+ * This implementation of {@link BuildResumptionManager} persists information in a properties file. The file is stored
+ * in the build output directory under the Maven execution root.
+ */
+@Named
+@Singleton
+public class DefaultBuildResumptionManager implements BuildResumptionManager
+{
+ private static final String RESUME_PROPERTIES_FILENAME = "resume.properties";
+ private static final String RESUME_FROM_PROPERTY = "resumeFrom";
+ private static final String EXCLUDED_PROJECTS_PROPERTY = "excludedProjects";
+ private static final String PROPERTY_DELIMITER = ", ";
+
+ @Inject
+ private Logger logger;
+
+ @Override
+ public boolean persistResumptionData( MavenExecutionResult result, MavenProject rootProject )
+ {
+ Properties properties = determineResumptionProperties( result );
+
+ if ( properties.isEmpty() )
+ {
+ logger.debug( "Will not create " + RESUME_PROPERTIES_FILENAME + " file: nothing to resume from" );
+ return false;
+ }
+
+ return writeResumptionFile( rootProject, properties );
+ }
+
+ @Override
+ public void applyResumptionData( MavenExecutionRequest request, MavenProject rootProject )
+ {
+ Properties properties = loadResumptionFile( rootProject.getBuild().getDirectory() );
+ applyResumptionProperties( request, properties );
+ }
+
+ @Override
+ public void removeResumptionData( MavenProject rootProject )
+ {
+ Path resumeProperties = Paths.get( rootProject.getBuild().getDirectory(), RESUME_PROPERTIES_FILENAME );
+ try
+ {
+ Files.deleteIfExists( resumeProperties );
+ }
+ catch ( IOException e )
+ {
+ logger.warn( "Could not delete " + RESUME_PROPERTIES_FILENAME + " file. ", e );
+ }
+ }
+
+ @Override
+ public String getResumeFromSelector( List<MavenProject> mavenProjects, MavenProject failedProject )
+ {
+ boolean hasOverlappingArtifactId = mavenProjects.stream()
+ .filter( project -> failedProject.getArtifactId().equals( project.getArtifactId() ) )
+ .count() > 1;
+
+ if ( hasOverlappingArtifactId )
+ {
+ return failedProject.getGroupId() + ":" + failedProject.getArtifactId();
+ }
+
+ return ":" + failedProject.getArtifactId();
+ }
+
+ @VisibleForTesting
+ Properties determineResumptionProperties( MavenExecutionResult result )
+ {
+ Properties properties = new Properties();
+
+ List<MavenProject> failedProjects = getFailedProjectsInOrder( result );
+ if ( !failedProjects.isEmpty() )
+ {
+ MavenProject resumeFromProject = failedProjects.get( 0 );
+ Optional<String> resumeFrom = getResumeFrom( result, resumeFromProject );
+ Optional<String> projectsToSkip = determineProjectsToSkip( result, failedProjects, resumeFromProject );
+
+ resumeFrom.ifPresent( value -> properties.setProperty( RESUME_FROM_PROPERTY, value ) );
+ projectsToSkip.ifPresent( value -> properties.setProperty( EXCLUDED_PROJECTS_PROPERTY, value ) );
+ }
+ else
+ {
+ logger.warn( "Could not create " + RESUME_PROPERTIES_FILENAME + " file: no failed projects found" );
+ }
+
+ return properties;
+ }
+
+ private List<MavenProject> getFailedProjectsInOrder( MavenExecutionResult result )
+ {
+ List<MavenProject> sortedProjects = result.getTopologicallySortedProjects();
+
+ return result.getExceptions().stream()
+ .filter( LifecycleExecutionException.class::isInstance )
+ .map( LifecycleExecutionException.class::cast )
+ .map( LifecycleExecutionException::getProject )
+ .sorted( comparing( sortedProjects::indexOf ) )
+ .collect( Collectors.toList() );
+ }
+
+ /**
+ * Determine the project where the next build can be resumed from.
+ * If the failed project is the first project of the build,
+ * it does not make sense to use --resume-from, so the result will be empty.
+ * @param result The result of the Maven build.
+ * @param failedProject The first failed project of the build.
+ * @return An optional containing the resume-from suggestion.
+ */
+ private Optional<String> getResumeFrom( MavenExecutionResult result, MavenProject failedProject )
+ {
+ List<MavenProject> allSortedProjects = result.getTopologicallySortedProjects();
+ if ( !allSortedProjects.get( 0 ).equals( failedProject ) )
+ {
+ return Optional.of( String.format( "%s:%s", failedProject.getGroupId(), failedProject.getArtifactId() ) );
Review comment:
I'm fine to replace format with string concat, I don't think it matters much in the sake of readability.
The inconsistency of `getResumeFromSelector()` is on purpose. I think that method was designed in order for the user to get the most short-hand hint to "resume-from" the build with. In this case the user does not have to see this selector at all, so we can use the full unique key (group+artifact id). The benefit is that we do not have to calculate whether an artifact-id alone is unique in the whole project.
----------------------------------------------------------------
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
[GitHub] [maven] MartinKanters commented on a change in pull request #342: [MNG-5760] Add --resume / -r switch
Posted by GitBox <gi...@apache.org>.
MartinKanters commented on a change in pull request #342:
URL: https://github.com/apache/maven/pull/342#discussion_r429751573
##########
File path: maven-core/src/main/java/org/apache/maven/execution/DefaultBuildResumptionManager.java
##########
@@ -0,0 +1,308 @@
+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;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.maven.lifecycle.LifecycleExecutionException;
+import org.apache.maven.model.Dependency;
+import org.apache.maven.project.MavenProject;
+import org.codehaus.plexus.logging.Logger;
+
+import javax.inject.Inject;
+import javax.inject.Named;
+import javax.inject.Singleton;
+import java.io.IOException;
+import java.io.Reader;
+import java.io.Writer;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.Paths;
+import java.util.Arrays;
+import java.util.List;
+import java.util.Objects;
+import java.util.Optional;
+import java.util.Properties;
+import java.util.stream.Collectors;
+
+import static java.util.Comparator.comparing;
+
+/**
+ * This implementation of {@link BuildResumptionManager} persists information in a properties file. The file is stored
+ * in the build output directory under the Maven execution root.
+ */
+@Named
+@Singleton
+public class DefaultBuildResumptionManager implements BuildResumptionManager
+{
+ private static final String RESUME_PROPERTIES_FILENAME = "resume.properties";
+ private static final String RESUME_FROM_PROPERTY = "resumeFrom";
+ private static final String EXCLUDED_PROJECTS_PROPERTY = "excludedProjects";
+ private static final String PROPERTY_DELIMITER = ", ";
+
+ @Inject
+ private Logger logger;
+
+ @Override
+ public boolean persistResumptionData( MavenExecutionResult result, MavenProject rootProject )
+ {
+ Properties properties = determineResumptionProperties( result );
+
+ if ( properties.isEmpty() )
+ {
+ logger.debug( "Will not create " + RESUME_PROPERTIES_FILENAME + " file: nothing to resume from" );
+ return false;
+ }
+
+ return writeResumptionFile( rootProject, properties );
+ }
+
+ @Override
+ public void applyResumptionData( MavenExecutionRequest request, MavenProject rootProject )
+ {
+ Properties properties = loadResumptionFile( rootProject.getBuild().getDirectory() );
+ applyResumptionProperties( request, properties );
+ }
+
+ @Override
+ public void removeResumptionData( MavenProject rootProject )
+ {
+ Path resumeProperties = Paths.get( rootProject.getBuild().getDirectory(), RESUME_PROPERTIES_FILENAME );
+ try
+ {
+ Files.deleteIfExists( resumeProperties );
+ }
+ catch ( IOException e )
+ {
+ logger.warn( "Could not delete " + RESUME_PROPERTIES_FILENAME + " file. ", e );
+ }
+ }
+
+ @Override
+ public String getResumeFromSelector( List<MavenProject> mavenProjects, MavenProject failedProject )
+ {
+ boolean hasOverlappingArtifactId = mavenProjects.stream()
+ .filter( project -> failedProject.getArtifactId().equals( project.getArtifactId() ) )
+ .count() > 1;
+
+ if ( hasOverlappingArtifactId )
+ {
+ return failedProject.getGroupId() + ":" + failedProject.getArtifactId();
+ }
+
+ return ":" + failedProject.getArtifactId();
+ }
+
+ @VisibleForTesting
+ Properties determineResumptionProperties( MavenExecutionResult result )
+ {
+ Properties properties = new Properties();
+
+ List<MavenProject> failedProjects = getFailedProjectsInOrder( result );
+ if ( !failedProjects.isEmpty() )
+ {
+ MavenProject resumeFromProject = failedProjects.get( 0 );
+ Optional<String> resumeFrom = getResumeFrom( result, resumeFromProject );
+ Optional<String> projectsToSkip = determineProjectsToSkip( result, failedProjects, resumeFromProject );
+
+ resumeFrom.ifPresent( value -> properties.setProperty( RESUME_FROM_PROPERTY, value ) );
+ projectsToSkip.ifPresent( value -> properties.setProperty( EXCLUDED_PROJECTS_PROPERTY, value ) );
+ }
+ else
+ {
+ logger.warn( "Could not create " + RESUME_PROPERTIES_FILENAME + " file: no failed projects found" );
+ }
+
+ return properties;
+ }
+
+ private List<MavenProject> getFailedProjectsInOrder( MavenExecutionResult result )
+ {
+ List<MavenProject> sortedProjects = result.getTopologicallySortedProjects();
+
+ return result.getExceptions().stream()
+ .filter( LifecycleExecutionException.class::isInstance )
+ .map( LifecycleExecutionException.class::cast )
+ .map( LifecycleExecutionException::getProject )
+ .sorted( comparing( sortedProjects::indexOf ) )
+ .collect( Collectors.toList() );
+ }
+
+ /**
+ * Determine the project where the next build can be resumed from.
+ * If the failed project is the first project of the build,
+ * it does not make sense to use --resume-from, so the result will be empty.
+ * @param result The result of the Maven build.
+ * @param failedProject The first failed project of the build.
+ * @return An optional containing the resume-from suggestion.
+ */
+ private Optional<String> getResumeFrom( MavenExecutionResult result, MavenProject failedProject )
+ {
+ List<MavenProject> allSortedProjects = result.getTopologicallySortedProjects();
+ if ( !allSortedProjects.get( 0 ).equals( failedProject ) )
+ {
+ return Optional.of( String.format( "%s:%s", failedProject.getGroupId(), failedProject.getArtifactId() ) );
+ }
+
+ return Optional.empty();
+ }
+
+ /**
+ * Projects after the first failed project could have succeeded by using -T or --fail-at-end.
+ * These projects can be skipped from later builds.
+ * This is not the case these projects are dependent on one of the failed projects.
+ * @param result The result of the Maven build.
+ * @param failedProjects The list of failed projects in the build.
+ * @param resumeFromProject The project where the build will be resumed with in the next run.
+ * @return An optional containing a comma separated list of projects which can be skipped,
+ * or an empty optional if no projects can be skipped.
+ */
+ private Optional<String> determineProjectsToSkip( MavenExecutionResult result, List<MavenProject> failedProjects,
+ MavenProject resumeFromProject )
+ {
+ List<MavenProject> allProjects = result.getTopologicallySortedProjects();
+ int resumeFromProjectIndex = allProjects.indexOf( resumeFromProject );
+ List<MavenProject> remainingProjects = allProjects.subList( resumeFromProjectIndex + 1, allProjects.size() );
+
+ List<GroupArtifactPair> failedProjectsGAList = failedProjects.stream()
+ .map( GroupArtifactPair::new )
+ .collect( Collectors.toList() );
+
+ String projectsToSkip = remainingProjects.stream()
+ .filter( project -> result.getBuildSummary( project ) instanceof BuildSuccess )
+ .filter( project -> hasNoDependencyOnProjects( project, failedProjectsGAList ) )
+ .map( project -> String.format( "%s:%s", project.getGroupId(), project.getArtifactId() ) )
+ .collect( Collectors.joining( PROPERTY_DELIMITER ) );
+
+ if ( !StringUtils.isEmpty( projectsToSkip ) )
+ {
+ return Optional.of( projectsToSkip );
+ }
+
+ return Optional.empty();
+ }
+
+ private boolean hasNoDependencyOnProjects( MavenProject project, List<GroupArtifactPair> projectsGAs )
+ {
+ return project.getDependencies().stream()
+ .map( GroupArtifactPair::new )
+ .noneMatch( projectsGAs::contains );
+ }
+
+ private boolean writeResumptionFile( MavenProject rootProject, Properties properties )
+ {
+ Path resumeProperties = Paths.get( rootProject.getBuild().getDirectory(), RESUME_PROPERTIES_FILENAME );
+ try
+ {
+ Files.createDirectories( resumeProperties.getParent() );
+ try ( Writer writer = Files.newBufferedWriter( resumeProperties ) )
+ {
+ properties.store( writer, null );
+ }
+ }
+ catch ( IOException e )
+ {
+ logger.warn( "Could not create " + RESUME_PROPERTIES_FILENAME + " file. ", e );
+ return false;
+ }
+
+ return true;
+ }
+
+ private Properties loadResumptionFile( String rootBuildDirectory )
Review comment:
I agree, this would be nicer when they are consistent. Thanks!
----------------------------------------------------------------
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
[GitHub] [maven] michael-o commented on a change in pull request #342: [MNG-5760] Add --resume / -r switch
Posted by GitBox <gi...@apache.org>.
michael-o commented on a change in pull request #342:
URL: https://github.com/apache/maven/pull/342#discussion_r429572738
##########
File path: maven-core/src/main/java/org/apache/maven/DefaultMaven.java
##########
@@ -366,20 +366,21 @@ private void afterSessionEnd( Collection<MavenProject> projects, MavenSession se
private void saveResumptionDataWhenApplicable( MavenExecutionResult result, MavenSession session )
{
- List<LifecycleExecutionException> lifecycleExecutionExceptions = result.getExceptions().stream()
+ long lifecycleExecutionExceptionCount = result.getExceptions().stream()
.filter( LifecycleExecutionException.class::isInstance )
- .map( LifecycleExecutionException.class::cast )
- .collect( Collectors.toList() );
+ .count();
Review comment:
Why do we need a stream to count elements?
----------------------------------------------------------------
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
[GitHub] [maven] rfscholte commented on a change in pull request #342: [MNG-5760] Add --resume / -r switch
Posted by GitBox <gi...@apache.org>.
rfscholte commented on a change in pull request #342:
URL: https://github.com/apache/maven/pull/342#discussion_r429804991
##########
File path: maven-core/src/main/java/org/apache/maven/DefaultMaven.java
##########
@@ -366,20 +366,21 @@ private void afterSessionEnd( Collection<MavenProject> projects, MavenSession se
private void saveResumptionDataWhenApplicable( MavenExecutionResult result, MavenSession session )
{
- List<LifecycleExecutionException> lifecycleExecutionExceptions = result.getExceptions().stream()
+ long lifecycleExecutionExceptionCount = result.getExceptions().stream()
.filter( LifecycleExecutionException.class::isInstance )
- .map( LifecycleExecutionException.class::cast )
- .collect( Collectors.toList() );
+ .count();
Review comment:
The next codeblock needs to know if there were lifecycleExecutionException. That said, the amount doesn't matter, a simple `boolean` would be sufficient. A count needs to loop over all exceptions, whereas replacing `filter` with `anyMatch` would be the optimized implementation.
----------------------------------------------------------------
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