Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unnecessary validate phase executed by :start maven plugin goal #9130

Closed
pzygielo opened this issue Jan 6, 2023 · 11 comments · Fixed by #9128
Closed

Unnecessary validate phase executed by :start maven plugin goal #9130

pzygielo opened this issue Jan 6, 2023 · 11 comments · Fixed by #9128

Comments

@pzygielo
Copy link
Contributor

pzygielo commented Jan 6, 2023

Here's my setup - https://github.com/pzrep/jetty-start-goal/:
https://github.com/eclipse/jetty.project/blob/b8db190114d047a4ac60d01c6fdda4fdbba9609e/pom.xml#L82-L88

This however results in validate being executed:

[INFO] >>> jetty-maven-plugin:11.0.3:start (start-jetty) > validate @ jetty-start-goal >>>
[INFO] 
[INFO] --- maven-help-plugin:3.3.0:active-profiles (default) @ jetty-start-goal ---
[INFO] 
Active Profiles for Project 'pzrep:jetty-start-goal:war:1.6.0-SNAPSHOT':

The following profiles are active:

 - with-jetty (source: pzrep:jetty-start-goal:1.6.0-SNAPSHOT)



[INFO] 
[INFO] <<< jetty-maven-plugin:11.0.3:start (start-jetty) < validate @ jetty-start-goal <<<
[INFO] 
[INFO] 
[INFO] --- jetty-maven-plugin:11.0.3:start (start-jetty) @ jetty-start-goal ---

But it was executed in line 13 already.

Originally posted by @pzygielo in #9128 (comment)

@pzygielo
Copy link
Contributor Author

pzygielo commented Jan 6, 2023

JETTY-1405 introduced :start, which reduced forked LC to validate only, but I'd like to have no forked LC at all.

@joakime
Copy link
Contributor

joakime commented Jan 7, 2023

@olamy and @jmcc0nn3ll both of you should take a look at this as well (so we can get all 3 maven committers looking at this)

@pzygielo
Copy link
Contributor Author

pzygielo commented Jan 7, 2023

@janbartel
Copy link
Contributor

@pzygielo I think we needed to have at least validate in order to ensure that some of the project artifacts etc were resolved ... but I might be misremembering. Taking another look.

@pzygielo
Copy link
Contributor Author

I think we needed to have at least validate

From the goal's description:

This goal is similar to the jetty:run goal, EXCEPT that it is designed to be bound to an execution inside your pom, rather than being run from the command line.

When using it, be careful to ensure that you bind it to a phase in which all necessary generated files and classes for the webapp
will have been created. If you run it from the command line, then also ensure that all necessary generated files and classes for
the webapp already exist.

So - let me repeat: validate already happens and there is no need for the plugin to execute it again. The fact that in some cases there is no visible output that it is executed for the second (and even the first) time does not mean that there is nothing happening.

@joakime
Copy link
Contributor

joakime commented Jan 10, 2023

The phase specified in the plugin is a default that is present in the plugin metadata, it can be changed in the plugin configuration of your project.
It doesn't force validate phase by ignoring your configuration.
This behavior is handled by, and controlled by, Maven itself, not the plugin, or it's metadata.

The double plugin execution you are seeing is likely from something else, causing unintended side effects.

What we need to diagnose ...

  • an example project
  • details about which jvm version you are using when it happens
  • details about which maven version you are using when it happens
  • details about how you are using maven (examples of some concepts, but not limited to ... command line, from IDE, via maven daemon, using wrappers, from CI, etc)
  • details about your maven command line (what goals and in what order are you using it)

You've referenced https://github.com/pzrep/jetty-start-goal/

But that doesn't show a double execution when run by command line from system installed maven, or the packaged mvnw in the project.
Turning on maven debug --debug shows that jetty only starts once as well.

@pzygielo
Copy link
Contributor Author

pzygielo commented Jan 10, 2023

The phase specified in the plugin is a default that is present in the plugin metadata,

That's the point. It's not the default phase that goal is bound to and can be changed with configuration. This would be in the form of @Mojo(defaultPhase=...). Currently it is @Mojo(name = "start", requiresDependencyResolution = ResolutionScope.TEST) - and I can see there is no defaultPhase there. This could be changed by configuration - but there is nothing to be changed.

This is about @Execute(phase = LifecyclePhase.VALIDATE) as this is what I proposed to remove in original PR.

@Execute != @Mojo.defaultPhase.

I can only suggest https://maven.apache.org/plugin-tools/maven-plugin-plugin/examples/using-annotations.html#annotations

it can be changed in the plugin configuration of your project.

This cannot be changed by plugin configuration.

It doesn't force validate phase by ignoring your configuration

No. It forces validate for other reasons - from the using annotations linked above:

@Execute: Used if your Mojo needs to fork a lifecycle,

And :start does that.

This behavior is handled by, and controlled by, Maven itself, not the plugin, or it's metadata.

It's explicitly requested by the plugin's metadata.

The double plugin execution you are seeing is likely from something else, causing unintended side effects.

No. It's the forked validate life cycle phase forked as requested by :start of jetty plugin.

What we need to diagnose ...

* an example project

I provided the simplest one already - https://github.com/pzrep/jetty-start-goal.

* details about which jvm version you are using when it happens

Not important. But https://github.com/pzrep/jetty-start-goal/blob/45b102341944c508e3307d8be8653824e904918d/.github/workflows/maven.yml#L14 specifies it as temurin 11.

* details about which maven version you are using when it happens

Not important. But https://github.com/pzrep/jetty-start-goal/blob/45b102341944c508e3307d8be8653824e904918d/.mvn/wrapper/maven-wrapper.properties#L18 specifies it as 3.8.7

* details about how you are using maven (examples of some concepts, but not limited to ... command line, from IDE, via maven daemon, using wrappers, from CI, etc)
* details about your maven command line (what goals and in what order are you using it)

https://github.com/pzrep/jetty-start-goal/blob/45b102341944c508e3307d8be8653824e904918d/.github/workflows/maven.yml#L16:

./mvnw --no-transfer-progress verify

You've referenced https://github.com/pzrep/jetty-start-goal/

But that doesn't show a double execution when run by command line from system installed maven, or the packaged mvnw in the project.

Yes, it does. I linked to https://github.com/pzrep/jetty-start-goal/actions/runs/3858708613/jobs/6577522172#step:4:52 already and said that validate was achieved for the first time in line 13.

The fact that there is no output for validate phase does not mean it doesn't happen. That's why I do explicitly bind active-profiles to validate phase in https://github.com/pzrep/jetty-start-goal/blob/45b102341944c508e3307d8be8653824e904918d/pom.xml#L23 to show OTHER plugin execution.

There is no need to call maven with --debug to observe what I fail to describe here.

@janbartel
Copy link
Contributor

@olamy it doesn't seem to do any harm if we remove the @Execute(phase = LifecyclePhase.VALIDATE), all of our integration tests complete in the green. Any objections to simply removing that line?

@janbartel
Copy link
Contributor

@pzygielo can you submit a PR that removes the validate line? Or do you want me to do it?

@olamy
Copy link
Member

olamy commented Jan 17, 2023

@olamy it doesn't seem to do any harm if we remove the @Execute(phase = LifecyclePhase.VALIDATE), all of our integration tests complete in the green. Any objections to simply removing that line?

LGTM

@pzygielo
Copy link
Contributor Author

@pzygielo can you submit a PR that removes the validate line? Or do you want me to do it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants