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 2022/01/15 17:16:44 UTC
[GitHub] [maven] hboutemy opened a new pull request #661: [MNG-7391] add execution strategy+runner required by Maven Build Cache
hboutemy opened a new pull request #661:
URL: https://github.com/apache/maven/pull/661
https://issues.apache.org/jira/browse/MNG-7391
supercedes #607
--
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] gnodet commented on a change in pull request #661: [MNG-7391] add execution strategy+runner required by Maven Build Cache
Posted by GitBox <gi...@apache.org>.
gnodet commented on a change in pull request #661:
URL: https://github.com/apache/maven/pull/661#discussion_r801619727
##########
File path: maven-core/src/test/java/org/apache/maven/lifecycle/internal/stub/MojoExecutorStub.java
##########
@@ -47,24 +48,23 @@ public MojoExecutorStub(
BuildPluginManager pluginManager,
MavenPluginManager mavenPluginManager,
LifecycleDependencyResolver lifeCycleDependencyResolver,
- ExecutionEventCatapult eventCatapult )
+ ExecutionEventCatapult eventCatapult,
+ MojosExecutionStrategy mojosExecutionStrategy )
{
- super( pluginManager, mavenPluginManager, lifeCycleDependencyResolver, eventCatapult );
+ super( pluginManager, mavenPluginManager, lifeCycleDependencyResolver, eventCatapult, mojosExecutionStrategy );
}
@Override
- public void execute( MavenSession session, MojoExecution mojoExecution, ProjectIndex projectIndex,
- DependencyContext dependencyContext, PhaseRecorder phaseRecorder )
+ public void execute( MavenSession session, List<MojoExecution> mojoExecutions, ProjectIndex projectIndex )
throws LifecycleExecutionException
{
- executions.add( mojoExecution );
+ executions.addAll( mojoExecutions );
}
@Override
- public void execute( MavenSession session, List<MojoExecution> mojoExecutions, ProjectIndex projectIndex )
- throws LifecycleExecutionException
+ public List<MavenProject> executeForkedExecutions( MojoExecution mojoExecution, MavenSession session, ProjectIndex projectIndex ) throws LifecycleExecutionException
{
- executions.addAll(mojoExecutions);
+ return null;
Review comment:
The `MojoExecutor` class is modified by the PR, so we need to adjust the corresponding stub.
The two methods that are overriden are actually not the same than before, as you can see, both signatures have changed. The first method became private and the stub now overrides a higher level one, which has a list as a parameter instead of a single `MojoExecution`. The second method is unrelated and not returns null as a stub.
--
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] michael-o commented on a change in pull request #661: [MNG-7391] add execution strategy+runner required by Maven Build Cache
Posted by GitBox <gi...@apache.org>.
michael-o commented on a change in pull request #661:
URL: https://github.com/apache/maven/pull/661#discussion_r801526860
##########
File path: maven-core/src/test/java/org/apache/maven/lifecycle/internal/stub/MojoExecutorStub.java
##########
@@ -47,24 +48,23 @@ public MojoExecutorStub(
BuildPluginManager pluginManager,
MavenPluginManager mavenPluginManager,
LifecycleDependencyResolver lifeCycleDependencyResolver,
- ExecutionEventCatapult eventCatapult )
+ ExecutionEventCatapult eventCatapult,
+ MojosExecutionStrategy mojosExecutionStrategy )
{
- super( pluginManager, mavenPluginManager, lifeCycleDependencyResolver, eventCatapult );
+ super( pluginManager, mavenPluginManager, lifeCycleDependencyResolver, eventCatapult, mojosExecutionStrategy );
}
@Override
- public void execute( MavenSession session, MojoExecution mojoExecution, ProjectIndex projectIndex,
- DependencyContext dependencyContext, PhaseRecorder phaseRecorder )
+ public void execute( MavenSession session, List<MojoExecution> mojoExecutions, ProjectIndex projectIndex )
throws LifecycleExecutionException
{
- executions.add( mojoExecution );
+ executions.addAll( mojoExecutions );
}
@Override
- public void execute( MavenSession session, List<MojoExecution> mojoExecutions, ProjectIndex projectIndex )
- throws LifecycleExecutionException
+ public List<MavenProject> executeForkedExecutions( MojoExecution mojoExecution, MavenSession session, ProjectIndex projectIndex ) throws LifecycleExecutionException
{
- executions.addAll(mojoExecutions);
+ return null;
Review comment:
Can you please explain those two changes. Why is the first one now `addAll` and the second one `null`?
--
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] michael-o commented on a change in pull request #661: [MNG-7391] add execution strategy+runner required by Maven Build Cache
Posted by GitBox <gi...@apache.org>.
michael-o commented on a change in pull request #661:
URL: https://github.com/apache/maven/pull/661#discussion_r801511196
##########
File path: maven-core/src/main/java/org/apache/maven/lifecycle/DefaultLifecycles.java
##########
@@ -45,7 +45,7 @@
@Singleton
public class DefaultLifecycles
{
- public static final String[] STANDARD_LIFECYCLES = { "default", "clean", "site", "wrapper" };
+ public static final String[] STANDARD_LIFECYCLES = { "clean", "default", "site", "wrapper" };
Review comment:
I see, your explanation makes sense and simplifies the process.
--
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] gnodet commented on a change in pull request #661: [MNG-7391] add execution strategy+runner required by Maven Build Cache
Posted by GitBox <gi...@apache.org>.
gnodet commented on a change in pull request #661:
URL: https://github.com/apache/maven/pull/661#discussion_r801487087
##########
File path: maven-core/src/main/java/org/apache/maven/lifecycle/DefaultLifecycles.java
##########
@@ -45,7 +45,7 @@
@Singleton
public class DefaultLifecycles
{
- public static final String[] STANDARD_LIFECYCLES = { "default", "clean", "site", "wrapper" };
+ public static final String[] STANDARD_LIFECYCLES = { "clean", "default", "site", "wrapper" };
Review comment:
Yes, the change is required.
The current code (without this PR) provides two different ordering for the lifecycles, one is given by `DefaultLifecycles#getLifeCycles()` which sorts the lifecycles according to the `STANDARD_LIFECYCLES` array. The other one comes `DefaultLifecyclePluginAnalyzer#getOrderedLifecycles()` which sorts the lifecycles alphabetically.
My analysis is that the first order isn't really used, while the second one is actually while computing the goals that will be invoked from the phases given on the command line.
I thus changed the first order to actually return the same thing that the second one and not rely on alphabetical ordering which imho does not make sense semantically : the `clean` phase should be executed before the `default` and the `site` after the `default`, and the alphabetical order is not really relevant.
If it's problematic, I'll revert that part and will provide another sort just for the use 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.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] gnodet commented on a change in pull request #661: [MNG-7391] add execution strategy+runner required by Maven Build Cache
Posted by GitBox <gi...@apache.org>.
gnodet commented on a change in pull request #661:
URL: https://github.com/apache/maven/pull/661#discussion_r801487087
##########
File path: maven-core/src/main/java/org/apache/maven/lifecycle/DefaultLifecycles.java
##########
@@ -45,7 +45,7 @@
@Singleton
public class DefaultLifecycles
{
- public static final String[] STANDARD_LIFECYCLES = { "default", "clean", "site", "wrapper" };
+ public static final String[] STANDARD_LIFECYCLES = { "clean", "default", "site", "wrapper" };
Review comment:
Yes, the change is required.
The current code (without this PR) provides two different ordering for the lifecycles, one is given by `DefaultLifecycles#getLifeCycles()` which sorts the lifecycles according to the `STANDARD_LIFECYCLES` array. The other one comes `DefaultLifecyclePluginAnalyzer#getOrderedLifecycles()` which sorts the lifecycles alphabetically.
My analysis is that the first order isn't really used, while the second one is actually used during the computation of the goals that will be invoked given the phases from the command line.
I thus changed the first ordering to actually return the same thing than the second one and not rely on alphabetical ordering which imho does not make sense semantically : the `clean` phase should be executed before the `default` and the `site` after the `default`, and the alphabetical order is not really relevant.
If it's problematic, I'll revert that part and will provide another sort just for the use 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.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] michael-o commented on a change in pull request #661: [MNG-7391] add execution strategy+runner required by Maven Build Cache
Posted by GitBox <gi...@apache.org>.
michael-o commented on a change in pull request #661:
URL: https://github.com/apache/maven/pull/661#discussion_r801055680
##########
File path: maven-core/src/main/java/org/apache/maven/lifecycle/DefaultLifecycles.java
##########
@@ -45,7 +45,7 @@
@Singleton
public class DefaultLifecycles
{
- public static final String[] STANDARD_LIFECYCLES = { "default", "clean", "site", "wrapper" };
+ public static final String[] STANDARD_LIFECYCLES = { "clean", "default", "site", "wrapper" };
Review comment:
Does the order make change make any difference since the next hunk says so?!
--
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] michael-o commented on a change in pull request #661: [MNG-7391] add execution strategy+runner required by Maven Build Cache
Posted by GitBox <gi...@apache.org>.
michael-o commented on a change in pull request #661:
URL: https://github.com/apache/maven/pull/661#discussion_r801056493
##########
File path: maven-core/src/main/java/org/apache/maven/plugin/MojoExecutionRunner.java
##########
@@ -0,0 +1,36 @@
+package org.apache.maven.plugin;
+
+/*
+ * 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 org.apache.maven.lifecycle.LifecycleExecutionException;
+
+/**
+ * Provides context for mojo execution. Invocation of #run will result in actual execution
Review comment:
`#run` should be a Javadoc link.
--
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] gnodet merged pull request #661: [MNG-7391] add execution strategy+runner required by Maven Build Cache
Posted by GitBox <gi...@apache.org>.
gnodet merged pull request #661:
URL: https://github.com/apache/maven/pull/661
--
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