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

Ensure all eeX versions of the plugin use prefix "jetty" #9156

Merged
merged 2 commits into from
Jan 25, 2023

Conversation

janbartel
Copy link
Contributor

Closes #9155.

Ensure all jetty maven plugins use the prefix jetty, rather than the default generated jetty-eeX prefix.

@janbartel janbartel self-assigned this Jan 11, 2023
@olamy
Copy link
Member

olamy commented Jan 12, 2023

how do you know which version you are using when simply using mvn jetty:run?
I would prefer have another plugin on the top of the 3 plugins which select which one to use depending on various configuration or pom dependencies such javax.servlet vs jakarta.servlet and still have the option to use mvn jetty-eeX:run.

@janbartel
Copy link
Contributor Author

@olamy but sure you must declare the plugin in the pom of your webapp, so there's no confusion as to which one you are using? It would be highly unlikely that anyone would declare multiple versions of jetty plugin for the same webapp.

@joakime
Copy link
Contributor

joakime commented Jan 12, 2023

If someone has more than 1 of them in their local repo, the use of jetty:run on the command line is meaningless and won't resolve to a specific plugin.

You could setup a specific one via it's groupId in the $HOME/.m2/settings.xml (see: settings > pluginGroups > pluginGroup)

Perhaps we need only 1 jetty-maven-plugin and "modules" (loose term) that selects the environment (and associated deps) based on factors in the project?

@janbartel
Copy link
Contributor Author

If someone has more than 1 of them in their local repo, the use of jetty:run on the command line is meaningless and won't resolve to a specific plugin.

There's nothing new here. If you have multiple previous versions of jetty (7, 8, 9, 10, 11 etc) in your repo the same would be true. Note that if I don't define the plugin in the pom - or don't get the right maven coordinates - jetty-8 runs and it's not even in my repo.
If you want to be sure what runs, you need to - as has always been the case - define it in your pom.

You could setup a specific one via it's groupId in the $HOME/.m2/settings.xml (see: settings > pluginGroups > pluginGroup)

Perhaps we need only 1 jetty-maven-plugin and "modules" (loose term) that selects the environment (and associated deps) based on factors in the project?

Maybe that could be a future enhancement. However, we don't do that for standalone jetty (eg the deployer doesn't work out what modules to run based on your webapp) because it's difficult and error-prone. If we work out a good algorithm for stand-alone jetty we could look at applying it to the plugin in the future.

The solution for running multiple different eeX versions of the jetty plugin is the same as multiple different versions of jetty - simply define different profiles and invoke whichever one you want.

@olamy
Copy link
Member

olamy commented Jan 13, 2023

we need to check as well what is the side effect for users who have registered a pluginGroup in their settings.
https://maven.apache.org/guides/introduction/introduction-to-plugin-prefix-mapping.html

@janbartel
Copy link
Contributor Author

we need to check as well what is the side effect for users who have registered a pluginGroup in their settings.

@olamy, well, if they specified org.eclipse.jetty then they will keep on using whatever they were using before because the new groupIds are org.eclipse.jetty.eeX.

@olamy
Copy link
Member

olamy commented Jan 16, 2023

we need to check as well what is the side effect for users who have registered a pluginGroup in their settings.

@olamy, well, if they specified org.eclipse.jetty then they will keep on using whatever they were using before because the new groupIds are org.eclipse.jetty.eeX.

Not sure how this will work with multi module build (e.g using mvn jetty:run from the top of a multi module build).

Furthermore we have been quite good to fix all the wrong maven naming convention we had with jetty-12.0.x, I don't like much the idea to reintroduce a new one.
Maven naming convention is usually the part of the artifactId before -maven-plugin

@janbartel
Copy link
Contributor Author

@olamy I don't see the problem with multi-module builds, can you explain that?

Furthermore we have been quite good to fix all the wrong maven naming convention we had with jetty-12.0.x, I don't like much the idea to reintroduce a new one.
Maven naming convention is usually the part of the artifactId before -maven-plugin

We comply with maven naming conventions. Our artifacts are named jetty-xxx-maven-plugin as per maven doco. The maven doco explicitly says that you can map your plugin to whatever prefix you want, rather than accept the auto generated default. And finally, I wasted about 1.5 hrs wondering why I couldn't get the ee9 or ee10 plugins to work at the command line, despite having defined them in my pom.xml. I was, of course, typing mvn jetty:run as has been the case since I first wrote the plugin years ago. I don't think I'm going to be alone in that, so I think it's important for the user experience to retain familiar naming, rather than just accept the maven auto generated default prefix.

@joakime
Copy link
Contributor

joakime commented Jan 16, 2023

We comply with maven naming conventions. Our artifacts are named jetty-xxx-maven-plugin as per maven doco. The maven doco explicitly says that you can map your plugin to whatever prefix you want, rather than accept the auto generated default. And finally, I wasted about 1.5 hrs wondering why I couldn't get the ee9 or ee10 plugins to work at the command line, despite having defined them in my pom.xml. I was, of course, typing mvn jetty:run as has been the case since I first wrote the plugin years ago. I don't think I'm going to be alone in that, so I think it's important for the user experience to retain familiar naming, rather than just accept the maven auto generated default prefix.

This is the part that bothers me.

With this proposed change, using jetty:run from the command line is no longer a sustainable option.
You HAVE to specify it in the pom and only use the pom phases now, the name of the <plugin-prefix>:<goal> would now be unreliable.

If this is your concern, then the solution should be ...

  • Leave jetty-eex as the plugin prefix.
  • Add detection code in the old versions (Jetty 9 thru 11) of jetty-maven-plugin for this new reality and warn / suggest improvements when used with a pom that has Jetty 12+ dependencies (or Servlet 6.x deps).

Basically, correct bad behavior, not introduce more bad behavior.

The second bullet point should happen, regardless of what else we do with this issue.

@janbartel
Copy link
Contributor Author

We comply with maven naming conventions. Our artifacts are named jetty-xxx-maven-plugin as per maven doco. The maven doco explicitly says that you can map your plugin to whatever prefix you want, rather than accept the auto generated default. And finally, I wasted about 1.5 hrs wondering why I couldn't get the ee9 or ee10 plugins to work at the command line, despite having defined them in my pom.xml. I was, of course, typing mvn jetty:run as has been the case since I first wrote the plugin years ago. I don't think I'm going to be alone in that, so I think it's important for the user experience to retain familiar naming, rather than just accept the maven auto generated default prefix.

This is the part that bothers me.

With this proposed change, using jetty:run from the command line is no longer a sustainable option. You HAVE to specify it in the pom and only use the pom phases now, the name of the <plugin-prefix>:<goal> would now be unreliable.

Not true. In order to resolve a command line like "mvn jetty:run" you must either: specify the jetty-maven-plugin in your pom or you instead specify a pluginGroup for it in your settings.xml. Otherwise maven has no way of resolving what "jetty" relates to. This is the output you get if you have not specified either the plugin in your pom or a pluginGroup in settings.xml:

[INFO] ------------------------------------------------------------------------
[INFO] BUILD FAILURE
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 2.321 s
[INFO] Finished at: 2023-01-17T10:08:19+11:00
[INFO] ------------------------------------------------------------------------
[ERROR] No plugin found for prefix 'jetty' in the current project and in the plugin groups [org.codehaus.mojo, org.sonatype.maven.plugins, com.googlecode.maven-overview-plugin, org.apache.maven.plugins] available from the repositories [local (/home/janb/.m2/repository), central (https://repo.maven.apache.org/maven2)] -> [Help 1]
[ERROR]
[ERROR] To see the full stack trace of the errors, re-run Maven with the -e switch.
[ERROR] Re-run Maven using the -X switch to enable full debug logging.
[ERROR]
[ERROR] For more information about the errors and possible solutions, please read the following articles:
[ERROR] [Help 1] http://cwiki.apache.org/confluence/display/MAVEN/NoPluginFoundForPrefixException

By not specifying the version of the plugin in your pom, you are also asking for a non-repeatable build, as maven will pick whatever is the highest version of the plugin matching the pluginGroup. This is what you get if you specify a pluginGroup of org.eclipse.jetty (ie valid for every version prior to jetty-12):
[16] mvn jetty:run
[INFO] Scanning for projects...
Downloading from central: https://repo.maven.apache.org/maven2/org/eclipse/jetty/maven-metadata.xml
Downloaded from central: https://repo.maven.apache.org/maven2/org/eclipse/jetty/maven-metadata.xml (408 B at 282 B/s)
Downloading from central: https://repo.maven.apache.org/maven2/org/eclipse/jetty/jetty-maven-plugin/maven-metadata.xml
Downloaded from central: https://repo.maven.apache.org/maven2/org/eclipse/jetty/jetty-maven-plugin/maven-metadata.xml (8.9 kB at 18 kB/s)
Downloading from central: https://repo.maven.apache.org/maven2/org/eclipse/jetty/jetty-maven-plugin/11.0.13/jetty-maven-plugin-11.0.13.pom
Downloaded from central: https://repo.maven.apache.org/maven2/org/eclipse/jetty/jetty-maven-plugin/11.0.13/jetty-maven-plugin-11.0.13.pom (13 kB at 25 kB/s)
Downloading from central: https://repo.maven.apache.org/maven2/org/eclipse/jetty/jetty-project/11.0.13/jetty-project-11.0.13.pom
Downloaded from central: https://repo.maven.apache.org/maven2/org/eclipse/jetty/jetty-project/11.0.13/jetty-project-11.0.13.pom (103 kB at 158 kB/s)
Downloading from central: https://repo.maven.apache.org/maven2/org/eclipse/jetty/jetty-maven-plugin/11.0.13/jetty-maven-plugin-11.0.13.jar
Downloaded from central: https://repo.maven.apache.org/maven2/org/eclipse/jetty/jetty-maven-plugin/11.0.13/jetty-maven-plugin-11.0.13.jar (141 kB at 238 kB/s)

So it has never been the case that you can just type "mvn jetty:run" at the command line and expect it to work without configuring something. There is nothing different about the change I'm proposing for jetty-12.

Those users who have fully specified a pre-jetty-12 version of plugin in their pom are using "jetty:run", and will not be using the jetty-12 version unless they change that specification.
Those users who have merely specified a pre-jetty-12 pluginGroup of org.eclipse.jetty in settings.xml are using "jetty:run" and will not be using the jetty-12 version unless they change the pluginGroup definition.

For either of these sets of users, with the change I'm proposing, after updating either their pom.xml or their settings.xml, they continue to type "mvn jetty:run" at the command line just as they always have. In fact for those users who are using pluginGroup in the settings.xml, they may not even be aware that the artifactId has changed in jetty-12.

I think if we change the well-known goal name then we will have to well document that but also be in for a lot of posts to the jetty email groups that "mvn jetty:run" doesn't work any more. I just don't see any end-user benefit in changing to "mvn jetty-eeX:run". If there is some significant benefit, can you explain what it is?

@olamy
Copy link
Member

olamy commented Jan 17, 2023

@olamy I don't see the problem with multi-module builds, can you explain that?

a few organisations use the concept of huge mono repo and so with maven modules still in ee8 living with modules already converted to ee9/ee10.
When working with multi modules, users usually run something such mvn jetty:run -pl :my-webapp -am to include in the jetty webapp dependant projects from the reactor when they are working on a project.
In this case you need to set the plugin version in the root pom.
I case of modules with different need, the root pom will contains in pluginMngt declaration for jetty-ee8-m-p and jetty-ee9-m-p etc...
In such case when running mvn jetty:run .... I'm not sure how Maven will work with that.
I just feel in the spirit of all we did with jetty-12 was to establish some real configuration done by user to setup their env with reference to the eeX they want to use.
All groupId/artifactId contains reference to the correct eeX, with using the distribution module name contains the reference eeX, I really feel this as a pattern or a convention we should keep having (even for the Jetty Maven plugin):

  • jetty-ee9:run
  • jetty:run -Denv=ee9 (with a new plugin on the top of every others)

Maybe I'm wrong but I really feel this more in the spirit/convention/etc... we did with jetty 12.

Furthermore we have been quite good to fix all the wrong maven naming convention we had with jetty-12.0.x, I don't like much the idea to reintroduce a new one.
Maven naming convention is usually the part of the artifactId before -maven-plugin

@janbartel
Copy link
Contributor Author

@olamy I'm still not seeing the problem. I'm attaching a multimodule project that has 2 webapps and one tld that both webapps depend on. webapp uses jetty-ee9-maven-plugin and webapp2 uses jetty-ee10-maven-plugin.At the top level project I can run either mvn jetty:run -pl webapp -am or mvn jetty:run -pl webapp2 -am and they each use the correct version of the jetty plugin. Can you modify to illustrate the problem you're worried about? To do that you'll need to build my branch and remove any maven-metadata.xml files from your ~.m2/repository/org/eclipse/jetty/eeX directories.

multimodule-with-tld.zip

@olamy
Copy link
Member

olamy commented Jan 17, 2023

@olamy I'm still not seeing the problem. I'm attaching a multimodule project that has 2 webapps and one tld that both webapps depend on. webapp uses jetty-ee9-maven-plugin and webapp2 uses jetty-ee10-maven-plugin.At the top level project I can run either mvn jetty:run -pl webapp -am or mvn jetty:run -pl webapp2 -am and they each use the correct version of the jetty plugin. Can you modify to illustrate the problem you're worried about? To do that you'll need to build my branch and remove any maven-metadata.xml files from your ~.m2/repository/org/eclipse/jetty/eeX directories.

multimodule-with-tld.zip

There is the problem I was expecting.
Look what happen when running mvn jetty:run -am -pl :webapp2

In the tld module the wrong plugin is used.

[INFO] <<< jetty-ee9-maven-plugin:12.0.0-SNAPSHOT:run (default-cli) < test-compile @ tld <<<
[INFO] 
[INFO] 
[INFO] --- jetty-ee9-maven-plugin:12.0.0-SNAPSHOT:run (default-cli) @ tld ---
[INFO] Packaging type [jar] is unsupported

BTW the run goal is not a goal bind to a phase (e.g not part of any lifecycle so not run per default) for this reason it should be declared only in build/pluginManagement. You get this working because you use the trick of declaring it in build/plugins which is not usual for such plugin not part of the lifecycle.
Just use build/pluginManagement section and you will notice the issue.

@janbartel
Copy link
Contributor Author

@olamy I'm still not seeing the problem. I'm attaching a multimodule project that has 2 webapps and one tld that both webapps depend on. webapp uses jetty-ee9-maven-plugin and webapp2 uses jetty-ee10-maven-plugin.At the top level project I can run either mvn jetty:run -pl webapp -am or mvn jetty:run -pl webapp2 -am and they each use the correct version of the jetty plugin. Can you modify to illustrate the problem you're worried about? To do that you'll need to build my branch and remove any maven-metadata.xml files from your ~.m2/repository/org/eclipse/jetty/eeX directories.
multimodule-with-tld.zip

There is the problem I was expecting. Look what happen when running mvn jetty:run -am -pl :webapp2

In the tld module the wrong plugin is used.

But the jetty plugin is not used by the tld module.

[INFO] <<< jetty-ee9-maven-plugin:12.0.0-SNAPSHOT:run (default-cli) < test-compile @ tld <<<
[INFO] 
[INFO] 
[INFO] --- jetty-ee9-maven-plugin:12.0.0-SNAPSHOT:run (default-cli) @ tld ---
[INFO] Packaging type [jar] is unsupported

BTW the run goal is not a goal bind to a phase (e.g not part of any lifecycle so not run per default) for this reason it should be declared only in build/pluginManagement. You get this working because you use the trick of declaring it in build/plugins which is not usual for such plugin not part of the lifecycle. Just use build/pluginManagement section and you will notice the issue.

It's not a trick, I'm just following the maven documentation :) See https://maven.apache.org/guides/plugin/guide-java-plugin-development.html. Nothing about having to declare your plugins differently whether they are or aren't bound to a phase. Now, with your extra insight into maven, maybe they should be, which means the maven documentation is wrong, which means millions of other users like me are "doin' it 'rong" and have been for years.

Can you clarify how you think having multiple different jetty-12 jetty plugin artifacts is any different to having multiple different pre-jetty-12 jetty plugin artifacts in a multimodule build? It seems an identical situation to me.

@gregw
Copy link
Contributor

gregw commented Jan 17, 2023

@janbartel @olamy @joakime Throwing in my 2c from a 20,000 ft mixed metaphor view... without having read the detail....

We've not renamed start.jar to ee10-start.jar, so I don't think we should rename mvn jetty:run. Currently we don't have the magic for jetty to work out which eeX environment that needs to be used for the webapp... but maybe in future we will work out that magic, so keeping "eeX" out of the command is what we should do.

If we REALLY need to put the "eeX" name in there, then it should be mvn jetty:ee10-run... but I don't really like that.... and it also precludes future magic to work out which eeX to run.

@olamy
Copy link
Member

olamy commented Jan 17, 2023

But the jetty plugin is not used by the tld module.

But it is executed and with the wrong version!

anyway if you want to merge this just do it. I'm just 0 as I feel it's wrong because of the naming.
Because I don't like the idea about having multiple plugins with different groupId/artifactId/environment using the same goal prefix (but maybe I'm wrong)

@joakime
Copy link
Contributor

joakime commented Jan 17, 2023

We've not renamed start.jar to ee10-start.jar, so I don't think we should rename mvn jetty:run. Currently we don't have the magic for jetty to work out which eeX environment that needs to be used for the webapp... but maybe in future we will work out that magic, so keeping "eeX" out of the command is what we should do.

Irrelevant, start.jar is not built against a single environment.
Nor do we have multiple start.jar versions for different environments.

If you want to go down this path, then this further enforces the idea that we should not have multiple jetty-maven-plugin implementations, but a single environment neutral one.

@joakime
Copy link
Contributor

joakime commented Jan 17, 2023

Because I don't like the idea about having multiple plugins with different groupId/artifactId/environment using the same goal prefix (but maybe I'm wrong)

I totally agree, the typical maven plugin lookup is based on groupId:artifactId, with a well established behavior with version too.
But now we are adding a 4th level environment, to find the correct plugin instance.
Before this change that was part of the artifactId and VERY clear.
With this proposed change we are stomping on this and cannot support a multimodule build with different environments.
I'm -1 on this.

start.jar is environment neutral, and even supports environments that we haven't even thought of yet.
jetty-maven-plugin should be the same, a single plugin, and environment agnostic. (it should even allow a user to create a new environment at will)

@gregw
Copy link
Contributor

gregw commented Jan 17, 2023

The start.jar example is not irrelevant. It is a single entry point that uses configuration and conventions to determine which jetty components to run and which eeX environments are run within them.
The mvn jetty:run command is also a single entry point that uses configurations and conventions to determine which jetty to run and which eeX environments are run withing them.

We've always been able to just do mvn jetty:run and use configuration for it to pick between jetty7, ...., jetty-10, jetty-11. The configuration is now also picking between ee8, ee9, ee10. It would be wonderful if we can have a single plugin that is environment agnostic, but currently I understand that is somewhere between a lot of work and impossible. But in reality users don't care. They just want to use the command that they always have used and that will be hard wired into their fingers, brains, scripts & documentation.

If we introduce env prefixes now, and then in 12.0.1 or 12.1.0 we work out how to have a single plugin with multiple env support, then we have thrown away decades of legacy to momentarily support a new command structure that exposes a temporary internal implementation.

@gregw
Copy link
Contributor

gregw commented Jan 17, 2023

With this proposed change we are stomping on this and cannot support a multimodule build with different environments.

This is not true. You can configure different plugins in different modules or put different plugins in different profiles. It is no different to how we have always been able to test against different versions of jetty.

@janbartel janbartel merged commit e271629 into jetty-12.0.x Jan 25, 2023
@janbartel janbartel deleted the jetty-12.0.x-9155-jetty-plugin-prefix branch January 25, 2023 06:36
@olamy olamy added the bad-idea label Jan 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants