mthmulders commented on a change in pull request #659:
URL: https://github.com/apache/maven/pull/659#discussion_r805204885



##########
File path: 
maven-core/src/main/java/org/apache/maven/graph/DefaultGraphBuilder.java
##########
@@ -242,106 +260,86 @@ public DefaultGraphBuilder( 
BuildResumptionDataRepository buildResumptionDataRep
         return selectedProjects;
     }
 
-    private List<MavenProject> trimResumedProjects( List<MavenProject> 
projects, ProjectDependencyGraph graph,
-                                                    MavenExecutionRequest 
request )
+    private Set<MavenProject> getResumedProjects( Set<MavenProject> 
projectsToActivate,
+                                                  List<MavenProject> 
allSortedProjects, MavenExecutionRequest request )
             throws MavenExecutionException
     {
-        List<MavenProject> result = projects;
-
-        if ( StringUtils.isNotEmpty( request.getResumeFrom() ) )
+        if ( StringUtils.isEmpty( request.getResumeFrom() ) )
         {
-            File reactorDirectory = getReactorDirectory( request );
-
-            String selector = request.getResumeFrom();
+            return projectsToActivate;
+        }
 
-            MavenProject resumingFromProject = projects.stream()
-                    .filter( project -> isMatchingProject( project, selector, 
reactorDirectory ) )
-                    .findFirst()
-                    .orElseThrow( () -> new MavenExecutionException(
-                            "Could not find project to resume reactor build 
from: " + selector + " vs "
-                            + formatProjects( projects ), request.getPom() ) );
-            int resumeFromProjectIndex = projects.indexOf( resumingFromProject 
);
-            List<MavenProject> retainingProjects = result.subList( 
resumeFromProjectIndex, projects.size() );
+        File reactorDirectory = getReactorDirectory( request );
+        List<MavenProject> projectSelection = new ArrayList<>( 
projectsToActivate );
+        projectSelection.sort( comparing( allSortedProjects::indexOf ) );
 
-            result = includeAlsoMakeTransitively( retainingProjects, request, 
graph );
-        }
+        String selector = request.getResumeFrom();
+        MavenProject resumingFromProject = projectsToActivate.stream()
+                .filter( project -> isMatchingProject( project, selector, 
reactorDirectory ) )
+                .findFirst()
+                .orElseThrow( () -> new MavenExecutionException(
+                        "Could not find project to resume reactor build from: 
" + selector + " vs "
+                        + formatProjects( projectSelection ), request.getPom() 
) );
 
-        return result;
+        int resumeFromProjectIndex = projectSelection.indexOf( 
resumingFromProject );
+        return new HashSet<>( projectSelection.subList( 
resumeFromProjectIndex, projectSelection.size() ) );
     }
 
-    private List<MavenProject> trimExcludedProjects( List<MavenProject> 
projects, ProjectDependencyGraph graph,
-                                                     MavenExecutionRequest 
request )
+    private Set<MavenProject> getExcludedProjects( Set<MavenProject> 
projectsToActivate, ProjectDependencyGraph graph,
+                                                   MavenExecutionRequest 
request )
         throws MavenExecutionException
     {
-        List<MavenProject> result = projects;
-
         ProjectActivation projectActivation = request.getProjectActivation();
         Set<String> requiredSelectors = 
projectActivation.getRequiredInactiveProjectSelectors();
         Set<String> optionalSelectors = 
projectActivation.getOptionalInactiveProjectSelectors();
-        if ( !requiredSelectors.isEmpty() || !optionalSelectors.isEmpty() )
+        if ( requiredSelectors.isEmpty() && optionalSelectors.isEmpty() )
         {
-            Set<MavenProject> excludedProjects = new HashSet<>( 
requiredSelectors.size() + optionalSelectors.size() );
-            List<MavenProject> allProjects = graph.getAllProjects();
-            excludedProjects.addAll( getProjectsBySelectors( request, 
allProjects, requiredSelectors, true ) );
-            excludedProjects.addAll( getProjectsBySelectors( request, 
allProjects, optionalSelectors, false ) );
+            return Collections.emptySet();
+        }
 
-            result = new ArrayList<>( projects );
-            result.removeAll( excludedProjects );
+        Set<MavenProject> excludedProjects = new HashSet<>( 
requiredSelectors.size() + optionalSelectors.size() );
+        List<MavenProject> allProjects = graph.getAllProjects();
+        excludedProjects.addAll( getProjectsBySelectors( request, allProjects, 
requiredSelectors, true ) );
+        excludedProjects.addAll( getProjectsBySelectors( request, allProjects, 
optionalSelectors, false ) );
 
-            if ( result.isEmpty() )
-            {
-                boolean isPlural = excludedProjects.size() > 1;
-                String message = String.format( "The project exclusion%s in 
--projects/-pl resulted in an "
-                        + "empty reactor, please correct %s.", isPlural ? "s" 
: "", isPlural ? "them" : "it" );
-                throw new MavenExecutionException( message, request.getPom() );
-            }
+        if ( excludedProjects.size() >= projectsToActivate.size() )
+        {
+            boolean isPlural = excludedProjects.size() > 1;
+            String message = String.format( "The project exclusion%s in 
--projects/-pl resulted in an "
+                    + "empty reactor, please correct %s.", isPlural ? "s" : 
"", isPlural ? "them" : "it" );
+            throw new MavenExecutionException( message, request.getPom() );
         }
 
-        return result;
+        return excludedProjects;
     }
 
-    private List<MavenProject> includeAlsoMakeTransitively( List<MavenProject> 
projects, MavenExecutionRequest request,
-                                                            
ProjectDependencyGraph graph )
+    private Set<MavenProject> getAlsoMakeDependencies( Set<MavenProject> 
projectsToActivate,
+                                                       MavenExecutionRequest 
request, ProjectDependencyGraph graph )
             throws MavenExecutionException
     {
-        List<MavenProject> result = projects;
-
         String makeBehavior = request.getMakeBehavior();
         boolean makeBoth = MavenExecutionRequest.REACTOR_MAKE_BOTH.equals( 
makeBehavior );
 
         boolean makeUpstream = makeBoth || 
MavenExecutionRequest.REACTOR_MAKE_UPSTREAM.equals( makeBehavior );
         boolean makeDownstream = makeBoth || 
MavenExecutionRequest.REACTOR_MAKE_DOWNSTREAM.equals( makeBehavior );
 
-        if ( StringUtils.isNotEmpty( makeBehavior ) && !makeUpstream && 
!makeDownstream )
+        if ( !makeUpstream && !makeDownstream )
         {
-            throw new MavenExecutionException( "Invalid reactor make behavior: 
" + makeBehavior,
-                    request.getPom() );
-        }
-
-        if ( makeUpstream || makeDownstream )
-        {
-            Set<MavenProject> projectsSet = new HashSet<>( projects );
-
-            for ( MavenProject project : projects )
+            if ( StringUtils.isNotEmpty( makeBehavior ) )
             {
-                if ( makeUpstream )
-                {
-                    projectsSet.addAll( graph.getUpstreamProjects( project, 
true ) );
-                }
-                if ( makeDownstream )
-                {
-                    projectsSet.addAll( graph.getDownstreamProjects( project, 
true ) );
-                }
+                throw new MavenExecutionException( "Invalid reactor make 
behavior: " + makeBehavior, request.getPom() );
             }
 
-            result = new ArrayList<>( projectsSet );
-
-            // Order the new list in the original order
-            List<MavenProject> sortedProjects = graph.getSortedProjects();
-            result.sort( comparing( sortedProjects::indexOf ) );
+            return Collections.emptySet();
         }
 
-        return result;
+        List<MavenProject> emptyList = Collections.emptyList();
+        return projectsToActivate.stream()

Review comment:
       I _think_ this does the same as the previous code, but to be honest, I'm 
having a hard time to make sure it does. Why did you introduce a bunch of 
Streams here? Does it give any benefit over the previous, Collection-based 
approach?

##########
File path: 
maven-core/src/main/java/org/apache/maven/graph/DefaultGraphBuilder.java
##########
@@ -137,72 +139,88 @@ public DefaultGraphBuilder( BuildResumptionDataRepository 
buildResumptionDataRep
         throws CycleDetectedException, DuplicateProjectException, 
MavenExecutionException
     {
         ProjectDependencyGraph projectDependencyGraph = new 
DefaultProjectDependencyGraph( projects );
-        List<MavenProject> activeProjects = 
projectDependencyGraph.getSortedProjects();
-        activeProjects = trimProjectsToRequest( activeProjects, 
projectDependencyGraph, session.getRequest() );
-        activeProjects = trimSelectedProjects( activeProjects, 
projectDependencyGraph, session.getRequest() );
-        activeProjects = trimResumedProjects( activeProjects, 
projectDependencyGraph, session.getRequest() );
-        activeProjects = trimExcludedProjects( activeProjects, 
projectDependencyGraph, session.getRequest() );
+        List<MavenProject> allSortedProjects = 
projectDependencyGraph.getSortedProjects();
+        MavenExecutionRequest request = session.getRequest();
+
+        // Select projects into the reactor
+        Set<MavenProject> projectsToActivate = new HashSet<>( 
allSortedProjects.size() );
+        Set<MavenProject> projectsBySelection = getProjectsBySelection( 
allSortedProjects, request );
+        projectsToActivate.addAll( projectsBySelection );
 
-        if ( activeProjects.size() != 
projectDependencyGraph.getSortedProjects().size() )
+        // If no projects were selected deliberately by the user, fall back to 
all projects in this dir and below.
+        if ( projectsToActivate.isEmpty() )
         {
-            projectDependencyGraph = new FilteredProjectDependencyGraph( 
projectDependencyGraph, activeProjects );
+            Set<MavenProject> projectsByDirectory = getProjectsByDirectory( 
allSortedProjects, request );
+            projectsToActivate.addAll( projectsByDirectory );
+        }
+
+        // Add all transitive dependencies
+        Set<MavenProject> alsoMakeDependencies = getAlsoMakeDependencies( 
projectsToActivate, request,
+                projectDependencyGraph );
+        projectsToActivate.addAll( alsoMakeDependencies );
+
+        // Trim until resumed project
+        Set<MavenProject> resumedProjects = getResumedProjects( 
projectsToActivate, allSortedProjects, request );
+        projectsToActivate.retainAll( resumedProjects );
+
+        // Add back all transitive dependencies after projects were trimmed
+        projectsToActivate.addAll( alsoMakeDependencies );
+
+        // Remove unwanted projects from the reactor
+        getExcludedProjects( projectsToActivate, projectDependencyGraph, 
request )
+                .forEach( projectsToActivate::remove );
+
+        if ( projectsToActivate.size() != 
projectDependencyGraph.getSortedProjects().size() )
+        {
+            // Sort all projects in build order
+            List<MavenProject> activeProjects = new ArrayList<>( 
projectsToActivate );
+            activeProjects.sort( comparing( allSortedProjects::indexOf ) );
+
+            return Result.success( new FilteredProjectDependencyGraph( 
projectDependencyGraph, activeProjects ) );
         }
 
         return Result.success( projectDependencyGraph );
     }
 
-    private List<MavenProject> trimProjectsToRequest( List<MavenProject> 
activeProjects,
-                                                      ProjectDependencyGraph 
graph,
+    private Set<MavenProject> getProjectsByDirectory( List<MavenProject> 
allSortedProjects,
                                                       MavenExecutionRequest 
request )
             throws MavenExecutionException
     {
-        List<MavenProject> result = activeProjects;
-
-        if ( request.getPom() != null )
+        if ( request.getPom() == null )
         {
-            result = getProjectsInRequestScope( request, activeProjects );
-
-            List<MavenProject> sortedProjects = graph.getSortedProjects();
-            result.sort( comparing( sortedProjects::indexOf ) );
-
-            result = includeAlsoMakeTransitively( result, request, graph );
+            return new HashSet<>( allSortedProjects );
         }
 
-        return result;
+        return getProjectsInRequestScope( request, allSortedProjects );
     }
 
-    private List<MavenProject> trimSelectedProjects( List<MavenProject> 
projects, ProjectDependencyGraph graph,
-                                                     MavenExecutionRequest 
request )
+    private Set<MavenProject> getProjectsBySelection( List<MavenProject> 
allSortedProjects,
+                                                      MavenExecutionRequest 
request )
         throws MavenExecutionException
     {
-        List<MavenProject> result = projects;
-
         ProjectActivation projectActivation = request.getProjectActivation();
         Set<String> requiredSelectors = 
projectActivation.getRequiredActiveProjectSelectors();
         Set<String> optionalSelectors = 
projectActivation.getOptionalActiveProjectSelectors();
-        if ( !requiredSelectors.isEmpty() || !optionalSelectors.isEmpty() )
+        if ( requiredSelectors.isEmpty() && optionalSelectors.isEmpty() )
         {
-            Set<MavenProject> selectedProjects = new HashSet<>( 
requiredSelectors.size() + optionalSelectors.size() );
-            selectedProjects.addAll( getProjectsBySelectors( request, 
projects, requiredSelectors, true ) );
-            selectedProjects.addAll( getProjectsBySelectors( request, 
projects, optionalSelectors, false ) );
-
-            // it can be empty when an optional project is missing from the 
reactor, fallback to returning all projects
-            if ( !selectedProjects.isEmpty() )
-            {
-                result = new ArrayList<>( selectedProjects );
+            return Collections.emptySet();
+        }
 
-                result = includeAlsoMakeTransitively( result, request, graph );
+        Set<MavenProject> selectedProjects = new HashSet<>( 
requiredSelectors.size() + optionalSelectors.size() );
+        selectedProjects.addAll( getProjectsBySelectors( request, 
allSortedProjects, requiredSelectors, true ) );
+        selectedProjects.addAll( getProjectsBySelectors( request, 
allSortedProjects, optionalSelectors, false ) );
 
-                // Order the new list in the original order
-                List<MavenProject> sortedProjects = graph.getSortedProjects();
-                result.sort( comparing( sortedProjects::indexOf ) );
-            }
+        // it can be empty when an optional project is missing from the reactor

Review comment:
       If I understand the situation correctly:
   
   ```suggestion
           // The set can be empty when an optional project is selected that 
doesn't exist in the reactor
   ```
   
   (If that doesn't make sense, feel free to reject)




-- 
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.

To unsubscribe, e-mail: issues-unsubscr...@maven.apache.org

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


Reply via email to