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/11/03 12:58:53 UTC
[GitHub] [maven] gnodet opened a new pull request #604: Make sure forked mojo executions have a correct phase set
gnodet opened a new pull request #604:
URL: https://github.com/apache/maven/pull/604
This fixes a possible exception when using forked lifecycles.
--
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] hboutemy commented on pull request #604: Finish switch to modello
Posted by GitBox <gi...@apache.org>.
hboutemy commented on pull request #604:
URL: https://github.com/apache/maven/pull/604#issuecomment-962465272
> @hboutemy should I commit them directly to the MNG-7129 branch ?
@gnodet my idea is that you should commit directly to MNG-7129_Modello = the place where we prepare the PR that we will review all together before merging to MNG-7129
--
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 #604: Finish switch to modello
Posted by GitBox <gi...@apache.org>.
gnodet commented on a change in pull request #604:
URL: https://github.com/apache/maven/pull/604#discussion_r744843355
##########
File path: maven-core/src/main/java/org/apache/maven/lifecycle/internal/DefaultLifecycleExecutionPlanCalculator.java
##########
@@ -554,6 +554,8 @@ private void injectLifecycleOverlay( Map<String, List<MojoExecution>> lifecycleM
MojoExecution forkedExecution = new MojoExecution( forkedMojoDescriptor, forkedGoal );
+ forkedExecution.setLifecyclePhase( mojoExecution.getLifecyclePhase() );
Review comment:
@AlexanderAshitkin I don't see why the modello generated code could not be moved into the extension.
--
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] hboutemy commented on a change in pull request #604: Finish switch to modello
Posted by GitBox <gi...@apache.org>.
hboutemy commented on a change in pull request #604:
URL: https://github.com/apache/maven/pull/604#discussion_r744135057
##########
File path: maven-core/src/main/java/org/apache/maven/lifecycle/internal/DefaultLifecycleExecutionPlanCalculator.java
##########
@@ -554,6 +554,8 @@ private void injectLifecycleOverlay( Map<String, List<MojoExecution>> lifecycleM
MojoExecution forkedExecution = new MojoExecution( forkedMojoDescriptor, forkedGoal );
+ forkedExecution.setLifecyclePhase( mojoExecution.getLifecyclePhase() );
Review comment:
how does that relate to MNG-7129 and the switch from JAXB to Modello?
--
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 pull request #604: Make sure forked mojo executions have a correct phase set
Posted by GitBox <gi...@apache.org>.
gnodet commented on pull request #604:
URL: https://github.com/apache/maven/pull/604#issuecomment-959066781
--
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 pull request #604: Make sure forked mojo executions have a correct phase set
Posted by GitBox <gi...@apache.org>.
gnodet commented on pull request #604:
URL: https://github.com/apache/maven/pull/604#issuecomment-959066781
--
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] hboutemy commented on pull request #604: Finish switch to modello
Posted by GitBox <gi...@apache.org>.
hboutemy commented on pull request #604:
URL: https://github.com/apache/maven/pull/604#issuecomment-962485124
ok, I did the cherry-picks and the rebase: I think we can close this PR
I'm now cleaning up unused dependencies
--
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] hboutemy commented on a change in pull request #604: Finish switch to modello
Posted by GitBox <gi...@apache.org>.
hboutemy commented on a change in pull request #604:
URL: https://github.com/apache/maven/pull/604#discussion_r744144590
##########
File path: maven-core/pom.xml
##########
@@ -235,11 +235,16 @@ under the License.
<goals>
<goal>java</goal>
<goal>xpp3-reader</goal>
+ <goal>xpp3-writer</goal>
Review comment:
do we really need a writer for cache files?
--
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] hboutemy commented on pull request #604: Finish switch to modello
Posted by GitBox <gi...@apache.org>.
hboutemy commented on pull request #604:
URL: https://github.com/apache/maven/pull/604#issuecomment-962481153
ok, now that I reviewed and understood many parts, here my proposal:
1. the first 2 commits should be directly be done in MNG-7129 branch: they are nice little fixes independent from Modello switch
2. last 3 commits should go directly to MNG-7129_Modello branch (that will have to be rebased on MNG-7129)
and the next update I want on MNG-7129_Modello branch is the removal of all dependencies that were added for JAXB: removing them is a key benefit
the full idea behind splitting everything in many buckets is that both original developers of MNG-7129 need to be able to review/understand what is done to their original code: for usual Maven contributors, all the work is about lowering the bar to understanding the global MNG-7129 branch to be able to merge some time later to master
--
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] AlexanderAshitkin commented on a change in pull request #604: Finish switch to modello
Posted by GitBox <gi...@apache.org>.
AlexanderAshitkin commented on a change in pull request #604:
URL: https://github.com/apache/maven/pull/604#discussion_r744833887
##########
File path: maven-core/src/main/java/org/apache/maven/lifecycle/internal/DefaultLifecycleExecutionPlanCalculator.java
##########
@@ -554,6 +554,8 @@ private void injectLifecycleOverlay( Map<String, List<MojoExecution>> lifecycleM
MojoExecution forkedExecution = new MojoExecution( forkedMojoDescriptor, forkedGoal );
+ forkedExecution.setLifecyclePhase( mojoExecution.getLifecyclePhase() );
Review comment:
Hi
Thanks for the change! Jaxb is gone for the good. In overall pr looks good for me.
Few notes:
1. as i understand modello schema is located in core. if we move the cache to an extension in future - doest it mean there will be dependency on core from the extension? Looks like modello scheme actuall couples the cache and the core.
2 There should be few small jaxb related leftovers - dependency in pom itself and in licenses, but in general it looks good.
--
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 pull request #604: Make sure forked mojo executions have a correct phase set
Posted by GitBox <gi...@apache.org>.
gnodet commented on pull request #604:
URL: https://github.com/apache/maven/pull/604#issuecomment-959068120
@hboutemy should I commit them directly to the MNG-7129 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.
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 closed pull request #604: Finish switch to modello
Posted by GitBox <gi...@apache.org>.
gnodet closed pull request #604:
URL: https://github.com/apache/maven/pull/604
--
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] AlexanderAshitkin commented on a change in pull request #604: Finish switch to modello
Posted by GitBox <gi...@apache.org>.
AlexanderAshitkin commented on a change in pull request #604:
URL: https://github.com/apache/maven/pull/604#discussion_r744833887
##########
File path: maven-core/src/main/java/org/apache/maven/lifecycle/internal/DefaultLifecycleExecutionPlanCalculator.java
##########
@@ -554,6 +554,8 @@ private void injectLifecycleOverlay( Map<String, List<MojoExecution>> lifecycleM
MojoExecution forkedExecution = new MojoExecution( forkedMojoDescriptor, forkedGoal );
+ forkedExecution.setLifecyclePhase( mojoExecution.getLifecyclePhase() );
Review comment:
Hi
Thanks for the change! Jaxb is gone for the good. In overall pr looks good for me.
Few notes:
1. as i understand modello schema is located in core. if we move the cache to an extension in future - doest it mean there will be dependency on core from the extension? Looks like modello scheme actuall couples the cache and the core.
2. There should be few small jaxb related leftovers - dependency in pom itself and in licenses, but in general it looks good.
--
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 pull request #604: Make sure forked mojo executions have a correct phase set
Posted by GitBox <gi...@apache.org>.
gnodet commented on pull request #604:
URL: https://github.com/apache/maven/pull/604#issuecomment-959066781
The second commit removes this exception when the cache is not enabled:
```
[ERROR] Cannot save incremental build aggregated report
java.lang.IllegalStateException: Cache is not initialized. Actual state: DISABLED
at com.google.common.base.Preconditions.checkState(Preconditions.java:510)
at org.apache.maven.caching.xml.CacheConfigImpl.checkInitializedState(CacheConfigImpl.java:577)
at org.apache.maven.caching.xml.CacheConfigImpl.isRemoteCacheEnabled(CacheConfigImpl.java:480)
at org.apache.maven.caching.LocalRepositoryImpl.saveCacheReport(LocalRepositoryImpl.java:411)
at org.apache.maven.caching.CacheControllerImpl.saveCacheReport(CacheControllerImpl.java:779)
at org.apache.maven.caching.CacheEventSpy.onEvent(CacheEventSpy.java:45)
at org.apache.maven.eventspy.internal.EventSpyDispatcher.onEvent(EventSpyDispatcher.java:104)
at org.apache.maven.eventspy.internal.EventSpyExecutionListener.sessionEnded(EventSpyExecutionListener.java:61)
at org.mvndaemon.mvnd.logging.smart.LoggingExecutionListener.sessionEnded(LoggingExecutionListener.java:80)
at org.apache.maven.lifecycle.internal.DefaultExecutionEventCatapult.fire(DefaultExecutionEventCatapult.java:64)
at org.apache.maven.lifecycle.internal.DefaultExecutionEventCatapult.fire(DefaultExecutionEventCatapult.java:42)
at org.apache.maven.lifecycle.internal.LifecycleStarter.execute(LifecycleStarter.java:137)
at org.apache.maven.DefaultMaven.doExecute(DefaultMaven.java:305)
at org.apache.maven.DefaultMaven.doExecute(DefaultMaven.java:192)
at org.apache.maven.DefaultMaven.execute(DefaultMaven.java:105)
at org.apache.maven.cli.DaemonMavenCli.execute(DaemonMavenCli.java:825)
at org.apache.maven.cli.DaemonMavenCli.doMain(DaemonMavenCli.java:244)
at org.apache.maven.cli.DaemonMavenCli.main(DaemonMavenCli.java:222)
at org.mvndaemon.mvnd.daemon.Server.handle(Server.java:573)
at org.mvndaemon.mvnd.daemon.Server.client(Server.java:262)
at org.mvndaemon.mvnd.daemon.Server.accept(Server.java:230)
at java.base/java.lang.Thread.run(Thread.java:833)
```
--
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 #604: Finish switch to modello
Posted by GitBox <gi...@apache.org>.
gnodet commented on a change in pull request #604:
URL: https://github.com/apache/maven/pull/604#discussion_r744137847
##########
File path: maven-core/src/main/java/org/apache/maven/lifecycle/internal/DefaultLifecycleExecutionPlanCalculator.java
##########
@@ -554,6 +554,8 @@ private void injectLifecycleOverlay( Map<String, List<MojoExecution>> lifecycleM
MojoExecution forkedExecution = new MojoExecution( forkedMojoDescriptor, forkedGoal );
+ forkedExecution.setLifecyclePhase( mojoExecution.getLifecyclePhase() );
Review comment:
It does not relate to JAXB, however, this fixes a NPE which happens in the cache system when using forked lifecycles. It may be fixed in a different way, but it did look like the easier fix.
My reasoning is the following : I suppose no-one ever looked for the phase of forked plugin executions and it makes sense to me to have those phases set to the phase of the original mojo execution.
The cache system does look and uses the phase in several locations, and this caused NullPointerException.
--
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] hboutemy edited a comment on pull request #604: Finish switch to modello
Posted by GitBox <gi...@apache.org>.
hboutemy edited a comment on pull request #604:
URL: https://github.com/apache/maven/pull/604#issuecomment-962465272
> @hboutemy should I commit them directly to the MNG-7129 branch ?
@gnodet my idea is that you should commit modello migration directly to MNG-7129_Modello when it is about the switch from JAXB to Modello, because I expect that we'll to the swtich in 1 PR that we'll apply to MNG-7129
for other unrelated changes, we need other branches/PRs: instead of creating branches in personal forks, given you have commit rights, I prefer in general branches directly in original Git repository because I think it makes working together easier.
--
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] hboutemy commented on a change in pull request #604: Finish switch to modello
Posted by GitBox <gi...@apache.org>.
hboutemy commented on a change in pull request #604:
URL: https://github.com/apache/maven/pull/604#discussion_r744146337
##########
File path: maven-core/pom.xml
##########
@@ -235,11 +235,16 @@ under the License.
<goals>
<goal>java</goal>
<goal>xpp3-reader</goal>
+ <goal>xpp3-writer</goal>
Review comment:
ok, I found my answer: not really for domain or config, but we need it for diff and report
--
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] hboutemy commented on pull request #604: Finish switch to modello
Posted by GitBox <gi...@apache.org>.
hboutemy commented on pull request #604:
URL: https://github.com/apache/maven/pull/604#issuecomment-962481855
thinking a little bit more, I just found that merging the PR to MNG-7129_Modello branch is what I just need to do
and only inject the 2 first commits to MNG-7129
I'll do that and we can continue the work together
--
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] AlexanderAshitkin commented on a change in pull request #604: Finish switch to modello
Posted by GitBox <gi...@apache.org>.
AlexanderAshitkin commented on a change in pull request #604:
URL: https://github.com/apache/maven/pull/604#discussion_r744833887
##########
File path: maven-core/src/main/java/org/apache/maven/lifecycle/internal/DefaultLifecycleExecutionPlanCalculator.java
##########
@@ -554,6 +554,8 @@ private void injectLifecycleOverlay( Map<String, List<MojoExecution>> lifecycleM
MojoExecution forkedExecution = new MojoExecution( forkedMojoDescriptor, forkedGoal );
+ forkedExecution.setLifecyclePhase( mojoExecution.getLifecyclePhase() );
Review comment:
Hi
Thanks for the change! Jaxb is gone for the good. In overall pr looks good for me.
Few notes:
1. as i understand modello schema is located in core. if we move the cache to an extension in future - doest it mean there will be dependency on core from the extension? It might be not convenient to change core if extension model need to be changed. Looks like modello scheme actuall couples the cache and the core.
2. There should be few small jaxb related leftovers - dependency in pom itself and in licenses, but in general it looks good.
--
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