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

Do not execute any phase for maven plugin :start #9128

Merged
merged 1 commit into from
Jan 17, 2023

Conversation

pzygielo
Copy link
Contributor

@pzygielo pzygielo commented Jan 6, 2023

@sbordet
Copy link
Contributor

sbordet commented Jan 6, 2023

Thanks for this PR, but can you please describe what is the problem that this PR would be fixing?

@pzygielo
Copy link
Contributor Author

pzygielo commented Jan 6, 2023

Given

https://github.com/eclipse/jetty.project/blob/8b7723bf98680a7250029fa2cc3a95fad7b401fc/jetty-maven-plugin/src/main/java/org/eclipse/jetty/maven/plugin/JettyStartMojo.java#L25

thus it's almost sure validate is already included in buildplan. And the project is validated by the time :start is reached.

This is inconvenient if other goals are also bound to validate as this triggers them for the second time on :start.

@joakime
Copy link
Contributor

joakime commented Jan 6, 2023

This is just the recommended phase following the maven "convention over configuration" approach.

You can override this with your own configuration to specify a different phase.

<plugin>
  <artifactId>jetty-maven-plugin</artifactId>
  <phase>prepare</phase>
</plugin>

or

<plugin>
  <artifactId>jetty-maven-plugin</artifactId>
  <executions>
    <execution>
      <phase>prepare</phase>
    </execution>
  </executions>
</plugin>

There's no need to remove this from the codebase.

@joakime
Copy link
Contributor

joakime commented Jan 6, 2023

Not going to merge this.

@joakime joakime closed this Jan 6, 2023
@pzygielo
Copy link
Contributor Author

pzygielo commented Jan 6, 2023

This is just the recommended phase following the maven "convention over configuration" approach.

You can override this with your own configuration to specify a different phase.

I don't think so.

<plugin>
  <artifactId>jetty-maven-plugin</artifactId>
  <phase>prepare</phase>
</plugin>

I can't.

This is not about goal being bound to phase (there is no default one set for this goal).

This is about extra execution of life cycle.

There's no need to remove this from the codebase.

Please use this goal once and reconsider.

@pzygielo pzygielo deleted the lcp branch January 6, 2023 21:13
@pzygielo
Copy link
Contributor Author

pzygielo commented Jan 6, 2023

Just for completeness, here's my setup:
https://github.com/eclipse/jetty.project/blob/b8db190114d047a4ac60d01c6fdda4fdbba9609e/pom.xml#L82-L88
I obviously have no other choice - the phase has to be set explicitly anyway for goal to run. Remove the line 84 and it will give

[INFO] Jetty not running!

in the end, as the :start won't happen at all.

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. How can I avoid that second run?

@joakime
Copy link
Contributor

joakime commented Jan 6, 2023

Open an issue, and please provide a link to an example project. (preferably a github repo)
What you are linking to isn't showing the whole picture.
Have the discussion first, don't jump to the kind of drastic (and workflow breaking) changes you have in this PR.
A solution can be had, we just need more information.

@pzygielo
Copy link
Contributor Author

pzygielo commented Jan 6, 2023

Open an issue, and please provide a link to an example project. (preferably a github repo) What you are linking to isn't showing the whole picture.

I was linking to action for https://github.com/pzrep/jetty-start-goal, which is complete reproducer. It is github repo. It is showing the whole picture.

Have the discussion first, don't jump
to the kind of drastic
(and workflow breaking)

👀 (Took me 8 months to jump, since my first discovery.)

My change is just following the (currently misleading) description the class has. And my expectation.

A solution can be had, we just need more information.

I'm sorry if I'm the first to complain for 11 years (this issue was introduced in jetty-8.1.0.RC2 - 22 December 2011).

I have no more information.

Thanks for checking.

@pzygielo pzygielo restored the lcp branch January 7, 2023 09:23
@olamy olamy reopened this Jan 17, 2023
@pzygielo pzygielo marked this pull request as ready for review January 17, 2023 09:48
@janbartel janbartel merged commit 5659e6d into jetty:jetty-10.0.x Jan 17, 2023
@pzygielo pzygielo deleted the lcp branch January 18, 2023 09:33
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 this pull request may close these issues.

Unnecessary validate phase executed by :start maven plugin goal
5 participants