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/13 18:34:29 UTC
[GitHub] [maven] MartinKanters opened a new pull request #446: [MNG-6511] - Optional project selection
MartinKanters opened a new pull request #446:
URL: https://github.com/apache/maven/pull/446
Following this checklist to help us incorporate your
contribution quickly and easily:
- [x] Make sure there is a [JIRA issue](https://issues.apache.org/jira/browse/MNG-6511) 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.
- [x] Each commit in the pull request should have a meaningful subject line and body.
- [x] 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.
- [x] Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
- [x] Run `mvn clean verify` to make sure basic checks pass. A more thorough check will
be performed on your pull request automatically.
- [x] 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.
- [x] 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)
- [x] 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 feature allows users to mark projects as optional when using the --projects (or -pl) switch using the `?`-prefix.
Old behavior is that a non-existing project in the -pl switch will break the build.
It works similar as the [optional profile functionality (MNG-7051)](https://issues.apache.org/jira/browse/MNG-7051) which was recently introduced on master.
Matching integration tests: https://github.com/apache/maven-integration-testing/pull/100
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] michael-o commented on a change in pull request #446: [MNG-6511] - Optional project selection
Posted by GitBox <gi...@apache.org>.
michael-o commented on a change in pull request #446:
URL: https://github.com/apache/maven/pull/446#discussion_r583711304
##########
File path: maven-core/src/main/java/org/apache/maven/graph/DefaultGraphBuilder.java
##########
@@ -176,38 +177,69 @@ public DefaultGraphBuilder( BuildResumptionDataRepository buildResumptionDataRep
{
List<MavenProject> result = projects;
- if ( !request.getSelectedProjects().isEmpty() )
+ ProjectActivation projectActivation = request.getProjectActivation();
+ Set<String> requiredSelectors = projectActivation.getRequiredActiveProjectSelectors();
+ Set<String> optionalSelectors = projectActivation.getOptionalActiveProjectSelectors();
+ if ( !requiredSelectors.isEmpty() || !optionalSelectors.isEmpty() )
{
- File reactorDirectory = getReactorDirectory( request );
+ 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 );
+
+ result = includeAlsoMakeTransitively( result, request, graph );
+
+ // Order the new list in the original order
+ List<MavenProject> sortedProjects = graph.getSortedProjects();
+ result.sort( comparing( sortedProjects::indexOf ) );
+ }
+ }
+
+ return result;
+ }
- Collection<MavenProject> selectedProjects = new LinkedHashSet<>();
+ private Set<MavenProject> getProjectsBySelectors( MavenExecutionRequest request, List<MavenProject> projects,
+ Set<String> projectSelectors, boolean required )
+ throws MavenExecutionException
+ {
+ Set<MavenProject> selectedProjects = new LinkedHashSet<>();
+ File reactorDirectory = getReactorDirectory( request );
- for ( String selector : request.getSelectedProjects() )
+ for ( String selector : projectSelectors )
+ {
+ Optional<MavenProject> optSelectedProject = projects.stream()
+ .filter( project -> isMatchingProject( project, selector, reactorDirectory ) )
+ .findFirst();
+ if ( !optSelectedProject.isPresent() )
{
- MavenProject selectedProject = projects.stream()
- .filter( project -> isMatchingProject( project, selector, reactorDirectory ) )
- .findFirst()
- .orElseThrow( () -> new MavenExecutionException(
- "Could not find the selected project in the reactor: " + selector, request.getPom() ) );
- selectedProjects.add( selectedProject );
-
- List<MavenProject> children = selectedProject.getCollectedProjects();
- if ( children != null )
+ String message = "Could not find the selected project in the reactor: " + selector;
+ if ( required )
+ {
+ throw new MavenExecutionException( message, request.getPom() );
+ }
+ else
{
- selectedProjects.addAll( children );
+ LOGGER.warn( message );
Review comment:
Yes, makes sense for optional profiles only.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] michael-o commented on a change in pull request #446: [MNG-6511] - Optional project selection
Posted by GitBox <gi...@apache.org>.
michael-o commented on a change in pull request #446:
URL: https://github.com/apache/maven/pull/446#discussion_r576387372
##########
File path: maven-core/src/main/java/org/apache/maven/graph/DefaultGraphBuilder.java
##########
@@ -242,21 +274,17 @@ public DefaultGraphBuilder( BuildResumptionDataRepository buildResumptionDataRep
{
List<MavenProject> result = projects;
- if ( !request.getExcludedProjects().isEmpty() )
+ ProjectActivation projectActivation = request.getProjectActivation();
+ Set<String> requiredSelectors = projectActivation.getRequiredInactiveProjectSelectors();
+ Set<String> optionalSelectors = projectActivation.getOptionalInactiveProjectSelectors();
+ if ( !requiredSelectors.isEmpty() || !optionalSelectors.isEmpty() )
{
- File reactorDirectory = getReactorDirectory( request );
+ Set<MavenProject> excludedProjects = new HashSet<>( requiredSelectors.size() + optionalSelectors.size() );
+ excludedProjects.addAll( getProjectsBySelectors( request, projects, requiredSelectors, true ) );
+ excludedProjects.addAll( getProjectsBySelectors( request, projects, optionalSelectors, false ) );
Review comment:
But shouldn't that be a null sum game? "No goal ..." sounds wrong in any case, doesn't it?
----------------------------------------------------------------
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 #446: [MNG-6511] - Optional project selection
Posted by GitBox <gi...@apache.org>.
MartinKanters commented on a change in pull request #446:
URL: https://github.com/apache/maven/pull/446#discussion_r575810640
##########
File path: maven-core/src/main/java/org/apache/maven/execution/ProjectActivation.java
##########
@@ -0,0 +1,173 @@
+package org.apache.maven.execution;
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import java.util.function.Predicate;
+
+import static java.util.stream.Collectors.toSet;
+
+/**
+ * Container for storing the request from the user to activate or de-activate certain projects and optionally fail the
+ * build if those projects do not exist.
+ */
+public class ProjectActivation
+{
+ private final Map<String, ActivationSettings> activations = new HashMap<>();
+
+ /**
+ * Adds a project activation to the request.
+ * @param selector The selector of the project.
Review comment:
I agree, it was not defined anywhere near this class (if at all). I've added it to each method where "selector" is mentioned. Do you think this is too much and rather only have it at class-level instead?
----------------------------------------------------------------
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 #446: [MNG-6511] - Optional project selection
Posted by GitBox <gi...@apache.org>.
MartinKanters commented on a change in pull request #446:
URL: https://github.com/apache/maven/pull/446#discussion_r577123679
##########
File path: maven-core/src/main/java/org/apache/maven/graph/DefaultGraphBuilder.java
##########
@@ -242,21 +274,17 @@ public DefaultGraphBuilder( BuildResumptionDataRepository buildResumptionDataRep
{
List<MavenProject> result = projects;
- if ( !request.getExcludedProjects().isEmpty() )
+ ProjectActivation projectActivation = request.getProjectActivation();
+ Set<String> requiredSelectors = projectActivation.getRequiredInactiveProjectSelectors();
+ Set<String> optionalSelectors = projectActivation.getOptionalInactiveProjectSelectors();
+ if ( !requiredSelectors.isEmpty() || !optionalSelectors.isEmpty() )
{
- File reactorDirectory = getReactorDirectory( request );
+ Set<MavenProject> excludedProjects = new HashSet<>( requiredSelectors.size() + optionalSelectors.size() );
+ excludedProjects.addAll( getProjectsBySelectors( request, projects, requiredSelectors, true ) );
+ excludedProjects.addAll( getProjectsBySelectors( request, projects, optionalSelectors, false ) );
Review comment:
True, but that's now fixed with this last commit I did, right? You will now get the following:
```
> mvn -pl !core-it-suite,:core-it-suite validate
[INFO] Scanning for projects...
[ERROR] The project exclusion settings resulted in an empty project list, please correct the usage of --projects/-pl !<project> or -<project>
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] michael-o commented on a change in pull request #446: [MNG-6511] - Optional project selection
Posted by GitBox <gi...@apache.org>.
michael-o commented on a change in pull request #446:
URL: https://github.com/apache/maven/pull/446#discussion_r576411705
##########
File path: maven-core/src/main/java/org/apache/maven/graph/DefaultGraphBuilder.java
##########
@@ -242,21 +274,17 @@ public DefaultGraphBuilder( BuildResumptionDataRepository buildResumptionDataRep
{
List<MavenProject> result = projects;
- if ( !request.getExcludedProjects().isEmpty() )
+ ProjectActivation projectActivation = request.getProjectActivation();
+ Set<String> requiredSelectors = projectActivation.getRequiredInactiveProjectSelectors();
+ Set<String> optionalSelectors = projectActivation.getOptionalInactiveProjectSelectors();
+ if ( !requiredSelectors.isEmpty() || !optionalSelectors.isEmpty() )
{
- File reactorDirectory = getReactorDirectory( request );
+ Set<MavenProject> excludedProjects = new HashSet<>( requiredSelectors.size() + optionalSelectors.size() );
+ excludedProjects.addAll( getProjectsBySelectors( request, projects, requiredSelectors, true ) );
+ excludedProjects.addAll( getProjectsBySelectors( request, projects, optionalSelectors, false ) );
Review comment:
but that would actually only take care of the symptom, wouldn't it?
----------------------------------------------------------------
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 #446: [MNG-6511] - Optional project selection
Posted by GitBox <gi...@apache.org>.
mthmulders commented on a change in pull request #446:
URL: https://github.com/apache/maven/pull/446#discussion_r589386978
##########
File path: maven-core/src/main/java/org/apache/maven/graph/DefaultGraphBuilder.java
##########
@@ -242,21 +274,17 @@ public DefaultGraphBuilder( BuildResumptionDataRepository buildResumptionDataRep
{
List<MavenProject> result = projects;
- if ( !request.getExcludedProjects().isEmpty() )
+ ProjectActivation projectActivation = request.getProjectActivation();
+ Set<String> requiredSelectors = projectActivation.getRequiredInactiveProjectSelectors();
+ Set<String> optionalSelectors = projectActivation.getOptionalInactiveProjectSelectors();
+ if ( !requiredSelectors.isEmpty() || !optionalSelectors.isEmpty() )
{
- File reactorDirectory = getReactorDirectory( request );
+ Set<MavenProject> excludedProjects = new HashSet<>( requiredSelectors.size() + optionalSelectors.size() );
+ excludedProjects.addAll( getProjectsBySelectors( request, projects, requiredSelectors, true ) );
+ excludedProjects.addAll( getProjectsBySelectors( request, projects, optionalSelectors, false ) );
Review comment:
Yes: because these changes detect when `-pl` leads to no modules being selected, and the `DefaultGraphBuilder` then aborts the process, we will no longer get the "No goals have been specified" message.
----------------------------------------------------------------
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 #446: [MNG-6511] - Optional project selection
Posted by GitBox <gi...@apache.org>.
MartinKanters commented on a change in pull request #446:
URL: https://github.com/apache/maven/pull/446#discussion_r591169720
##########
File path: maven-core/src/main/java/org/apache/maven/graph/DefaultGraphBuilder.java
##########
@@ -242,21 +274,17 @@ public DefaultGraphBuilder( BuildResumptionDataRepository buildResumptionDataRep
{
List<MavenProject> result = projects;
- if ( !request.getExcludedProjects().isEmpty() )
+ ProjectActivation projectActivation = request.getProjectActivation();
+ Set<String> requiredSelectors = projectActivation.getRequiredInactiveProjectSelectors();
+ Set<String> optionalSelectors = projectActivation.getOptionalInactiveProjectSelectors();
+ if ( !requiredSelectors.isEmpty() || !optionalSelectors.isEmpty() )
{
- File reactorDirectory = getReactorDirectory( request );
+ Set<MavenProject> excludedProjects = new HashSet<>( requiredSelectors.size() + optionalSelectors.size() );
+ excludedProjects.addAll( getProjectsBySelectors( request, projects, requiredSelectors, true ) );
+ excludedProjects.addAll( getProjectsBySelectors( request, projects, optionalSelectors, false ) );
Review comment:
@michael-o we have implemented all logic to make the 4 points in [the reply above](https://github.com/apache/maven/pull/446#discussion_r583679349) work. It's up for review again if you have some time :)
----------------------------------------------------------------
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 #446: [MNG-6511] - Optional project selection
Posted by GitBox <gi...@apache.org>.
mthmulders commented on a change in pull request #446:
URL: https://github.com/apache/maven/pull/446#discussion_r583683653
##########
File path: maven-core/src/main/java/org/apache/maven/graph/DefaultGraphBuilder.java
##########
@@ -242,21 +274,17 @@ public DefaultGraphBuilder( BuildResumptionDataRepository buildResumptionDataRep
{
List<MavenProject> result = projects;
- if ( !request.getExcludedProjects().isEmpty() )
+ ProjectActivation projectActivation = request.getProjectActivation();
+ Set<String> requiredSelectors = projectActivation.getRequiredInactiveProjectSelectors();
+ Set<String> optionalSelectors = projectActivation.getOptionalInactiveProjectSelectors();
+ if ( !requiredSelectors.isEmpty() || !optionalSelectors.isEmpty() )
{
- File reactorDirectory = getReactorDirectory( request );
+ Set<MavenProject> excludedProjects = new HashSet<>( requiredSelectors.size() + optionalSelectors.size() );
+ excludedProjects.addAll( getProjectsBySelectors( request, projects, requiredSelectors, true ) );
+ excludedProjects.addAll( getProjectsBySelectors( request, projects, optionalSelectors, false ) );
Review comment:
The "No goals here" is in 3.6.3, IIUC. With these proposed changes it shouldn't happen anymore.
----------------------------------------------------------------
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 #446: [MNG-6511] - Optional project selection
Posted by GitBox <gi...@apache.org>.
MartinKanters commented on a change in pull request #446:
URL: https://github.com/apache/maven/pull/446#discussion_r576404399
##########
File path: maven-core/src/main/java/org/apache/maven/graph/DefaultGraphBuilder.java
##########
@@ -242,21 +274,17 @@ public DefaultGraphBuilder( BuildResumptionDataRepository buildResumptionDataRep
{
List<MavenProject> result = projects;
- if ( !request.getExcludedProjects().isEmpty() )
+ ProjectActivation projectActivation = request.getProjectActivation();
+ Set<String> requiredSelectors = projectActivation.getRequiredInactiveProjectSelectors();
+ Set<String> optionalSelectors = projectActivation.getOptionalInactiveProjectSelectors();
+ if ( !requiredSelectors.isEmpty() || !optionalSelectors.isEmpty() )
{
- File reactorDirectory = getReactorDirectory( request );
+ Set<MavenProject> excludedProjects = new HashSet<>( requiredSelectors.size() + optionalSelectors.size() );
+ excludedProjects.addAll( getProjectsBySelectors( request, projects, requiredSelectors, true ) );
+ excludedProjects.addAll( getProjectsBySelectors( request, projects, optionalSelectors, false ) );
Review comment:
"No goal" is definitely a bug. I'm thinking of throwing an exception when the reactor command line flags result in an empty project list.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] michael-o commented on a change in pull request #446: [MNG-6511] - Optional project selection
Posted by GitBox <gi...@apache.org>.
michael-o commented on a change in pull request #446:
URL: https://github.com/apache/maven/pull/446#discussion_r575836149
##########
File path: maven-core/src/main/java/org/apache/maven/execution/ProjectActivation.java
##########
@@ -0,0 +1,173 @@
+package org.apache.maven.execution;
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import java.util.function.Predicate;
+
+import static java.util.stream.Collectors.toSet;
+
+/**
+ * Container for storing the request from the user to activate or de-activate certain projects and optionally fail the
+ * build if those projects do not exist.
+ */
+public class ProjectActivation
+{
+ private final Map<String, ActivationSettings> activations = new HashMap<>();
+
+ /**
+ * Adds a project activation to the request.
+ * @param selector The selector of the project.
Review comment:
Make it class level or maybe we have definition at maven-site?
----------------------------------------------------------------
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 #446: [MNG-6511] - Optional project selection
Posted by GitBox <gi...@apache.org>.
MartinKanters commented on a change in pull request #446:
URL: https://github.com/apache/maven/pull/446#discussion_r575856204
##########
File path: maven-core/src/main/java/org/apache/maven/graph/DefaultGraphBuilder.java
##########
@@ -242,21 +274,17 @@ public DefaultGraphBuilder( BuildResumptionDataRepository buildResumptionDataRep
{
List<MavenProject> result = projects;
- if ( !request.getExcludedProjects().isEmpty() )
+ ProjectActivation projectActivation = request.getProjectActivation();
+ Set<String> requiredSelectors = projectActivation.getRequiredInactiveProjectSelectors();
+ Set<String> optionalSelectors = projectActivation.getOptionalInactiveProjectSelectors();
+ if ( !requiredSelectors.isEmpty() || !optionalSelectors.isEmpty() )
{
- File reactorDirectory = getReactorDirectory( request );
+ Set<MavenProject> excludedProjects = new HashSet<>( requiredSelectors.size() + optionalSelectors.size() );
+ excludedProjects.addAll( getProjectsBySelectors( request, projects, requiredSelectors, true ) );
+ excludedProjects.addAll( getProjectsBySelectors( request, projects, optionalSelectors, false ) );
Review comment:
I need a bit more time for this. The latest maven master build actually has the same bug as 3.6.3, but the build of this PR does not have the error. I need to go now, but will check tomorrow or something if I can find out why (and whether we want it or not..?)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] MartinKanters commented on a change in pull request #446: [MNG-6511] - Optional project selection
Posted by GitBox <gi...@apache.org>.
MartinKanters commented on a change in pull request #446:
URL: https://github.com/apache/maven/pull/446#discussion_r575807467
##########
File path: maven-core/src/main/java/org/apache/maven/execution/MavenExecutionRequest.java
##########
@@ -278,55 +290,61 @@
MavenExecutionRequest setProfiles( List<Profile> profiles );
/**
- * @deprecated Use {@link #getProfileActivation()}.
+ * @deprecated Since Maven 4: use {@link #getProfileActivation()}.
*/
@Deprecated
MavenExecutionRequest addActiveProfile( String profile );
/**
- * @deprecated Use {@link #getProfileActivation()}.
+ * @deprecated Since Maven 4: use {@link #getProfileActivation()}.
*/
@Deprecated
MavenExecutionRequest addActiveProfiles( List<String> profiles );
/**
- * @deprecated Use {@link #getProfileActivation()}.
+ * @deprecated Since Maven 4: use {@link #getProfileActivation()}.
Review comment:
Sure, will refactor it into a new PR after we've made a decision on the unmodifiableList change https://github.com/apache/maven/pull/446#discussion_r575807350
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] michael-o commented on a change in pull request #446: [MNG-6511] - Optional project selection
Posted by GitBox <gi...@apache.org>.
michael-o commented on a change in pull request #446:
URL: https://github.com/apache/maven/pull/446#discussion_r575836283
##########
File path: maven-core/src/main/java/org/apache/maven/graph/DefaultGraphBuilder.java
##########
@@ -242,21 +274,17 @@ public DefaultGraphBuilder( BuildResumptionDataRepository buildResumptionDataRep
{
List<MavenProject> result = projects;
- if ( !request.getExcludedProjects().isEmpty() )
+ ProjectActivation projectActivation = request.getProjectActivation();
+ Set<String> requiredSelectors = projectActivation.getRequiredInactiveProjectSelectors();
+ Set<String> optionalSelectors = projectActivation.getOptionalInactiveProjectSelectors();
+ if ( !requiredSelectors.isEmpty() || !optionalSelectors.isEmpty() )
{
- File reactorDirectory = getReactorDirectory( request );
+ Set<MavenProject> excludedProjects = new HashSet<>( requiredSelectors.size() + optionalSelectors.size() );
+ excludedProjects.addAll( getProjectsBySelectors( request, projects, requiredSelectors, true ) );
+ excludedProjects.addAll( getProjectsBySelectors( request, projects, optionalSelectors, false ) );
Review comment:
Interesting, thanks. What will happen with `-pl !proj,+proj`?
----------------------------------------------------------------
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 #446: [MNG-6511] - Optional project selection
Posted by GitBox <gi...@apache.org>.
mthmulders commented on a change in pull request #446:
URL: https://github.com/apache/maven/pull/446#discussion_r583671079
##########
File path: maven-core/src/main/java/org/apache/maven/execution/ProjectActivation.java
##########
@@ -0,0 +1,173 @@
+package org.apache.maven.execution;
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import java.util.function.Predicate;
+
+import static java.util.stream.Collectors.toSet;
+
+/**
+ * Container for storing the request from the user to activate or de-activate certain projects and optionally fail the
+ * build if those projects do not exist.
+ */
+public class ProjectActivation
+{
+ private final Map<String, ActivationSettings> activations = new HashMap<>();
+
+ /**
+ * Adds a project activation to the request.
+ * @param selector The selector of the project.
Review comment:
For now, we've moved it to class level. If we find a nice spot in maven-site we can still add it there.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] michael-o commented on a change in pull request #446: [MNG-6511] - Optional project selection
Posted by GitBox <gi...@apache.org>.
michael-o commented on a change in pull request #446:
URL: https://github.com/apache/maven/pull/446#discussion_r583723197
##########
File path: maven-core/src/main/java/org/apache/maven/graph/DefaultGraphBuilder.java
##########
@@ -242,21 +274,17 @@ public DefaultGraphBuilder( BuildResumptionDataRepository buildResumptionDataRep
{
List<MavenProject> result = projects;
- if ( !request.getExcludedProjects().isEmpty() )
+ ProjectActivation projectActivation = request.getProjectActivation();
+ Set<String> requiredSelectors = projectActivation.getRequiredInactiveProjectSelectors();
+ Set<String> optionalSelectors = projectActivation.getOptionalInactiveProjectSelectors();
+ if ( !requiredSelectors.isEmpty() || !optionalSelectors.isEmpty() )
{
- File reactorDirectory = getReactorDirectory( request );
+ Set<MavenProject> excludedProjects = new HashSet<>( requiredSelectors.size() + optionalSelectors.size() );
+ excludedProjects.addAll( getProjectsBySelectors( request, projects, requiredSelectors, true ) );
+ excludedProjects.addAll( getProjectsBySelectors( request, projects, optionalSelectors, false ) );
Review comment:
But only because you throw the exception, right?
----------------------------------------------------------------
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 #446: [MNG-6511] - Optional project selection
Posted by GitBox <gi...@apache.org>.
MartinKanters commented on a change in pull request #446:
URL: https://github.com/apache/maven/pull/446#discussion_r576420436
##########
File path: maven-core/src/main/java/org/apache/maven/graph/DefaultGraphBuilder.java
##########
@@ -242,21 +274,17 @@ public DefaultGraphBuilder( BuildResumptionDataRepository buildResumptionDataRep
{
List<MavenProject> result = projects;
- if ( !request.getExcludedProjects().isEmpty() )
+ ProjectActivation projectActivation = request.getProjectActivation();
+ Set<String> requiredSelectors = projectActivation.getRequiredInactiveProjectSelectors();
+ Set<String> optionalSelectors = projectActivation.getOptionalInactiveProjectSelectors();
+ if ( !requiredSelectors.isEmpty() || !optionalSelectors.isEmpty() )
{
- File reactorDirectory = getReactorDirectory( request );
+ Set<MavenProject> excludedProjects = new HashSet<>( requiredSelectors.size() + optionalSelectors.size() );
+ excludedProjects.addAll( getProjectsBySelectors( request, projects, requiredSelectors, true ) );
+ excludedProjects.addAll( getProjectsBySelectors( request, projects, optionalSelectors, false ) );
Review comment:
I guess it would, but I'm not sure how to fix it otherwise. What would you rather have? I don't think we can already check whether `a` or `:a` are the same in an earlier stage.
I've just committed the exception before I read your reply. Please let me know what you think.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] MartinKanters commented on a change in pull request #446: [MNG-6511] - Optional project selection
Posted by GitBox <gi...@apache.org>.
MartinKanters commented on a change in pull request #446:
URL: https://github.com/apache/maven/pull/446#discussion_r576385701
##########
File path: maven-core/src/main/java/org/apache/maven/graph/DefaultGraphBuilder.java
##########
@@ -242,21 +274,17 @@ public DefaultGraphBuilder( BuildResumptionDataRepository buildResumptionDataRep
{
List<MavenProject> result = projects;
- if ( !request.getExcludedProjects().isEmpty() )
+ ProjectActivation projectActivation = request.getProjectActivation();
+ Set<String> requiredSelectors = projectActivation.getRequiredInactiveProjectSelectors();
+ Set<String> optionalSelectors = projectActivation.getOptionalInactiveProjectSelectors();
+ if ( !requiredSelectors.isEmpty() || !optionalSelectors.isEmpty() )
{
- File reactorDirectory = getReactorDirectory( request );
+ Set<MavenProject> excludedProjects = new HashSet<>( requiredSelectors.size() + optionalSelectors.size() );
+ excludedProjects.addAll( getProjectsBySelectors( request, projects, requiredSelectors, true ) );
+ excludedProjects.addAll( getProjectsBySelectors( request, projects, optionalSelectors, false ) );
Review comment:
Right, the data structure inside `ProjectActivation` is a `Map<(ProjectSelector)String, (enum)ActivationSetting>`, so that means that a project cannot be both turned selected and excluded. The one who gets set later wins (in `-pl !a,+a` '+a' wins).
This was not fully intentional, but I don't dislike the outcome. The same goes for ProfileActivation, by the way.
The bug remains though if the user invokes `mvn -pl -a,+:a` (same proj, different selector). It will output "No goals have been specified for this build.". I'll improve the error message in this case.
----------------------------------------------------------------
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] asfgit closed pull request #446: [MNG-6511] - Optional project selection
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #446:
URL: https://github.com/apache/maven/pull/446
--
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 #446: [MNG-6511] - Optional project selection
Posted by GitBox <gi...@apache.org>.
mthmulders commented on a change in pull request #446:
URL: https://github.com/apache/maven/pull/446#discussion_r589428068
##########
File path: maven-core/src/main/java/org/apache/maven/graph/DefaultGraphBuilder.java
##########
@@ -176,38 +177,69 @@ public DefaultGraphBuilder( BuildResumptionDataRepository buildResumptionDataRep
{
List<MavenProject> result = projects;
- if ( !request.getSelectedProjects().isEmpty() )
+ ProjectActivation projectActivation = request.getProjectActivation();
+ Set<String> requiredSelectors = projectActivation.getRequiredActiveProjectSelectors();
+ Set<String> optionalSelectors = projectActivation.getOptionalActiveProjectSelectors();
+ if ( !requiredSelectors.isEmpty() || !optionalSelectors.isEmpty() )
{
- File reactorDirectory = getReactorDirectory( request );
+ 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 );
+
+ result = includeAlsoMakeTransitively( result, request, graph );
+
+ // Order the new list in the original order
+ List<MavenProject> sortedProjects = graph.getSortedProjects();
+ result.sort( comparing( sortedProjects::indexOf ) );
+ }
+ }
+
+ return result;
+ }
- Collection<MavenProject> selectedProjects = new LinkedHashSet<>();
+ private Set<MavenProject> getProjectsBySelectors( MavenExecutionRequest request, List<MavenProject> projects,
+ Set<String> projectSelectors, boolean required )
+ throws MavenExecutionException
+ {
+ Set<MavenProject> selectedProjects = new LinkedHashSet<>();
+ File reactorDirectory = getReactorDirectory( request );
- for ( String selector : request.getSelectedProjects() )
+ for ( String selector : projectSelectors )
+ {
+ Optional<MavenProject> optSelectedProject = projects.stream()
+ .filter( project -> isMatchingProject( project, selector, reactorDirectory ) )
+ .findFirst();
+ if ( !optSelectedProject.isPresent() )
{
- MavenProject selectedProject = projects.stream()
- .filter( project -> isMatchingProject( project, selector, reactorDirectory ) )
- .findFirst()
- .orElseThrow( () -> new MavenExecutionException(
- "Could not find the selected project in the reactor: " + selector, request.getPom() ) );
- selectedProjects.add( selectedProject );
-
- List<MavenProject> children = selectedProject.getCollectedProjects();
- if ( children != null )
+ String message = "Could not find the selected project in the reactor: " + selector;
+ if ( required )
+ {
+ throw new MavenExecutionException( message, request.getPom() );
+ }
+ else
{
- selectedProjects.addAll( children );
+ LOGGER.warn( message );
Review comment:
Will address this [separately](https://github.com/apache/maven/pull/453), to avoid messing up this one, which is about 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] michael-o commented on a change in pull request #446: [MNG-6511] - Optional project selection
Posted by GitBox <gi...@apache.org>.
michael-o commented on a change in pull request #446:
URL: https://github.com/apache/maven/pull/446#discussion_r583681045
##########
File path: maven-core/src/main/java/org/apache/maven/graph/DefaultGraphBuilder.java
##########
@@ -242,21 +274,17 @@ public DefaultGraphBuilder( BuildResumptionDataRepository buildResumptionDataRep
{
List<MavenProject> result = projects;
- if ( !request.getExcludedProjects().isEmpty() )
+ ProjectActivation projectActivation = request.getProjectActivation();
+ Set<String> requiredSelectors = projectActivation.getRequiredInactiveProjectSelectors();
+ Set<String> optionalSelectors = projectActivation.getOptionalInactiveProjectSelectors();
+ if ( !requiredSelectors.isEmpty() || !optionalSelectors.isEmpty() )
{
- File reactorDirectory = getReactorDirectory( request );
+ Set<MavenProject> excludedProjects = new HashSet<>( requiredSelectors.size() + optionalSelectors.size() );
+ excludedProjects.addAll( getProjectsBySelectors( request, projects, requiredSelectors, true ) );
+ excludedProjects.addAll( getProjectsBySelectors( request, projects, optionalSelectors, false ) );
Review comment:
All four steps make sense here. Though, I am still confused about the "No goals here...".
----------------------------------------------------------------
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 #446: [MNG-6511] - Optional project selection
Posted by GitBox <gi...@apache.org>.
MartinKanters commented on a change in pull request #446:
URL: https://github.com/apache/maven/pull/446#discussion_r575851985
##########
File path: maven-core/src/main/java/org/apache/maven/graph/DefaultGraphBuilder.java
##########
@@ -242,21 +274,17 @@ public DefaultGraphBuilder( BuildResumptionDataRepository buildResumptionDataRep
{
List<MavenProject> result = projects;
- if ( !request.getExcludedProjects().isEmpty() )
+ ProjectActivation projectActivation = request.getProjectActivation();
+ Set<String> requiredSelectors = projectActivation.getRequiredInactiveProjectSelectors();
+ Set<String> optionalSelectors = projectActivation.getOptionalInactiveProjectSelectors();
+ if ( !requiredSelectors.isEmpty() || !optionalSelectors.isEmpty() )
{
- File reactorDirectory = getReactorDirectory( request );
+ Set<MavenProject> excludedProjects = new HashSet<>( requiredSelectors.size() + optionalSelectors.size() );
+ excludedProjects.addAll( getProjectsBySelectors( request, projects, requiredSelectors, true ) );
+ excludedProjects.addAll( getProjectsBySelectors( request, projects, optionalSelectors, false ) );
Review comment:
Interesting question, the `DefaultGraphBuilder` will first filter based on the selected modules (+), afterwards it will filter out the excluded projects, so in this case nothing will be selected. Actually, it will result in a bug (I'll create a JIRA ticket for it)
This is an attempt in the integration-test project:
```
>mvn validate -pl !core-it-suite,+core-it-suite
[INFO] Scanning for projects...
[INFO] ------------------------------------------------------------------------
[INFO] BUILD FAILURE
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 0.630 s
[INFO] Finished at: 2021-02-14T20:15:52+01:00
[INFO] ------------------------------------------------------------------------
[ERROR] No goals have been specified for this build. You must specify a valid lifecycle phase or a goal in the format <plugin-prefix>:<goal> or <plugin-group-id>:<plugin-artifact-id>[:<plugin-version>]:<goal>. Available lifecycle phases are: validate, initialize, generate-sources, process-sources, generate-reso
urces, process-resources, compile, process-classes, generate-test-sources, process-test-sources, generate-test-resources, process-test-resources, test-compile, process-test-classes, test, prepare-package, package, pre-integration-test, integration-test, post-integration-test, verify, install, deploy, pre-clean,
clean, post-clean, pre-site, site, post-site, site-deploy. -> [Help 1]```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] michael-o commented on a change in pull request #446: [MNG-6511] - Optional project selection
Posted by GitBox <gi...@apache.org>.
michael-o commented on a change in pull request #446:
URL: https://github.com/apache/maven/pull/446#discussion_r577151442
##########
File path: maven-core/src/main/java/org/apache/maven/graph/DefaultGraphBuilder.java
##########
@@ -242,21 +274,17 @@ public DefaultGraphBuilder( BuildResumptionDataRepository buildResumptionDataRep
{
List<MavenProject> result = projects;
- if ( !request.getExcludedProjects().isEmpty() )
+ ProjectActivation projectActivation = request.getProjectActivation();
+ Set<String> requiredSelectors = projectActivation.getRequiredInactiveProjectSelectors();
+ Set<String> optionalSelectors = projectActivation.getOptionalInactiveProjectSelectors();
+ if ( !requiredSelectors.isEmpty() || !optionalSelectors.isEmpty() )
{
- File reactorDirectory = getReactorDirectory( request );
+ Set<MavenProject> excludedProjects = new HashSet<>( requiredSelectors.size() + optionalSelectors.size() );
+ excludedProjects.addAll( getProjectsBySelectors( request, projects, requiredSelectors, true ) );
+ excludedProjects.addAll( getProjectsBySelectors( request, projects, optionalSelectors, false ) );
Review comment:
Fixed the symptom. That's what I understand and it should carry a comment about that. One should explore the "No goal..." issue 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] MartinKanters commented on a change in pull request #446: [MNG-6511] - Optional project selection
Posted by GitBox <gi...@apache.org>.
MartinKanters commented on a change in pull request #446:
URL: https://github.com/apache/maven/pull/446#discussion_r575809881
##########
File path: maven-core/src/main/java/org/apache/maven/graph/DefaultGraphBuilder.java
##########
@@ -242,21 +274,17 @@ public DefaultGraphBuilder( BuildResumptionDataRepository buildResumptionDataRep
{
List<MavenProject> result = projects;
- if ( !request.getExcludedProjects().isEmpty() )
+ ProjectActivation projectActivation = request.getProjectActivation();
+ Set<String> requiredSelectors = projectActivation.getRequiredInactiveProjectSelectors();
+ Set<String> optionalSelectors = projectActivation.getOptionalInactiveProjectSelectors();
+ if ( !requiredSelectors.isEmpty() || !optionalSelectors.isEmpty() )
{
- File reactorDirectory = getReactorDirectory( request );
+ Set<MavenProject> excludedProjects = new HashSet<>( requiredSelectors.size() + optionalSelectors.size() );
+ excludedProjects.addAll( getProjectsBySelectors( request, projects, requiredSelectors, true ) );
+ excludedProjects.addAll( getProjectsBySelectors( request, projects, optionalSelectors, false ) );
Review comment:
This piece of code removes the excluded projects (`-pl !proj` and `-pl -?proj`) from the reactor. The other method limits the reactor to all selected projects (`-pl proj,?proj`). Both methods find all projects by the project selectors and by whether they are required or optional.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] michael-o commented on a change in pull request #446: [MNG-6511] - Optional project selection
Posted by GitBox <gi...@apache.org>.
michael-o commented on a change in pull request #446:
URL: https://github.com/apache/maven/pull/446#discussion_r575854161
##########
File path: maven-core/src/main/java/org/apache/maven/graph/DefaultGraphBuilder.java
##########
@@ -242,21 +274,17 @@ public DefaultGraphBuilder( BuildResumptionDataRepository buildResumptionDataRep
{
List<MavenProject> result = projects;
- if ( !request.getExcludedProjects().isEmpty() )
+ ProjectActivation projectActivation = request.getProjectActivation();
+ Set<String> requiredSelectors = projectActivation.getRequiredInactiveProjectSelectors();
+ Set<String> optionalSelectors = projectActivation.getOptionalInactiveProjectSelectors();
+ if ( !requiredSelectors.isEmpty() || !optionalSelectors.isEmpty() )
{
- File reactorDirectory = getReactorDirectory( request );
+ Set<MavenProject> excludedProjects = new HashSet<>( requiredSelectors.size() + optionalSelectors.size() );
+ excludedProjects.addAll( getProjectsBySelectors( request, projects, requiredSelectors, true ) );
+ excludedProjects.addAll( getProjectsBySelectors( request, projects, optionalSelectors, false ) );
Review comment:
OK...
----------------------------------------------------------------
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 #446: [MNG-6511] - Optional project selection
Posted by GitBox <gi...@apache.org>.
mthmulders commented on a change in pull request #446:
URL: https://github.com/apache/maven/pull/446#discussion_r583665088
##########
File path: maven-core/src/main/java/org/apache/maven/execution/MavenExecutionRequest.java
##########
@@ -278,55 +290,61 @@
MavenExecutionRequest setProfiles( List<Profile> profiles );
/**
- * @deprecated Use {@link #getProfileActivation()}.
+ * @deprecated Since Maven 4: use {@link #getProfileActivation()}.
*/
@Deprecated
MavenExecutionRequest addActiveProfile( String profile );
/**
- * @deprecated Use {@link #getProfileActivation()}.
+ * @deprecated Since Maven 4: use {@link #getProfileActivation()}.
*/
@Deprecated
MavenExecutionRequest addActiveProfiles( List<String> profiles );
/**
- * @deprecated Use {@link #getProfileActivation()}.
+ * @deprecated Since Maven 4: use {@link #getProfileActivation()}.
Review comment:
Moved those changes to #452 and force-pushed this branch.
----------------------------------------------------------------
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 #446: [MNG-6511] - Optional project selection
Posted by GitBox <gi...@apache.org>.
mthmulders commented on a change in pull request #446:
URL: https://github.com/apache/maven/pull/446#discussion_r583681131
##########
File path: maven-core/src/main/java/org/apache/maven/graph/DefaultGraphBuilder.java
##########
@@ -176,38 +177,69 @@ public DefaultGraphBuilder( BuildResumptionDataRepository buildResumptionDataRep
{
List<MavenProject> result = projects;
- if ( !request.getSelectedProjects().isEmpty() )
+ ProjectActivation projectActivation = request.getProjectActivation();
+ Set<String> requiredSelectors = projectActivation.getRequiredActiveProjectSelectors();
+ Set<String> optionalSelectors = projectActivation.getOptionalActiveProjectSelectors();
+ if ( !requiredSelectors.isEmpty() || !optionalSelectors.isEmpty() )
{
- File reactorDirectory = getReactorDirectory( request );
+ 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 );
+
+ result = includeAlsoMakeTransitively( result, request, graph );
+
+ // Order the new list in the original order
+ List<MavenProject> sortedProjects = graph.getSortedProjects();
+ result.sort( comparing( sortedProjects::indexOf ) );
+ }
+ }
+
+ return result;
+ }
- Collection<MavenProject> selectedProjects = new LinkedHashSet<>();
+ private Set<MavenProject> getProjectsBySelectors( MavenExecutionRequest request, List<MavenProject> projects,
+ Set<String> projectSelectors, boolean required )
+ throws MavenExecutionException
+ {
+ Set<MavenProject> selectedProjects = new LinkedHashSet<>();
+ File reactorDirectory = getReactorDirectory( request );
- for ( String selector : request.getSelectedProjects() )
+ for ( String selector : projectSelectors )
+ {
+ Optional<MavenProject> optSelectedProject = projects.stream()
+ .filter( project -> isMatchingProject( project, selector, reactorDirectory ) )
+ .findFirst();
+ if ( !optSelectedProject.isPresent() )
{
- MavenProject selectedProject = projects.stream()
- .filter( project -> isMatchingProject( project, selector, reactorDirectory ) )
- .findFirst()
- .orElseThrow( () -> new MavenExecutionException(
- "Could not find the selected project in the reactor: " + selector, request.getPom() ) );
- selectedProjects.add( selectedProject );
-
- List<MavenProject> children = selectedProject.getCollectedProjects();
- if ( children != null )
+ String message = "Could not find the selected project in the reactor: " + selector;
+ if ( required )
+ {
+ throw new MavenExecutionException( message, request.getPom() );
+ }
+ else
{
- selectedProjects.addAll( children );
+ LOGGER.warn( message );
Review comment:
Makes sense... I think it should be consistent between project and profile selection, so if we decide it should be `info` here that should also change for profiles.
@michael-o, what do you think?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] michael-o commented on a change in pull request #446: [MNG-6511] - Optional project selection
Posted by GitBox <gi...@apache.org>.
michael-o commented on a change in pull request #446:
URL: https://github.com/apache/maven/pull/446#discussion_r575700502
##########
File path: maven-core/src/main/java/org/apache/maven/execution/MavenExecutionRequest.java
##########
@@ -278,55 +290,61 @@
MavenExecutionRequest setProfiles( List<Profile> profiles );
/**
- * @deprecated Use {@link #getProfileActivation()}.
+ * @deprecated Since Maven 4: use {@link #getProfileActivation()}.
*/
@Deprecated
MavenExecutionRequest addActiveProfile( String profile );
/**
- * @deprecated Use {@link #getProfileActivation()}.
+ * @deprecated Since Maven 4: use {@link #getProfileActivation()}.
*/
@Deprecated
MavenExecutionRequest addActiveProfiles( List<String> profiles );
/**
- * @deprecated Use {@link #getProfileActivation()}.
+ * @deprecated Since Maven 4: use {@link #getProfileActivation()}.
Review comment:
I think these profile-related changes should be a separate PR.
##########
File path: maven-core/src/main/java/org/apache/maven/graph/DefaultGraphBuilder.java
##########
@@ -242,21 +274,17 @@ public DefaultGraphBuilder( BuildResumptionDataRepository buildResumptionDataRep
{
List<MavenProject> result = projects;
- if ( !request.getExcludedProjects().isEmpty() )
+ ProjectActivation projectActivation = request.getProjectActivation();
+ Set<String> requiredSelectors = projectActivation.getRequiredInactiveProjectSelectors();
+ Set<String> optionalSelectors = projectActivation.getOptionalInactiveProjectSelectors();
+ if ( !requiredSelectors.isEmpty() || !optionalSelectors.isEmpty() )
{
- File reactorDirectory = getReactorDirectory( request );
+ Set<MavenProject> excludedProjects = new HashSet<>( requiredSelectors.size() + optionalSelectors.size() );
+ excludedProjects.addAll( getProjectsBySelectors( request, projects, requiredSelectors, true ) );
+ excludedProjects.addAll( getProjectsBySelectors( request, projects, optionalSelectors, false ) );
Review comment:
Call me blind, but this just looks like [here](https://github.com/apache/maven/pull/446/files#diff-5a5fe4cfc363538b43d7a3e89cc8b12b64d7ae0f4159caa45740405a299b6e3eR180-R187), but method names are opposite. Can you explain please?
##########
File path: maven-core/src/main/java/org/apache/maven/execution/ActivationSettings.java
##########
@@ -0,0 +1,58 @@
+package org.apache.maven.execution;
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+/**
+ * Describes whether something (a profile or a project) should be activated or not, and if that is required or optional.
Review comment:
This is now completely decoupled from profiles or projects, I think the docs on the parens are superfluous.
##########
File path: maven-core/src/main/java/org/apache/maven/execution/ProfileActivation.java
##########
@@ -74,7 +44,7 @@ static ActivationSettings of( final boolean active, final boolean optional )
@Deprecated
public List<String> getActiveProfiles()
{
- return new ArrayList<>( getProfileIds( pa -> pa.active ) );
+ return Collections.unmodifiableList( new ArrayList<>( getProfileIds( pa -> pa.active ) ) );
Review comment:
This is likely nitpicking, but the previous impl did not return an immutable collection. I think since this deprecated anyway, it should not change its behavior.
##########
File path: maven-core/src/main/java/org/apache/maven/execution/ProjectActivation.java
##########
@@ -0,0 +1,173 @@
+package org.apache.maven.execution;
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import java.util.function.Predicate;
+
+import static java.util.stream.Collectors.toSet;
+
+/**
+ * Container for storing the request from the user to activate or de-activate certain projects and optionally fail the
+ * build if those projects do not exist.
+ */
+public class ProjectActivation
+{
+ private final Map<String, ActivationSettings> activations = new HashMap<>();
+
+ /**
+ * Adds a project activation to the request.
+ * @param selector The selector of the project.
+ * @param active Should the project be activated?
+ * @param optional Can the build continue if the project does not exist?
+ */
+ public void addProjectActivation( String selector, boolean active, boolean optional )
+ {
+ final ActivationSettings settings = ActivationSettings.of( active, optional );
+ this.activations.put( selector, settings );
+ }
+
+ private Set<String> getProjectSelectors( final Predicate<ActivationSettings> predicate )
+ {
+ return this.activations.entrySet().stream()
+ .filter( e -> predicate.test( e.getValue() ) )
+ .map( e -> e.getKey() )
+ .collect( toSet() );
+ }
+
+ /**
+ * @return Required active project selectors, never {@code null}.
+ */
+ public Set<String> getRequiredActiveProjectSelectors()
+ {
+ return getProjectSelectors( pa -> !pa.optional && pa.active );
+ }
+
+ /**
+ * @return Optional active project selectors, never {@code null}.
+ */
+ public Set<String> getOptionalActiveProjectSelectors()
+ {
+ return getProjectSelectors( pa -> pa.optional && pa.active );
+ }
+
+ /**
+ * @return Required inactive project selectors, never {@code null}.
+ */
+ public Set<String> getRequiredInactiveProjectSelectors()
+ {
+ return getProjectSelectors( pa -> !pa.optional && !pa.active );
+ }
+
+ /**
+ * @return Optional inactive project selectors, never {@code null}.
+ */
+ public Set<String> getOptionalInactiveProjectSelectors()
+ {
+ return getProjectSelectors( pa -> pa.optional && !pa.active );
+ }
+
+ /**
+ * Mimics the pre-Maven 4 "selected projects" list.
+ * @deprecated Use {@link #getRequiredActiveProjectSelectors()} and {@link #getOptionalActiveProjectSelectors()}
+ * instead.
+ */
+ @Deprecated
+ public List<String> getSelectedProjects()
+ {
+ return Collections.unmodifiableList( new ArrayList<>( getProjectSelectors( pa -> pa.active ) ) );
Review comment:
Same as above
##########
File path: maven-core/src/main/java/org/apache/maven/execution/ActivationSettings.java
##########
@@ -0,0 +1,58 @@
+package org.apache.maven.execution;
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+/**
+ * Describes whether something (a profile or a project) should be activated or not, and if that is required or optional.
Review comment:
Since you use the term 'target' later it should say: Describes whether a target...
##########
File path: maven-core/src/main/java/org/apache/maven/execution/ProjectActivation.java
##########
@@ -0,0 +1,173 @@
+package org.apache.maven.execution;
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import java.util.function.Predicate;
+
+import static java.util.stream.Collectors.toSet;
+
+/**
+ * Container for storing the request from the user to activate or de-activate certain projects and optionally fail the
+ * build if those projects do not exist.
+ */
+public class ProjectActivation
+{
+ private final Map<String, ActivationSettings> activations = new HashMap<>();
+
+ /**
+ * Adds a project activation to the request.
+ * @param selector The selector of the project.
Review comment:
You probably should describe at least at class level what a selector is . It took me a while that it is either a `module`, `:artifactId` or `groupId:artifactId`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] michael-o commented on a change in pull request #446: [MNG-6511] - Optional project selection
Posted by GitBox <gi...@apache.org>.
michael-o commented on a change in pull request #446:
URL: https://github.com/apache/maven/pull/446#discussion_r576424436
##########
File path: maven-core/src/main/java/org/apache/maven/graph/DefaultGraphBuilder.java
##########
@@ -242,21 +274,17 @@ public DefaultGraphBuilder( BuildResumptionDataRepository buildResumptionDataRep
{
List<MavenProject> result = projects;
- if ( !request.getExcludedProjects().isEmpty() )
+ ProjectActivation projectActivation = request.getProjectActivation();
+ Set<String> requiredSelectors = projectActivation.getRequiredInactiveProjectSelectors();
+ Set<String> optionalSelectors = projectActivation.getOptionalInactiveProjectSelectors();
+ if ( !requiredSelectors.isEmpty() || !optionalSelectors.isEmpty() )
{
- File reactorDirectory = getReactorDirectory( request );
+ Set<MavenProject> excludedProjects = new HashSet<>( requiredSelectors.size() + optionalSelectors.size() );
+ excludedProjects.addAll( getProjectsBySelectors( request, projects, requiredSelectors, true ) );
+ excludedProjects.addAll( getProjectsBySelectors( request, projects, optionalSelectors, false ) );
Review comment:
That's acceptable. I think we need to place a comment above that checking saying it is a workaround and create an issue with the actual cause for later analysis because as soon as this goes to master it will be forgotten!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] michael-o commented on a change in pull request #446: [MNG-6511] - Optional project selection
Posted by GitBox <gi...@apache.org>.
michael-o commented on a change in pull request #446:
URL: https://github.com/apache/maven/pull/446#discussion_r577076081
##########
File path: maven-core/src/main/java/org/apache/maven/graph/DefaultGraphBuilder.java
##########
@@ -242,21 +274,17 @@ public DefaultGraphBuilder( BuildResumptionDataRepository buildResumptionDataRep
{
List<MavenProject> result = projects;
- if ( !request.getExcludedProjects().isEmpty() )
+ ProjectActivation projectActivation = request.getProjectActivation();
+ Set<String> requiredSelectors = projectActivation.getRequiredInactiveProjectSelectors();
+ Set<String> optionalSelectors = projectActivation.getOptionalInactiveProjectSelectors();
+ if ( !requiredSelectors.isEmpty() || !optionalSelectors.isEmpty() )
{
- File reactorDirectory = getReactorDirectory( request );
+ Set<MavenProject> excludedProjects = new HashSet<>( requiredSelectors.size() + optionalSelectors.size() );
+ excludedProjects.addAll( getProjectsBySelectors( request, projects, requiredSelectors, true ) );
+ excludedProjects.addAll( getProjectsBySelectors( request, projects, optionalSelectors, false ) );
Review comment:
I can't tell what the correct behavior would be, but not saying no goals given. I guess this is a separate discussion.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] michael-o commented on a change in pull request #446: [MNG-6511] - Optional project selection
Posted by GitBox <gi...@apache.org>.
michael-o commented on a change in pull request #446:
URL: https://github.com/apache/maven/pull/446#discussion_r576411705
##########
File path: maven-core/src/main/java/org/apache/maven/graph/DefaultGraphBuilder.java
##########
@@ -242,21 +274,17 @@ public DefaultGraphBuilder( BuildResumptionDataRepository buildResumptionDataRep
{
List<MavenProject> result = projects;
- if ( !request.getExcludedProjects().isEmpty() )
+ ProjectActivation projectActivation = request.getProjectActivation();
+ Set<String> requiredSelectors = projectActivation.getRequiredInactiveProjectSelectors();
+ Set<String> optionalSelectors = projectActivation.getOptionalInactiveProjectSelectors();
+ if ( !requiredSelectors.isEmpty() || !optionalSelectors.isEmpty() )
{
- File reactorDirectory = getReactorDirectory( request );
+ Set<MavenProject> excludedProjects = new HashSet<>( requiredSelectors.size() + optionalSelectors.size() );
+ excludedProjects.addAll( getProjectsBySelectors( request, projects, requiredSelectors, true ) );
+ excludedProjects.addAll( getProjectsBySelectors( request, projects, optionalSelectors, false ) );
Review comment:
But that would actually only take care of the symptom, wouldn't it?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] michael-o commented on pull request #446: [MNG-6511] - Optional project selection
Posted by GitBox <gi...@apache.org>.
michael-o commented on pull request #446:
URL: https://github.com/apache/maven/pull/446#issuecomment-778660715
@MartinKanters Thank you, I will gladly check this one.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] michael-o commented on a change in pull request #446: [MNG-6511] - Optional project selection
Posted by GitBox <gi...@apache.org>.
michael-o commented on a change in pull request #446:
URL: https://github.com/apache/maven/pull/446#discussion_r583678292
##########
File path: maven-core/src/main/java/org/apache/maven/execution/ProjectActivation.java
##########
@@ -0,0 +1,173 @@
+package org.apache.maven.execution;
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import java.util.function.Predicate;
+
+import static java.util.stream.Collectors.toSet;
+
+/**
+ * Container for storing the request from the user to activate or de-activate certain projects and optionally fail the
+ * build if those projects do not exist.
+ */
+public class ProjectActivation
+{
+ private final Map<String, ActivationSettings> activations = new HashMap<>();
+
+ /**
+ * Adds a project activation to the request.
+ * @param selector The selector of the project.
Review comment:
Accepted.
----------------------------------------------------------------
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] famod commented on a change in pull request #446: [MNG-6511] - Optional project selection
Posted by GitBox <gi...@apache.org>.
famod commented on a change in pull request #446:
URL: https://github.com/apache/maven/pull/446#discussion_r579859091
##########
File path: maven-core/src/main/java/org/apache/maven/graph/DefaultGraphBuilder.java
##########
@@ -176,38 +177,69 @@ public DefaultGraphBuilder( BuildResumptionDataRepository buildResumptionDataRep
{
List<MavenProject> result = projects;
- if ( !request.getSelectedProjects().isEmpty() )
+ ProjectActivation projectActivation = request.getProjectActivation();
+ Set<String> requiredSelectors = projectActivation.getRequiredActiveProjectSelectors();
+ Set<String> optionalSelectors = projectActivation.getOptionalActiveProjectSelectors();
+ if ( !requiredSelectors.isEmpty() || !optionalSelectors.isEmpty() )
{
- File reactorDirectory = getReactorDirectory( request );
+ 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 );
+
+ result = includeAlsoMakeTransitively( result, request, graph );
+
+ // Order the new list in the original order
+ List<MavenProject> sortedProjects = graph.getSortedProjects();
+ result.sort( comparing( sortedProjects::indexOf ) );
+ }
+ }
+
+ return result;
+ }
- Collection<MavenProject> selectedProjects = new LinkedHashSet<>();
+ private Set<MavenProject> getProjectsBySelectors( MavenExecutionRequest request, List<MavenProject> projects,
+ Set<String> projectSelectors, boolean required )
+ throws MavenExecutionException
+ {
+ Set<MavenProject> selectedProjects = new LinkedHashSet<>();
+ File reactorDirectory = getReactorDirectory( request );
- for ( String selector : request.getSelectedProjects() )
+ for ( String selector : projectSelectors )
+ {
+ Optional<MavenProject> optSelectedProject = projects.stream()
+ .filter( project -> isMatchingProject( project, selector, reactorDirectory ) )
+ .findFirst();
+ if ( !optSelectedProject.isPresent() )
{
- MavenProject selectedProject = projects.stream()
- .filter( project -> isMatchingProject( project, selector, reactorDirectory ) )
- .findFirst()
- .orElseThrow( () -> new MavenExecutionException(
- "Could not find the selected project in the reactor: " + selector, request.getPom() ) );
- selectedProjects.add( selectedProject );
-
- List<MavenProject> children = selectedProject.getCollectedProjects();
- if ( children != null )
+ String message = "Could not find the selected project in the reactor: " + selector;
+ if ( required )
+ {
+ throw new MavenExecutionException( message, request.getPom() );
+ }
+ else
{
- selectedProjects.addAll( children );
+ LOGGER.warn( message );
Review comment:
TBH, I didn't follow the discussion of optional profiles, but shouldn't this be `info` instead?
I mean I as a user have already made very clear at this point (via "?") that the project is optional. So why warn me if it isn't there? For me this is purely informative.
IMO, same for optional profiles.
----------------------------------------------------------------
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 #446: [MNG-6511] - Optional project selection
Posted by GitBox <gi...@apache.org>.
mthmulders commented on a change in pull request #446:
URL: https://github.com/apache/maven/pull/446#discussion_r583665464
##########
File path: maven-core/src/main/java/org/apache/maven/execution/ProfileActivation.java
##########
@@ -74,7 +44,7 @@ static ActivationSettings of( final boolean active, final boolean optional )
@Deprecated
public List<String> getActiveProfiles()
{
- return new ArrayList<>( getProfileIds( pa -> pa.active ) );
+ return Collections.unmodifiableList( new ArrayList<>( getProfileIds( pa -> pa.active ) ) );
Review comment:
Moved those changes to #452 and force-pushed this branch.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] michael-o commented on a change in pull request #446: [MNG-6511] - Optional project selection
Posted by GitBox <gi...@apache.org>.
michael-o commented on a change in pull request #446:
URL: https://github.com/apache/maven/pull/446#discussion_r575836052
##########
File path: maven-core/src/main/java/org/apache/maven/execution/ProfileActivation.java
##########
@@ -74,7 +44,7 @@ static ActivationSettings of( final boolean active, final boolean optional )
@Deprecated
public List<String> getActiveProfiles()
{
- return new ArrayList<>( getProfileIds( pa -> pa.active ) );
+ return Collections.unmodifiableList( new ArrayList<>( getProfileIds( pa -> pa.active ) ) );
Review comment:
Agreed.
----------------------------------------------------------------
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 #446: [MNG-6511] - Optional project selection
Posted by GitBox <gi...@apache.org>.
MartinKanters commented on a change in pull request #446:
URL: https://github.com/apache/maven/pull/446#discussion_r575853056
##########
File path: maven-core/src/main/java/org/apache/maven/graph/DefaultGraphBuilder.java
##########
@@ -242,21 +274,17 @@ public DefaultGraphBuilder( BuildResumptionDataRepository buildResumptionDataRep
{
List<MavenProject> result = projects;
- if ( !request.getExcludedProjects().isEmpty() )
+ ProjectActivation projectActivation = request.getProjectActivation();
+ Set<String> requiredSelectors = projectActivation.getRequiredInactiveProjectSelectors();
+ Set<String> optionalSelectors = projectActivation.getOptionalInactiveProjectSelectors();
+ if ( !requiredSelectors.isEmpty() || !optionalSelectors.isEmpty() )
{
- File reactorDirectory = getReactorDirectory( request );
+ Set<MavenProject> excludedProjects = new HashSet<>( requiredSelectors.size() + optionalSelectors.size() );
+ excludedProjects.addAll( getProjectsBySelectors( request, projects, requiredSelectors, true ) );
+ excludedProjects.addAll( getProjectsBySelectors( request, projects, optionalSelectors, false ) );
Review comment:
Actually.. this is the case for 3.6.3, the latest master actually builds the targeted project just fine. I'm not sure why it's not filtered out... Let me get back to that.
----------------------------------------------------------------
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 #446: [MNG-6511] - Optional project selection
Posted by GitBox <gi...@apache.org>.
MartinKanters commented on a change in pull request #446:
URL: https://github.com/apache/maven/pull/446#discussion_r575810315
##########
File path: maven-core/src/main/java/org/apache/maven/execution/ActivationSettings.java
##########
@@ -0,0 +1,58 @@
+package org.apache.maven.execution;
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+/**
+ * Describes whether something (a profile or a project) should be activated or not, and if that is required or optional.
Review comment:
I agree, changed it accordingly
----------------------------------------------------------------
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 #446: [MNG-6511] - Optional project selection
Posted by GitBox <gi...@apache.org>.
MartinKanters commented on a change in pull request #446:
URL: https://github.com/apache/maven/pull/446#discussion_r575807350
##########
File path: maven-core/src/main/java/org/apache/maven/execution/ProfileActivation.java
##########
@@ -74,7 +44,7 @@ static ActivationSettings of( final boolean active, final boolean optional )
@Deprecated
public List<String> getActiveProfiles()
{
- return new ArrayList<>( getProfileIds( pa -> pa.active ) );
+ return Collections.unmodifiableList( new ArrayList<>( getProfileIds( pa -> pa.active ) ) );
Review comment:
I understand your concern and usually we wouldn't have changed something like this all of a sudden. The reason why we did change it is because the current implementation returns a view of the active profiles in this case. If some extension or whatever would add elements to this list it will not have any effect. Our theory is that if we make it unmodifiable, we at throw an exception if someone tries this. This hopefully points the dev to the method and they notice the replacement methods.
----------------------------------------------------------------
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] famod commented on pull request #446: [MNG-6511] - Optional project selection
Posted by GitBox <gi...@apache.org>.
famod commented on pull request #446:
URL: https://github.com/apache/maven/pull/446#issuecomment-782919336
I might be blind, but shouldn't this be documented in `-help`?
As of 3.6.3, "!" isn't documented either and should be added as well.
----------------------------------------------------------------
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 #446: [MNG-6511] - Optional project selection
Posted by GitBox <gi...@apache.org>.
MartinKanters commented on a change in pull request #446:
URL: https://github.com/apache/maven/pull/446#discussion_r575853056
##########
File path: maven-core/src/main/java/org/apache/maven/graph/DefaultGraphBuilder.java
##########
@@ -242,21 +274,17 @@ public DefaultGraphBuilder( BuildResumptionDataRepository buildResumptionDataRep
{
List<MavenProject> result = projects;
- if ( !request.getExcludedProjects().isEmpty() )
+ ProjectActivation projectActivation = request.getProjectActivation();
+ Set<String> requiredSelectors = projectActivation.getRequiredInactiveProjectSelectors();
+ Set<String> optionalSelectors = projectActivation.getOptionalInactiveProjectSelectors();
+ if ( !requiredSelectors.isEmpty() || !optionalSelectors.isEmpty() )
{
- File reactorDirectory = getReactorDirectory( request );
+ Set<MavenProject> excludedProjects = new HashSet<>( requiredSelectors.size() + optionalSelectors.size() );
+ excludedProjects.addAll( getProjectsBySelectors( request, projects, requiredSelectors, true ) );
+ excludedProjects.addAll( getProjectsBySelectors( request, projects, optionalSelectors, false ) );
Review comment:
Actually.. this is the case for 3.6.3, the latest master actually builds the targeted project just fine. I'm not sure it's not filtered out... Let me get back to that.
----------------------------------------------------------------
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] famod commented on a change in pull request #446: [MNG-6511] - Optional project selection
Posted by GitBox <gi...@apache.org>.
famod commented on a change in pull request #446:
URL: https://github.com/apache/maven/pull/446#discussion_r579859091
##########
File path: maven-core/src/main/java/org/apache/maven/graph/DefaultGraphBuilder.java
##########
@@ -176,38 +177,69 @@ public DefaultGraphBuilder( BuildResumptionDataRepository buildResumptionDataRep
{
List<MavenProject> result = projects;
- if ( !request.getSelectedProjects().isEmpty() )
+ ProjectActivation projectActivation = request.getProjectActivation();
+ Set<String> requiredSelectors = projectActivation.getRequiredActiveProjectSelectors();
+ Set<String> optionalSelectors = projectActivation.getOptionalActiveProjectSelectors();
+ if ( !requiredSelectors.isEmpty() || !optionalSelectors.isEmpty() )
{
- File reactorDirectory = getReactorDirectory( request );
+ 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 );
+
+ result = includeAlsoMakeTransitively( result, request, graph );
+
+ // Order the new list in the original order
+ List<MavenProject> sortedProjects = graph.getSortedProjects();
+ result.sort( comparing( sortedProjects::indexOf ) );
+ }
+ }
+
+ return result;
+ }
- Collection<MavenProject> selectedProjects = new LinkedHashSet<>();
+ private Set<MavenProject> getProjectsBySelectors( MavenExecutionRequest request, List<MavenProject> projects,
+ Set<String> projectSelectors, boolean required )
+ throws MavenExecutionException
+ {
+ Set<MavenProject> selectedProjects = new LinkedHashSet<>();
+ File reactorDirectory = getReactorDirectory( request );
- for ( String selector : request.getSelectedProjects() )
+ for ( String selector : projectSelectors )
+ {
+ Optional<MavenProject> optSelectedProject = projects.stream()
+ .filter( project -> isMatchingProject( project, selector, reactorDirectory ) )
+ .findFirst();
+ if ( !optSelectedProject.isPresent() )
{
- MavenProject selectedProject = projects.stream()
- .filter( project -> isMatchingProject( project, selector, reactorDirectory ) )
- .findFirst()
- .orElseThrow( () -> new MavenExecutionException(
- "Could not find the selected project in the reactor: " + selector, request.getPom() ) );
- selectedProjects.add( selectedProject );
-
- List<MavenProject> children = selectedProject.getCollectedProjects();
- if ( children != null )
+ String message = "Could not find the selected project in the reactor: " + selector;
+ if ( required )
+ {
+ throw new MavenExecutionException( message, request.getPom() );
+ }
+ else
{
- selectedProjects.addAll( children );
+ LOGGER.warn( message );
Review comment:
TBH, I didn't follow the discussion of optional profiles, but shouldn't this be `info` instead?
I mean I as a user have already made very clear at this point (via "?") that the project is optional. So why warn me if it isn't there? For me this is purely informative.
IMO, the same applies to optional profiles.
----------------------------------------------------------------
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 #446: [MNG-6511] - Optional project selection
Posted by GitBox <gi...@apache.org>.
mthmulders commented on a change in pull request #446:
URL: https://github.com/apache/maven/pull/446#discussion_r583679349
##########
File path: maven-core/src/main/java/org/apache/maven/graph/DefaultGraphBuilder.java
##########
@@ -242,21 +274,17 @@ public DefaultGraphBuilder( BuildResumptionDataRepository buildResumptionDataRep
{
List<MavenProject> result = projects;
- if ( !request.getExcludedProjects().isEmpty() )
+ ProjectActivation projectActivation = request.getProjectActivation();
+ Set<String> requiredSelectors = projectActivation.getRequiredInactiveProjectSelectors();
+ Set<String> optionalSelectors = projectActivation.getOptionalInactiveProjectSelectors();
+ if ( !requiredSelectors.isEmpty() || !optionalSelectors.isEmpty() )
{
- File reactorDirectory = getReactorDirectory( request );
+ Set<MavenProject> excludedProjects = new HashSet<>( requiredSelectors.size() + optionalSelectors.size() );
+ excludedProjects.addAll( getProjectsBySelectors( request, projects, requiredSelectors, true ) );
+ excludedProjects.addAll( getProjectsBySelectors( request, projects, optionalSelectors, false ) );
Review comment:
We discussed this with @rfscholte and the idea is to:
1. If the user gives conflicting settings for one project, they will cancel each other out: `-pl :a,-:a,:b` will yield selection of `:b`.
2. When the above rule yields no selected projects (everything is cancelled out), we throw ["empty project list" message](https://github.com/apache/maven/pull/446#discussion_r577123679).
3. When `?:a,:a` is specified, `:a` wins because it's "stronger".
4. When `a,:a` is specified (two selectors that yield the same module) the above rules apply as they refer to the same module. It is not considered a failure or error.
What are your thoughts on this?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] MartinKanters commented on a change in pull request #446: [MNG-6511] - Optional project selection
Posted by GitBox <gi...@apache.org>.
MartinKanters commented on a change in pull request #446:
URL: https://github.com/apache/maven/pull/446#discussion_r577045936
##########
File path: maven-core/src/main/java/org/apache/maven/graph/DefaultGraphBuilder.java
##########
@@ -242,21 +274,17 @@ public DefaultGraphBuilder( BuildResumptionDataRepository buildResumptionDataRep
{
List<MavenProject> result = projects;
- if ( !request.getExcludedProjects().isEmpty() )
+ ProjectActivation projectActivation = request.getProjectActivation();
+ Set<String> requiredSelectors = projectActivation.getRequiredInactiveProjectSelectors();
+ Set<String> optionalSelectors = projectActivation.getOptionalInactiveProjectSelectors();
+ if ( !requiredSelectors.isEmpty() || !optionalSelectors.isEmpty() )
{
- File reactorDirectory = getReactorDirectory( request );
+ Set<MavenProject> excludedProjects = new HashSet<>( requiredSelectors.size() + optionalSelectors.size() );
+ excludedProjects.addAll( getProjectsBySelectors( request, projects, requiredSelectors, true ) );
+ excludedProjects.addAll( getProjectsBySelectors( request, projects, optionalSelectors, false ) );
Review comment:
I agree, but I don't think I understand how it would ideally work. Would you want to let `-a,+:a` work the same way as `-a,+a` instead of have the exception thrown? (last one wins)
----------------------------------------------------------------
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 #446: [MNG-6511] - Optional project selection
Posted by GitBox <gi...@apache.org>.
MartinKanters commented on pull request #446:
URL: https://github.com/apache/maven/pull/446#issuecomment-778660643
@michael-o FYI, this PR is very similar to the optional profile one. As you wanted to check that one, perhaps you would like to review this as well.
----------------------------------------------------------------
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 #446: [MNG-6511] - Optional project selection
Posted by GitBox <gi...@apache.org>.
MartinKanters commented on pull request #446:
URL: https://github.com/apache/maven/pull/446#issuecomment-795036507
> I might be blind, but shouldn't this be documented in `-help`?
> As of 3.6.3, "!" isn't documented either and should be added as well.
I just noticed I never replied. I agree with you, we'll pick up the help text and the maven site docs once all work for -pl is implemented. (which includes https://issues.apache.org/jira/browse/MNG-7112 as well)
----------------------------------------------------------------
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