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/09/16 17:49:50 UTC
[GitHub] [maven-invoker-plugin] slawekjaranowski opened a new pull request #68: [MINVOKER-271] fix detection of setup projects
slawekjaranowski opened a new pull request #68:
URL: https://github.com/apache/maven-invoker-plugin/pull/68
fix implementation according to documentations on params:
- invokerTest
- pomExcludes
- setupIncludes
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-invoker-plugin] slawekjaranowski commented on pull request #68: [MINVOKER-271] fix detection of setup projects
Posted by GitBox <gi...@apache.org>.
slawekjaranowski commented on pull request #68:
URL: https://github.com/apache/maven-invoker-plugin/pull/68#issuecomment-982858029
@cstamas , @slachiewicz - so if no more remarks please merge.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-invoker-plugin] slawekjaranowski commented on pull request #68: [MINVOKER-271] fix detection of setup projects
Posted by GitBox <gi...@apache.org>.
slawekjaranowski commented on pull request #68:
URL: https://github.com/apache/maven-invoker-plugin/pull/68#issuecomment-945811597
Can somebody help with review?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-invoker-plugin] slawekjaranowski commented on pull request #68: [MINVOKER-271] fix detection of setup projects
Posted by GitBox <gi...@apache.org>.
slawekjaranowski commented on pull request #68:
URL: https://github.com/apache/maven-invoker-plugin/pull/68#issuecomment-955242877
@slachiewicz , @cstamas ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-invoker-plugin] slawekjaranowski commented on pull request #68: [MINVOKER-271] fix detection of setup projects
Posted by GitBox <gi...@apache.org>.
slawekjaranowski commented on pull request #68:
URL: https://github.com/apache/maven-invoker-plugin/pull/68#issuecomment-926587840
kindly reminder about review
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-invoker-plugin] michael-o commented on pull request #68: [MINVOKER-271] fix detection of setup projects
Posted by GitBox <gi...@apache.org>.
michael-o commented on pull request #68:
URL: https://github.com/apache/maven-invoker-plugin/pull/68#issuecomment-981138687
> @michael-o kindly reminder 😄
TBH, no clue here. I have never worked on Invoker before. @cstamas If you are certain that the change is correct, please merge this month.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-invoker-plugin] slawekjaranowski commented on pull request #68: [MINVOKER-271] fix detection of setup projects
Posted by GitBox <gi...@apache.org>.
slawekjaranowski commented on pull request #68:
URL: https://github.com/apache/maven-invoker-plugin/pull/68#issuecomment-981137708
@michael-o kindly reminder 😄
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-invoker-plugin] slachiewicz closed pull request #68: [MINVOKER-271] fix detection of setup projects
Posted by GitBox <gi...@apache.org>.
slachiewicz closed pull request #68:
URL: https://github.com/apache/maven-invoker-plugin/pull/68
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-invoker-plugin] slawekjaranowski commented on a change in pull request #68: [MINVOKER-271] fix detection of setup projects
Posted by GitBox <gi...@apache.org>.
slawekjaranowski commented on a change in pull request #68:
URL: https://github.com/apache/maven-invoker-plugin/pull/68#discussion_r716018370
##########
File path: pom.xml
##########
@@ -275,6 +275,18 @@ under the License.
<version>${mavenVersion}</version>
<scope>test</scope>
</dependency>
+ <dependency>
+ <groupId>org.assertj</groupId>
+ <artifactId>assertj-core</artifactId>
+ <version>3.20.2</version>
+ <scope>test</scope>
+ </dependency>
+ <dependency>
+ <groupId>org.slf4j</groupId>
+ <artifactId>slf4j-simple</artifactId>
+ <version>1.7.32</version>
+ <scope>test</scope>
+ </dependency>
Review comment:
we have slf4j on classpath, coming from maven-artifact-transfer and maven-script-interpreter so in order to avoid worning about missing binding during unit test I added it.
##########
File path: src/main/java/org/apache/maven/plugins/invoker/AbstractInvokerMojo.java
##########
@@ -2399,11 +2396,46 @@ else if ( !invokerProperties.isExpectedResult( result.getExitCode(), invocationI
}
}
+ private List<String> calculateIncludes()
+ {
+ if ( invokerTest != null )
+ {
+ String[] testRegexes = StringUtils.split( invokerTest, "," );
+ return Arrays.stream( testRegexes )
+ .map( String::trim )
+ .filter( s -> !s.isEmpty() )
+ .filter( s -> !s.startsWith( "!" ) )
+ .collect( Collectors.toList() );
+ }
+ else
+ {
+ Set<String> uniqueIncludes = new HashSet<>();
+ uniqueIncludes.addAll( pomIncludes );
+ uniqueIncludes.addAll( setupIncludes );
+ return new ArrayList<>( uniqueIncludes );
+ }
+ }
+
private List<String> calculateExcludes()
throws IOException
{
- List<String> excludes =
- ( pomExcludes != null ) ? new ArrayList<>( pomExcludes ) : new ArrayList<>();
+ List<String> excludes;
+
+ if ( invokerTest != null )
+ {
+ String[] testRegexes = StringUtils.split( invokerTest, "," );
+ excludes = Arrays.stream( testRegexes )
+ .map( String::trim )
+ .filter( s -> !s.isEmpty() )
+ .filter( s -> s.startsWith( "!" ) )
+ .map( s -> s.substring( 1 ) )
+ .collect( Collectors.toList() );
+ }
+ else
+ {
+ excludes = pomExcludes != null ? new ArrayList<>( pomExcludes ) : new ArrayList<>();
+ }
+
Review comment:
the sama as `calculateIncludes` do in one place
##########
File path: src/main/mdo/invocation.mdo
##########
@@ -197,11 +196,6 @@ under the License.
*/
public static final String NORMAL = "normal";
- /**
- * A build job that was directly selected via the <code>-Dinvoker.test=xxx,yyy</code> parameter.
- */
- public static final String DIRECT = "direct";
-
Review comment:
simplify code - job type is set during scan jobs in the same way - it not depends on invoker.test so this type is no more used
##########
File path: src/main/java/org/apache/maven/plugins/invoker/AbstractInvokerMojo.java
##########
@@ -796,23 +797,13 @@ else if ( lastModifiedRecursive( projectsDirectory ) <= lastModifiedRecursive( c
}
// First run setup jobs.
- List<BuildJob> setupBuildJobs = null;
- try
- {
- setupBuildJobs = getSetupBuildJobsFromFolders();
- }
- catch ( IOException e )
- {
- getLog().error( "Failure during scanning of folders.", e );
- // TODO: Check shouldn't we fail in case of problems?
- }
+ List<BuildJob> setupBuildJobs = getSetupJobs( buildJobs );
Review comment:
method `getBuildJobs` returns now jobs with proper type - no separate method and way for searching setup jobs
##########
File path: src/main/java/org/apache/maven/plugins/invoker/AbstractInvokerMojo.java
##########
@@ -2417,25 +2449,6 @@ else if ( !invokerProperties.isExpectedResult( result.getExitCode(), invocationI
}
- /**
- * @return The list of setupUp jobs.
- * @throws IOException
- * @see {@link #setupIncludes}
- */
- private List<BuildJob> getSetupBuildJobsFromFolders()
- throws IOException, MojoExecutionException
- {
- List<String> excludes = calculateExcludes();
-
- List<BuildJob> setupPoms = scanProjectsDirectory( setupIncludes, excludes, BuildJob.Type.SETUP );
- if ( getLog().isDebugEnabled() )
- {
- getLog().debug( "Setup projects: " + setupPoms );
- }
-
- return setupPoms;
- }
-
Review comment:
`getBuildJobs` return all jobs with type - we needn't separate method
##########
File path: src/main/java/org/apache/maven/plugins/invoker/AbstractInvokerMojo.java
##########
@@ -2399,11 +2396,46 @@ else if ( !invokerProperties.isExpectedResult( result.getExitCode(), invocationI
}
}
+ private List<String> calculateIncludes()
+ {
+ if ( invokerTest != null )
+ {
+ String[] testRegexes = StringUtils.split( invokerTest, "," );
+ return Arrays.stream( testRegexes )
+ .map( String::trim )
+ .filter( s -> !s.isEmpty() )
+ .filter( s -> !s.startsWith( "!" ) )
+ .collect( Collectors.toList() );
+ }
+ else
+ {
+ Set<String> uniqueIncludes = new HashSet<>();
+ uniqueIncludes.addAll( pomIncludes );
+ uniqueIncludes.addAll( setupIncludes );
+ return new ArrayList<>( uniqueIncludes );
+ }
+ }
+
Review comment:
according to documentation and assumptions invokerTest should override rest params, so calculate it in one place
##########
File path: src/main/java/org/apache/maven/plugins/invoker/AbstractInvokerMojo.java
##########
@@ -741,7 +742,7 @@ public void execute()
+ "pom File parameter. Reason: " + e.getMessage(), e );
}
- buildJobs = Collections.singletonList( new BuildJob( pom.getName(), BuildJob.Type.NORMAL ) );
+ buildJobs = Collections.singletonList( new BuildJob( pom.getName() ) );
Review comment:
now default value is NORMAL
##########
File path: src/main/java/org/apache/maven/plugins/invoker/AbstractInvokerMojo.java
##########
@@ -2456,62 +2469,35 @@ public int compare( BuildJob o1, BuildJob o2 )
List<BuildJob> getBuildJobs()
throws IOException, MojoExecutionException
{
- List<BuildJob> buildJobs;
- if ( invokerTest == null )
- {
- List<String> excludes = calculateExcludes();
+ List<String> includes = calculateIncludes();
+ List<String> excludes = calculateExcludes();
+ List<BuildJob> buildJobsAll = scanProjectsDirectory( includes, excludes );
+ List<BuildJob> buildJobsSetup = scanProjectsDirectory( setupIncludes, excludes );
- List<BuildJob> setupPoms = scanProjectsDirectory( setupIncludes, excludes, BuildJob.Type.SETUP );
- if ( getLog().isDebugEnabled() )
- {
- getLog().debug( "Setup projects: " + Collections.singletonList( setupPoms ) );
- }
+ List<String> setupProjects = buildJobsSetup.stream()
+ .map( BuildJob::getProject )
+ .collect( Collectors.toList() );
- List<BuildJob> normalPoms = scanProjectsDirectory( pomIncludes, excludes, BuildJob.Type.NORMAL );
- Map<String, BuildJob> uniquePoms = new LinkedHashMap<>();
- for ( BuildJob setupPom : setupPoms )
- {
- uniquePoms.put( setupPom.getProject(), setupPom );
- }
- for ( BuildJob normalPom : normalPoms )
- {
- if ( !uniquePoms.containsKey( normalPom.getProject() ) )
- {
- uniquePoms.put( normalPom.getProject(), normalPom );
- }
- }
-
- buildJobs = new ArrayList<>( uniquePoms.values() );
- }
- else
+ for ( BuildJob job : buildJobsAll )
{
- String[] testRegexes = StringUtils.split( invokerTest, "," );
- List<String> includes = new ArrayList<>( testRegexes.length );
- List<String> excludes = new ArrayList<>();
-
- for ( String regex : testRegexes )
+ if ( setupProjects.contains( job.getProject() ) )
{
- // user just use -Dinvoker.test=MWAR191,MNG111 to use a directory thats the end is not pom.xml
- if ( regex.startsWith( "!" ) )
- {
- excludes.add( regex.substring( 1 ) );
- }
- else
- {
- includes.add( regex );
- }
+ job.setType( BuildJob.Type.SETUP );
}
-
Review comment:
simplify joss searching - calculate includes and excludes in the same way
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-invoker-plugin] slawekjaranowski commented on pull request #68: [MINVOKER-271] fix detection of setup projects
Posted by GitBox <gi...@apache.org>.
slawekjaranowski commented on pull request #68:
URL: https://github.com/apache/maven-invoker-plugin/pull/68#issuecomment-973096877
Thanks for approval.
@michael-o do you want to review also? ... or can be merged?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org