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 2021/02/12 11:04:43 UTC
[GitHub] [maven] mthmulders commented on a change in pull request #444: [MNG-7095] Fix resume for parallel builds
mthmulders commented on a change in pull request #444:
URL: https://github.com/apache/maven/pull/444#discussion_r575106537
##########
File path: maven-core/src/main/java/org/apache/maven/execution/BuildResumptionData.java
##########
@@ -32,36 +29,20 @@
/**
* The project where the next build could resume from.
*/
- private final String resumeFrom;
-
- /**
- * List of projects to skip.
- */
- private final List<String> projectsToSkip;
+ private final List<String> projectList;
Review comment:
The name of the variable is a bit vague, it doesn't clearly indicate what kind of projects it contains. How about `remainingProjects`? Also, the Javadoc is outdated - the Javadoc of the corresponding `getProjectList()` looks a lot better.
##########
File path: maven-core/src/main/java/org/apache/maven/execution/BuildResumptionData.java
##########
@@ -32,36 +29,20 @@
/**
* The project where the next build could resume from.
*/
- private final String resumeFrom;
-
- /**
- * List of projects to skip.
- */
- private final List<String> projectsToSkip;
+ private final List<String> projectList;
- public BuildResumptionData ( final String resumeFrom, final List<String> projectsToSkip )
+ public BuildResumptionData ( final List<String> projectList )
{
- this.resumeFrom = resumeFrom;
- this.projectsToSkip = projectsToSkip;
+ this.projectList = projectList;
}
/**
- * Returns the project where the next build can resume from.
- * This is usually the first failed project in the order of the reactor.
- * @return An optional containing the group and artifact id of the project. It does not make sense to resume
- * the build when the first project of the reactor has failed, so then it will return an empty optional.
+ * Returns the projects that still need to be build when resuming.
Review comment:
```suggestion
* Returns the projects that still need to be built when resuming.
```
##########
File path: maven-embedder/src/main/java/org/apache/maven/cli/MavenCli.java
##########
@@ -1032,20 +1031,6 @@ private int execute( CliRequest cliRequest )
{
logBuildResumeHint( "mvn <args> -r" );
}
- else if ( !failedProjects.isEmpty() )
Review comment:
I don't understand why you removed this block.
##########
File path: maven-core/src/test/java/org/apache/maven/execution/DefaultBuildResumptionAnalyzerTest.java
##########
@@ -133,6 +129,12 @@ private Dependency toDependency(MavenProject mavenProject )
return dependency;
}
+ private MavenProject createSkippedMavenProject( String artifactId )
+ {
+ MavenProject project = createMavenProject( artifactId );
Review comment:
Method body can be one line as the outcome of `createMavenProject` is not changed.
##########
File path: maven-core/src/main/java/org/apache/maven/execution/DefaultBuildResumptionAnalyzer.java
##########
@@ -47,132 +41,49 @@
@Override
public Optional<BuildResumptionData> determineBuildResumptionData( final MavenExecutionResult result )
{
- final List<MavenProject> failedProjects = getFailedProjectsInOrder( result );
-
- if ( failedProjects.isEmpty() )
+ if ( !result.hasExceptions() )
{
- LOGGER.info( "No failed projects found, resuming the build would not make sense." );
return Optional.empty();
}
- final MavenProject resumeFromProject = failedProjects.get( 0 );
-
- final String resumeFromSelector;
- final List<String> projectsToSkip;
- if ( isFailedProjectFirstInBuild( result, resumeFromProject ) )
- {
- // As the first module in the build failed, there is no need to specify this as the resumeFrom project.
- resumeFromSelector = null;
- projectsToSkip = determineProjectsToSkip( result, failedProjects, 0 );
- }
- else
- {
- resumeFromSelector = resumeFromProject.getGroupId() + ":" + resumeFromProject.getArtifactId();
- List<MavenProject> allProjects = result.getTopologicallySortedProjects();
- int resumeFromProjectIndex = allProjects.indexOf( resumeFromProject );
- projectsToSkip = determineProjectsToSkip( result, failedProjects, resumeFromProjectIndex + 1 );
- }
-
- boolean canBuildBeResumed = StringUtils.isNotEmpty( resumeFromSelector ) || !projectsToSkip.isEmpty();
- if ( canBuildBeResumed )
- {
- return Optional.of( new BuildResumptionData( resumeFromSelector, projectsToSkip ) );
- }
- else
- {
- return Optional.empty();
- }
- }
-
- private boolean isFailedProjectFirstInBuild( final MavenExecutionResult result, final MavenProject failedProject )
- {
- final List<MavenProject> sortedProjects = result.getTopologicallySortedProjects();
- return sortedProjects.indexOf( failedProject ) == 0;
- }
-
- 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 )
- .filter( Objects::nonNull )
- .sorted( comparing( sortedProjects::indexOf ) )
- .collect( Collectors.toList() );
- }
-
- /**
- * 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 startFromProjectIndex Start looking for projects which can be skipped from a certain index.
- * @return A list of projects which can be skipped in a later build.
- */
- private List<String> determineProjectsToSkip( MavenExecutionResult result,
- List<MavenProject> failedProjects,
- int startFromProjectIndex )
- {
- List<MavenProject> allProjects = result.getTopologicallySortedProjects();
- List<MavenProject> remainingProjects = allProjects.subList( startFromProjectIndex, allProjects.size() );
-
- List<GroupArtifactPair> failedProjectsGAList = failedProjects.stream()
- .map( GroupArtifactPair::new )
- .collect( Collectors.toList() );
-
- return remainingProjects.stream()
+ long succeeded = sortedProjects.stream()
.filter( project -> result.getBuildSummary( project ) instanceof BuildSuccess )
- .filter( project -> hasNoDependencyOnProjects( project, failedProjectsGAList ) )
- .map( project -> project.getGroupId() + ":" + project.getArtifactId() )
- .collect( Collectors.toList() );
- }
-
- private boolean hasNoDependencyOnProjects( MavenProject project, List<GroupArtifactPair> projectsGAs )
- {
- return project.getDependencies().stream()
- .map( GroupArtifactPair::new )
- .noneMatch( projectsGAs::contains );
- }
-
- private static class GroupArtifactPair
- {
- private final String groupId;
- private final String artifactId;
-
- GroupArtifactPair( MavenProject project )
- {
- this.groupId = project.getGroupId();
- this.artifactId = project.getArtifactId();
- }
+ .count();
- GroupArtifactPair( Dependency dependency )
+ if ( succeeded == 0 )
{
- this.groupId = dependency.getGroupId();
- this.artifactId = dependency.getArtifactId();
+ return Optional.empty();
}
- @Override
- public boolean equals( Object o )
+ List<MavenProject> projects = sortedProjects.stream()
Review comment:
Maybe `notSucceededProjects` makes more sense? Now the only way to understand which projects are in the list is to interpret line 60 - 63 using brainpower :-).
##########
File path: maven-core/src/main/java/org/apache/maven/execution/DefaultBuildResumptionDataRepository.java
##########
@@ -133,22 +126,14 @@ private Properties loadResumptionFile( Path rootBuildDirectory )
// This method is made package-private for testing purposes
void applyResumptionProperties( MavenExecutionRequest request, Properties properties )
{
- if ( properties.containsKey( RESUME_FROM_PROPERTY ) && StringUtils.isEmpty( request.getResumeFrom() ) )
+ if ( properties.containsKey( RESUME_PROJECT_LIST )
+ && StringUtils.isEmpty( request.getResumeFrom() ) )
{
- String propertyValue = properties.getProperty( RESUME_FROM_PROPERTY );
- request.setResumeFrom( propertyValue );
+ String propertyValue = properties.getProperty( RESUME_PROJECT_LIST );
+ Stream.of( propertyValue.split( PROPERTY_DELIMITER ) )
+ .filter( StringUtils::isNotEmpty )
+ .forEach( request.getSelectedProjects():: add );
Review comment:
What if the project from the **resume.properties** file was already selected by the user using `-pl`? I think it'll end up in the `selectedProjects` twice, but I'm not sure whether/how Maven will handle that.
##########
File path: maven-core/src/main/java/org/apache/maven/execution/DefaultBuildResumptionDataRepository.java
##########
@@ -44,8 +44,7 @@
public class DefaultBuildResumptionDataRepository implements BuildResumptionDataRepository
{
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 RESUME_PROJECT_LIST = "projectList";
Review comment:
How about `remainingProjects`, to express what it contains, just like the variable inside the `BuildResumptionData` class? Could you at least align the name of the constant with the name of the property inside the file?
##########
File path: maven-core/src/test/java/org/apache/maven/execution/DefaultBuildResumptionAnalyzerTest.java
##########
@@ -81,22 +79,22 @@ public void projectsSucceedingAfterFailedProjectsAreExcluded()
Optional<BuildResumptionData> result = analyzer.determineBuildResumptionData( executionResult );
assertThat( result.isPresent(), is( true ) );
- assertThat( result.get().getProjectsToSkip(), contains( "test:C" ) );
+ assertThat( result.get().getProjectList(), is( asList( "test:B" ) ) );
}
@Test
public void projectsDependingOnFailedProjectsAreNotExcluded()
{
MavenProject projectA = createSucceededMavenProject( "A" );
MavenProject projectB = createFailedMavenProject( "B" );
- MavenProject projectC = createSucceededMavenProject( "C" );
+ MavenProject projectC = createSkippedMavenProject( "C" );
Review comment:
Good catch!
##########
File path: maven-core/src/main/java/org/apache/maven/execution/DefaultBuildResumptionAnalyzer.java
##########
@@ -47,132 +41,49 @@
@Override
public Optional<BuildResumptionData> determineBuildResumptionData( final MavenExecutionResult result )
{
- final List<MavenProject> failedProjects = getFailedProjectsInOrder( result );
-
- if ( failedProjects.isEmpty() )
+ if ( !result.hasExceptions() )
{
- LOGGER.info( "No failed projects found, resuming the build would not make sense." );
return Optional.empty();
}
- final MavenProject resumeFromProject = failedProjects.get( 0 );
-
- final String resumeFromSelector;
- final List<String> projectsToSkip;
- if ( isFailedProjectFirstInBuild( result, resumeFromProject ) )
- {
- // As the first module in the build failed, there is no need to specify this as the resumeFrom project.
- resumeFromSelector = null;
- projectsToSkip = determineProjectsToSkip( result, failedProjects, 0 );
- }
- else
- {
- resumeFromSelector = resumeFromProject.getGroupId() + ":" + resumeFromProject.getArtifactId();
- List<MavenProject> allProjects = result.getTopologicallySortedProjects();
- int resumeFromProjectIndex = allProjects.indexOf( resumeFromProject );
- projectsToSkip = determineProjectsToSkip( result, failedProjects, resumeFromProjectIndex + 1 );
- }
-
- boolean canBuildBeResumed = StringUtils.isNotEmpty( resumeFromSelector ) || !projectsToSkip.isEmpty();
- if ( canBuildBeResumed )
- {
- return Optional.of( new BuildResumptionData( resumeFromSelector, projectsToSkip ) );
- }
- else
- {
- return Optional.empty();
- }
- }
-
- private boolean isFailedProjectFirstInBuild( final MavenExecutionResult result, final MavenProject failedProject )
- {
- final List<MavenProject> sortedProjects = result.getTopologicallySortedProjects();
- return sortedProjects.indexOf( failedProject ) == 0;
- }
-
- 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 )
- .filter( Objects::nonNull )
- .sorted( comparing( sortedProjects::indexOf ) )
- .collect( Collectors.toList() );
- }
-
- /**
- * 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 startFromProjectIndex Start looking for projects which can be skipped from a certain index.
- * @return A list of projects which can be skipped in a later build.
- */
- private List<String> determineProjectsToSkip( MavenExecutionResult result,
- List<MavenProject> failedProjects,
- int startFromProjectIndex )
- {
- List<MavenProject> allProjects = result.getTopologicallySortedProjects();
- List<MavenProject> remainingProjects = allProjects.subList( startFromProjectIndex, allProjects.size() );
-
- List<GroupArtifactPair> failedProjectsGAList = failedProjects.stream()
- .map( GroupArtifactPair::new )
- .collect( Collectors.toList() );
-
- return remainingProjects.stream()
+ long succeeded = sortedProjects.stream()
.filter( project -> result.getBuildSummary( project ) instanceof BuildSuccess )
- .filter( project -> hasNoDependencyOnProjects( project, failedProjectsGAList ) )
- .map( project -> project.getGroupId() + ":" + project.getArtifactId() )
- .collect( Collectors.toList() );
- }
-
- private boolean hasNoDependencyOnProjects( MavenProject project, List<GroupArtifactPair> projectsGAs )
- {
- return project.getDependencies().stream()
- .map( GroupArtifactPair::new )
- .noneMatch( projectsGAs::contains );
- }
-
- private static class GroupArtifactPair
- {
- private final String groupId;
- private final String artifactId;
-
- GroupArtifactPair( MavenProject project )
- {
- this.groupId = project.getGroupId();
- this.artifactId = project.getArtifactId();
- }
+ .count();
- GroupArtifactPair( Dependency dependency )
+ if ( succeeded == 0 )
{
- this.groupId = dependency.getGroupId();
- this.artifactId = dependency.getArtifactId();
+ return Optional.empty();
}
- @Override
- public boolean equals( Object o )
+ List<MavenProject> projects = sortedProjects.stream()
+ .filter( project -> result.getBuildSummary( project ) == null
+ || result.getBuildSummary( project ) instanceof BuildFailure )
+ .collect( Collectors.toList() );
+ while ( true )
{
- if ( this == o )
+ List<MavenProject> children = projects.stream()
Review comment:
If I understand correctly, this block removes children from failed projects? I assume this is because, if a parent failed, its children will automatically be built as well? Maybe a small comment would've made sense... ;-)
Why is this needed? Did you consider the impact of the `--non-recursive` argument?
##########
File path: maven-core/src/test/java/org/apache/maven/execution/DefaultBuildResumptionDataRepositoryTest.java
##########
@@ -54,38 +55,38 @@ public void resumeFromPropertyDoesNotOverrideExistingRequestParameters()
MavenExecutionRequest request = new DefaultMavenExecutionRequest();
request.setResumeFrom( ":module-b" );
Properties properties = new Properties();
- properties.setProperty( "resumeFrom", ":module-a" );
+ properties.setProperty( "projectList", ":module-a" );
repository.applyResumptionProperties( request, properties );
assertThat( request.getResumeFrom(), is( ":module-b" ) );
}
@Test
- public void excludedProjectsFromPropertyGetsAddedToExistingRequestParameters()
+ public void projectsFromPropertyGetsAddedToExistingRequestParameters()
{
MavenExecutionRequest request = new DefaultMavenExecutionRequest();
- List<String> excludedProjects = new ArrayList<>();
- excludedProjects.add( ":module-a" );
- request.setExcludedProjects( excludedProjects );
+ List<String> selectedProjects = new ArrayList<>();
+ selectedProjects.add( ":module-a" );
+ request.setSelectedProjects( selectedProjects );
Properties properties = new Properties();
- properties.setProperty( "excludedProjects", ":module-b, :module-c" );
+ properties.setProperty( "projectList", ":module-b, :module-c" );
repository.applyResumptionProperties( request, properties );
- assertThat( request.getExcludedProjects(), contains( ":module-a", ":module-b", ":module-c" ) );
+ assertThat( request.getSelectedProjects(), contains( ":module-a", ":module-b", ":module-c" ) );
}
@Test
- public void excludedProjectsAreNotAddedWhenPropertyValueIsEmpty()
+ public void SELECTEDProjectsAreNotAddedWhenPropertyValueIsEmpty()
Review comment:
Please don't use capitals in the method name.
----------------------------------------------------------------
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