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/09 12:14:57 UTC
[GitHub] [maven] gnodet opened a new pull request #444: [MNG-7095] Fix resume for parallel builds
gnodet opened a new pull request #444:
URL: https://github.com/apache/maven/pull/444
Following this checklist to help us incorporate your
contribution quickly and easily:
- [ ] Make sure there is a [JIRA issue](https://issues.apache.org/jira/browse/MNG) filed
for the change (usually before you start working on it). Trivial changes like typos do not
require a JIRA issue. Your pull request should address just this issue, without
pulling in other changes.
- [ ] Each commit in the pull request should have a meaningful subject line and body.
- [ ] Format the pull request title like `[MNG-XXX] - Fixes bug in ApproximateQuantiles`,
where you replace `MNG-XXX` with the appropriate JIRA issue. Best practice
is to use the JIRA issue title in the pull request title and in the first line of the
commit message.
- [ ] Write a pull request description that 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.
If your pull request is about ~20 lines of code you don't need to sign an
[Individual Contributor License Agreement](https://www.apache.org/licenses/icla.pdf) if you are unsure
please ask on the developers list.
To make clear that you license your contribution under
the [Apache License Version 2.0, January 2004](http://www.apache.org/licenses/LICENSE-2.0)
you have to acknowledge this by using the following check-box.
- [ ] I hereby declare this contribution to be licenced under the [Apache License Version 2.0, January 2004](http://www.apache.org/licenses/LICENSE-2.0)
- [ ] In any other case, please file an [Apache Individual Contributor License Agreement](https://www.apache.org/licenses/icla.pdf).
[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 pull request #444: [MNG-7095] Fix resume for parallel builds
Posted by GitBox <gi...@apache.org>.
mthmulders commented on pull request #444:
URL: https://github.com/apache/maven/pull/444#issuecomment-775902301
@MartinKanters and I will be taking a look later this week.
----------------------------------------------------------------
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 #444: [MNG-7095] Fix resume for parallel builds
Posted by GitBox <gi...@apache.org>.
MartinKanters commented on a change in pull request #444:
URL: https://github.com/apache/maven/pull/444#discussion_r575186110
##########
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:
As far as I remember `--non-recursive` will result in `getCollectedProjects` to be empty (even if the pom has child modules). Actually --non-recursive will _only_ read the target pom (either ./pom.xml or the pom file selected via `-f pom.xml`, and none of it's child projects, I guess). So any of the selected projects will not take effect anyway.
My preference would go out to listing all modules in the BuildResumptionData regardless, because:
- the behavior in `DefaultGraphBuilder` could change,
- users will not typically open `resume.properties`, so we do not have to optimize the flag for readability,
- this `while(true)` loop adds complexity to the otherwise concise method.
Edit: I see you removed the code, I'll resolve the comment.
----------------------------------------------------------------
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 #444: [MNG-7095] Fix resume for parallel builds
Posted by GitBox <gi...@apache.org>.
MartinKanters edited a comment on pull request #444:
URL: https://github.com/apache/maven/pull/444#issuecomment-778412923
> > > @mthmulders @MartinKanters quick question.
> > > I'm working on improving the integration test and have the following scenario: a parent, 3 modules named `A`, `B`, `C` with `C` depending on `B` (that's the `mng-5760/parent-dependent` test project).
> > > I run the `test` goal on the project with `-T2` and with `A` and `C` failing, which means `B` succeeds. When resuming, `C` fails because it can not find the artifact `B`. That's because the goal run previously was `test` and not `install` so the artifact is not in the local repository. However, shouldn't maven be _aware_ that the project is in the local reactor somehow so that the build can succeed ?
> >
> >
> > @gnodet I would also expect that to succeed. Although module B hadn't been packaged in the earlier build, it does have a folder with compiled classes, and that should be enough. Can you isolate that behaviour into a separate test project so I can have a look at that later?
>
> I've pushed [apache/maven-integration-testing#98](https://github.com/apache/maven-integration-testing/pull/98) which contains 2 integration tests for the parallel build with the resume flag.
>
> I'll create another PR with an integration test for the other problem.
You've encountered the bug that MNG-4660 has fixed, but is not released in a main Maven release yet. With the latest Maven snapshot build you should not have to use install anymore.
I reviewed your ITs, they look good (apart from some small points).
----------------------------------------------------------------
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] gnodet commented on a change in pull request #444: [MNG-7095] Fix resume for parallel builds
Posted by GitBox <gi...@apache.org>.
gnodet commented on a change in pull request #444:
URL: https://github.com/apache/maven/pull/444#discussion_r575182290
##########
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:
That's clearly a typo !
----------------------------------------------------------------
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 #444: [MNG-7095] Fix resume for parallel builds
Posted by GitBox <gi...@apache.org>.
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
[GitHub] [maven] mthmulders commented on a change in pull request #444: [MNG-7095] Fix resume for parallel builds
Posted by GitBox <gi...@apache.org>.
mthmulders commented on a change in pull request #444:
URL: https://github.com/apache/maven/pull/444#discussion_r575230619
##########
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:
Just checked the combination of `-r` and `--non-recursive`, and it doesn't work:
> [INFO] Scanning for projects...
> [INFO] Resuming from org.apache.maven:maven-plugin-api, org.apache.maven:maven-xml, org.apache.maven:maven-model-builder, org.apache.maven:maven-settings-builder, org.apache.maven:maven-resolver-provider, org.apache.maven:maven-core, org.apache.maven:maven-slf4j-provider, org.apache.maven:maven-embedder, org.apache.maven:maven-compat, org.apache.maven:apache-maven, org.apache.maven:maven-wrapper, org.apache.maven:apache-maven-wrapper due to the --resume / -r feature.
> [ERROR] [ERROR] Could not find the selected project in the reactor: org.apache.maven:maven-plugin-api
This makes sense as the modules listed in **target/resume.properties** do not exist in the Reactor. `--non-recursive` effectively reduces the available projects to only the selected project, as @MartinKanters pointed out.
It's not something that's changing with these code changes, so let's keep it this way and see if the message is clear enough for end-users.
----------------------------------------------------------------
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 closed pull request #444: [MNG-7095] Fix resume for parallel builds
Posted by GitBox <gi...@apache.org>.
MartinKanters closed pull request #444:
URL: https://github.com/apache/maven/pull/444
----------------------------------------------------------------
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 #444: [MNG-7095] Fix resume for parallel builds
Posted by GitBox <gi...@apache.org>.
MartinKanters commented on a change in pull request #444:
URL: https://github.com/apache/maven/pull/444#discussion_r575186110
##########
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:
As far as I remember `--non-recursive` will result in `getCollectedProjects` to be empty (even if the pom has child modules). Actually --non-recursive will _only_ read the target pom (either ./pom.xml or the pom file selected via `-f pom.xml`, and none of it's child projects, I guess). So any of the selected projects will not take effect anyway.
My preference would go out to listing all modules in the BuildResumptionData regardless, because:
- the behavior in `DefaultGraphBuilder` could change,
- users will not typically open `resume.properties`, so we do not have to optimize the flag for readability,
- this `while(true)` loop adds complexity to the otherwise concise method.
----------------------------------------------------------------
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 #444: [MNG-7095] Fix resume for parallel builds
Posted by GitBox <gi...@apache.org>.
mthmulders commented on a change in pull request #444:
URL: https://github.com/apache/maven/pull/444#discussion_r575231427
##########
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:
Just checked, and including `-pl :xxx` where `xxx` is also in **target/resume.properties** builds that module once, as you'd hope/expect.
----------------------------------------------------------------
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 #444: [MNG-7095] Fix resume for parallel builds
Posted by GitBox <gi...@apache.org>.
MartinKanters commented on pull request #444:
URL: https://github.com/apache/maven/pull/444#issuecomment-778436709
Cool, your ITs are looking good right now, so the PR can be merged in my opinion. @mthmulders do you want to take another look? Otherwise I can merge somewhere this weekend.
----------------------------------------------------------------
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] gnodet commented on pull request #444: [MNG-7095] Fix resume for parallel builds
Posted by GitBox <gi...@apache.org>.
gnodet commented on pull request #444:
URL: https://github.com/apache/maven/pull/444#issuecomment-778264311
@mthmulders @MartinKanters quick question.
I'm working on improving the integration test and have the following scenario: a parent, 3 modules named `A`, `B`, `C` with `C` depending on `B` (that's the `mng-5760/parent-dependent` test project).
I run the `test` goal on the project with `-T2` and with `A` and `C` failing, which means `B` succeeds. When resuming, `C` fails because it can not find the artifact `B`. That's because the goal run previously was `test` and not `install` so the artifact is not in the local repository. However, shouldn't maven be _aware_ that the project is in the local reactor somehow so that the build can succeed ?
----------------------------------------------------------------
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] gnodet commented on pull request #444: [MNG-7095] Fix resume for parallel builds
Posted by GitBox <gi...@apache.org>.
gnodet commented on pull request #444:
URL: https://github.com/apache/maven/pull/444#issuecomment-778360471
> > @mthmulders @MartinKanters quick question.
> > I'm working on improving the integration test and have the following scenario: a parent, 3 modules named `A`, `B`, `C` with `C` depending on `B` (that's the `mng-5760/parent-dependent` test project).
> > I run the `test` goal on the project with `-T2` and with `A` and `C` failing, which means `B` succeeds. When resuming, `C` fails because it can not find the artifact `B`. That's because the goal run previously was `test` and not `install` so the artifact is not in the local repository. However, shouldn't maven be _aware_ that the project is in the local reactor somehow so that the build can succeed ?
>
> @gnodet I would also expect that to succeed. Although module B hadn't been packaged in the earlier build, it does have a folder with compiled classes, and that should be enough. Can you isolate that behaviour into a separate test project so I can have a look at that later?
I've pushed https://github.com/apache/maven-integration-testing/pull/98 which contains 2 integration tests for the parallel build with the resume flag.
I'll create another PR with an integration test for the other problem.
----------------------------------------------------------------
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 pull request #444: [MNG-7095] Fix resume for parallel builds
Posted by GitBox <gi...@apache.org>.
mthmulders commented on pull request #444:
URL: https://github.com/apache/maven/pull/444#issuecomment-778301747
> @mthmulders @MartinKanters quick question.
> I'm working on improving the integration test and have the following scenario: a parent, 3 modules named `A`, `B`, `C` with `C` depending on `B` (that's the `mng-5760/parent-dependent` test project).
> I run the `test` goal on the project with `-T2` and with `A` and `C` failing, which means `B` succeeds. When resuming, `C` fails because it can not find the artifact `B`. That's because the goal run previously was `test` and not `install` so the artifact is not in the local repository. However, shouldn't maven be _aware_ that the project is in the local reactor somehow so that the build can succeed ?
@gnodet I would also expect that to succeed. Although module B hadn't been packaged in the earlier build, it does have a folder with compiled classes, and that should be enough. Can you isolate that behaviour into a separate test project so I can have a look at that later?
----------------------------------------------------------------
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] gnodet commented on pull request #444: [MNG-7095] Fix resume for parallel builds
Posted by GitBox <gi...@apache.org>.
gnodet commented on pull request #444:
URL: https://github.com/apache/maven/pull/444#issuecomment-778388548
I've raised MNG-7099 and provided a PR with an integration test.
----------------------------------------------------------------
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] gnodet commented on a change in pull request #444: [MNG-7095] Fix resume for parallel builds
Posted by GitBox <gi...@apache.org>.
gnodet commented on a change in pull request #444:
URL: https://github.com/apache/maven/pull/444#discussion_r575250093
##########
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 see. I'll bring it back.
----------------------------------------------------------------
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] gnodet commented on a change in pull request #444: [MNG-7095] Fix resume for parallel builds
Posted by GitBox <gi...@apache.org>.
gnodet commented on a change in pull request #444:
URL: https://github.com/apache/maven/pull/444#discussion_r575178829
##########
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:
I'll refactor this class to change the field name and fix the javadoc.
##########
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:
See above
----------------------------------------------------------------
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] gnodet commented on pull request #444: [MNG-7095] Fix resume for parallel builds
Posted by GitBox <gi...@apache.org>.
gnodet commented on pull request #444:
URL: https://github.com/apache/maven/pull/444#issuecomment-778273066
I've added an integration test that tries to make sure the resume won't rebuild already built projects,
see https://github.com/apache/maven-integration-testing/pull/98
Eventually, a test that would verify that SKIPPED projects that are _topologically before_ the first FAILED project are rebuilt too would be nice.
----------------------------------------------------------------
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 #444: [MNG-7095] Fix resume for parallel builds
Posted by GitBox <gi...@apache.org>.
mthmulders commented on a change in pull request #444:
URL: https://github.com/apache/maven/pull/444#discussion_r575231766
##########
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:
```suggestion
.forEach( request.getSelectedProjects()::add );
```
----------------------------------------------------------------
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 #444: [MNG-7095] Fix resume for parallel builds
Posted by GitBox <gi...@apache.org>.
MartinKanters commented on pull request #444:
URL: https://github.com/apache/maven/pull/444#issuecomment-778597830
Thanks for your contribution @gnodet !
----------------------------------------------------------------
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 #444: [MNG-7095] Fix resume for parallel builds
Posted by GitBox <gi...@apache.org>.
MartinKanters commented on pull request #444:
URL: https://github.com/apache/maven/pull/444#issuecomment-778412923
> > > @mthmulders @MartinKanters quick question.
> > > I'm working on improving the integration test and have the following scenario: a parent, 3 modules named `A`, `B`, `C` with `C` depending on `B` (that's the `mng-5760/parent-dependent` test project).
> > > I run the `test` goal on the project with `-T2` and with `A` and `C` failing, which means `B` succeeds. When resuming, `C` fails because it can not find the artifact `B`. That's because the goal run previously was `test` and not `install` so the artifact is not in the local repository. However, shouldn't maven be _aware_ that the project is in the local reactor somehow so that the build can succeed ?
> >
> >
> > @gnodet I would also expect that to succeed. Although module B hadn't been packaged in the earlier build, it does have a folder with compiled classes, and that should be enough. Can you isolate that behaviour into a separate test project so I can have a look at that later?
>
> I've pushed [apache/maven-integration-testing#98](https://github.com/apache/maven-integration-testing/pull/98) which contains 2 integration tests for the parallel build with the resume flag.
>
> I'll create another PR with an integration test for the other problem.
You've encountered the bug that MNG-4660 has fixed, but is not released in a main Maven release yet. With the latest Maven snapshot build you should not have to use install anymore.
I did review your other ITs as well, they look good (apart from some small points).
----------------------------------------------------------------
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] gnodet commented on a change in pull request #444: [MNG-7095] Fix resume for parallel builds
Posted by GitBox <gi...@apache.org>.
gnodet commented on a change in pull request #444:
URL: https://github.com/apache/maven/pull/444#discussion_r575178653
##########
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:
I haven't checked the possible impact of `--non-recursive`. My main motivation was to keep the list small, especially as the `--resume` will now behave as `--pl` and that one automatically adds children projects to the build, see https://github.com/apache/maven/blob/22016982341262193d802ef51b2c236bac98464a/maven-core/src/main/java/org/apache/maven/graph/DefaultGraphBuilder.java#L194-L198
I don't think this is impacted by the `--non-recursive`. What could be the behavior when using both `--resume` and `--non-recursive` ? Not sure it makes much 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] gnodet commented on pull request #444: [MNG-7095] Fix resume for parallel builds
Posted by GitBox <gi...@apache.org>.
gnodet commented on pull request #444:
URL: https://github.com/apache/maven/pull/444#issuecomment-778266531
> @mthmulders @MartinKanters quick question.
> I'm working on improving the integration test and have the following scenario: a parent, 3 modules named `A`, `B`, `C` with `C` depending on `B` (that's the `mng-5760/parent-dependent` test project).
> I run the `test` goal on the project with `-T2` and with `A` and `C` failing, which means `B` succeeds. When resuming, `C` fails because it can not find the artifact `B`. That's because the goal run previously was `test` and not `install` so the artifact is not in the local repository. However, shouldn't maven be _aware_ that the project is in the local reactor somehow so that the build can succeed ?
I think this problem is not specific to the `--resume` feature, and it seems the behavior is the same for the `--resume-from` feature. So I'll push a PR with the test once I've run the full 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] gnodet commented on pull request #444: [MNG-7095] Fix resume for parallel builds
Posted by GitBox <gi...@apache.org>.
gnodet commented on pull request #444:
URL: https://github.com/apache/maven/pull/444#issuecomment-778165900
> I like the fact that the solution becomes much simpler. Could you please add a unit test or an [integration test](https://github.com/apache/maven-integration-testing/) that proves the issue has been fixed?
I've pushed a PR with the suggested fixes. I'll try to create an integration tests asap.
----------------------------------------------------------------
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] gnodet commented on a change in pull request #444: [MNG-7095] Fix resume for parallel builds
Posted by GitBox <gi...@apache.org>.
gnodet commented on a change in pull request #444:
URL: https://github.com/apache/maven/pull/444#discussion_r575184291
##########
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 can bring it back, I just don't think there's any case where `-rf` could work while `-r` would not be available. Especially now with the fact that it's just a list of non succeeded projects...
----------------------------------------------------------------
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 #444: [MNG-7095] Fix resume for parallel builds
Posted by GitBox <gi...@apache.org>.
MartinKanters commented on a change in pull request #444:
URL: https://github.com/apache/maven/pull/444#discussion_r575239494
##########
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:
It would indeed be a very exceptional case, but it's possible that writing the resume.properties file on disk fails for some reason. That's why we kept it.
This piece of code ensures that in that event, the canResume flag is not set to `true`, so it would end up in logging the `-rf` hint.
https://github.com/apache/maven/blob/master/maven-core/src/main/java/org/apache/maven/DefaultMaven.java#L404-L412
----------------------------------------------------------------
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