[MPLUGIN-450] Require goalPrefix to be valid

[gnodet]

JIRA issue: https://issues.apache.org/jira/browse/MPLUGIN-450

[michael-o]

This change then deserves a new minor version

[michael-o]

@gnodet Please go ahead: squash, rebase and merge. I will take care of the next minor release.

[alexsuter]

Updating to the last version breaks my plugin builds and I don't know what needs to be specified for goalPrefix. I also can not find good documentation for it.

[ottlinger]

Same with me - apache/creadur-rat#190
As a consumer of the api I need to specify my own goal-prefix, right?

[cstamas]

https://maven.apache.org/plugin-tools/maven-plugin-plugin/helpmojo-mojo.html#goalPrefix

In short, just add this to maven-plugin-plugin configuration:

  <goalPrefix>foo</goalPrefix>

[alexsuter]

I guess @ottlinger will use now:

 <goalPrefix>rat</goalPrefix>

[ottlinger]

I guess @ottlinger will use now:

 <goalPrefix>rat</goalPrefix>

mmmmh - I thought apache-rat or do I misinterpret what the goal-prefix is? I thought it is a fully qualified name?

[cstamas]

According to doco it is apache-rat, yes: https://creadur.apache.org/rat/apache-rat-plugin/index.html

[laeubi]

@gnodet the JIRA mentions

but we may want to have some option to turn off this feature

but it seems there is no such option as far as I can see?

Can the "heuristics" maybe be enhanced to also accept <prefix>-plugin and using then as a prefix?
This seems to be the previous behavior here in fact.

e.g in Tycho there are several plugins named e.g. tycho-<somename>-plugin and having them all renamed /relocated to tycho-<somename>-maven-plugin seems not really usefull, also configuring a prefix for each of them is possible we never needed any nor used any prefix as the most natural choice would be tycho for all plugins but that seems not possible....

I also see some risk that different plugins now they are forced to, choose some prefix that clashes in the name.

[michael-o]

@gnodet the JIRA mentions

but we may want to have some option to turn off this feature

but it seems there is no such option as far as I can see?

Can the "heuristics" maybe be enhanced to also accept <prefix>-plugin and using then as a prefix? This seems to be the previous behavior here in fact.

e.g in Tycho there are several plugins named e.g. tycho-<somename>-plugin and having them all renamed /relocated to tycho-<somename>-maven-plugin seems not really usefull, also configuring a prefix for each of them is possible we never needed any nor used any prefix as the most natural choice would be tycho for all plugins but that seems not possible....

I also see some risk that different plugins now they are forced to, choose some prefix that clashes in the name.

You should file a JIRA issue for any request which we can explore together...

[rmannibucau]

I also see some risk that different plugins now they are forced to, choose some prefix that clashes in the name.

AFAIK it always had been the case, the heuristic always had been considered bad but put there by compatibility when it got realized.
The fact plugin prefix is an identifier in a build - you can extend it to globally on a pure theorical standpoint - had the issue you mention but only if you call it from the CLI using the prefix - else it is ignored and not used - and if we want to drop that constraint that it means we'll not enable to call prefix:goal but will enforce ga[v]:goal which is a joke from a CLI standpoint so guess we'll probably stay in the current status and maybe just log the heuristic and show the plugin configuration to set to stay in the same status, was not an issue before, it will not be tomorrow ;).

[laeubi]

I have now created:

• https://issues.apache.org/jira/browse/MPLUGIN-504

I don't see any issue that can arise from the default (beside that the prefix is maybe longer than one wants) but why enforce it to specify what the plugin was able to decide for years without a problem?

Also not that there is a lot of plugins that will never really be called from cli .... so it is not true that before there was no prefix, it has always chosen one that seem to fit most purposes.

[rmannibucau]

I don't see any issue that can arise from the default (beside that the prefix is maybe longer than one wants) but why enforce it to specify what the plugin was able to decide for years without a problem?

We got issues with plugins wrongly naming the artifactId (not respecting foo-maven-plugin) or misusing plugin in the name leading to a surprising name.

Also not that there is a lot of plugins that will never really be called from cli

All plugins can be called at some point from the cli.
An example is git-commit-id plugin, it is never supposed to be called from the cli...but it is, so as a maven dev you can't assume that and the actual assumption would be "most plugin will be called at some point from the cli".
Keep in mind you can reproduce the full lifecycle from the CLI. Useless? not from a CI perspective (enables to add phases depending the env or ex).

[laeubi]

We got issues with plugins wrongly naming the artifactId (not respecting foo-maven-plugin) or misusing plugin in the name leading to a surprising name.

All this can happen with a configured prefix as well... And as said if one is "surprised" can still configure it already but not it fails for all "non surprising" cases as well.

[rmannibucau]

@laeubi well I'm not sure cause no it cant happen. You can get conflicts but no surprise nor wrong behavior if explicit compared to an almost random heuristic.
Alternative is to force the naming convention - artifactid=x-maven-plugin whichbis worse is backward compat so guess we are not that bad and fact you dont know what you setup foe tycho shows it is a good thing to force the config, just an error message enhancement to help users to recover from the upgrade.

[laeubi]

I'm not sure cause no it cant happen. You can get conflicts but no surprise nor wrong behavior if explicit compared to an almost random heuristic.

Until now no one could give an example of "random" behavior, so can you show where this random part happen? For me it was (and is) completely deterministic as for each given input it always gives the same output.

Especially it seems that two projects (tycho+apache-rat) that are using the convention <projectname>-plugin getting the same resulting prefix (== <projectname> ) so it can't be that random....

It was simply assumed (what obviously is wrong) that:

1. goal prefix is optional, but "good to have" --> actually goal prefix was always present, so only explicitly specify it was optional)
2. Cases when is not present, is when heuristics fails (so plugin module is not named as "xxx-maven-plugin" or "maven-xxx-plugin") --> also "xxx-plugin" works fine before, as well as even xxx as artifact id
3. The cases when prefix is not present is mostly unintentional --> the new behavior does not fail/warn/... in cases there is no prefix, as obviously tycho+rat have a prefix successfully derived but now fail

So obviously the assumption that anything that do not matches *-maven-plugin *-maven-plugin got no prefix is wrong, you never get an empty prefix as claimed in the ticket.

[rmannibucau]

Especially it seems that two projects (tycho+apache-rat) that are using the convention -plugin getting the same resulting prefix (== ) so it can't be that random....

s/random/uncontrolled or unknown/

and this is the issue, when then the plugin wants to take control cause you don't want to type my-plugin-foo or anything else then you break backward compat of the runtime so better to adjust the tooling (build only, note there is no breaking change for the runtime).
The fix is always trivial: grab your last release read your plugin.xml and just configure it to be aligned.

So obviously the assumption that anything that do not matches *-maven-plugin *-maven-plugin got no prefix is wrong, you never get an empty prefix as claimed in the ticket.

No, you can review the previous code, if the artifactId does not follow maven convention then the heuristic is just failing and returns null leading to no prefix in plugin.xml.
A common pattern I saw was to name the maven plugin "maven-plugin", then, due to the Pattern.compile("-?(maven|plugin)-?").replaceAll("") usage in the heuristic it was leading to no prefix.

Indeed we can relax a bit the fix Guillaume did to keep the heuristic and warn the user it should be set explicitly to ensure it is under control but I guess the best is just current code which is the most straight forward for anyone, once the error message is explicit the fix is trivial for anyone and there wouldn't be much discussion I think.

That said I would be ok if we do this change only for maven 4 and keep maven 3 as this but I think it goes in the right direction enhancing the quality of the built plugins and the warning would just be as bothering as an error so let's keep it an error for the final deliverable.

Side note: if you don't know the prefix you were using you can also rely on the doc, as of today, since it is explicit there, you can review https://tycho.eclipseprojects.io/doc/latest/tycho-compiler-plugin/plugin-info.html for example.

[laeubi]

The fix is always trivial: grab your last release read your plugin.xml and just configure it to be aligned.

This recommendation together with this fact

if the artifactId does not follow maven convention then the heuristic is just failing and returns null leading to no prefix in plugin.xml

confuses me... so how can I lookup something that isn't there?

So I think the error is correct if it is truly empty... but not for cases where it has something else, then one might warn that the prefix is probably not as expected (where its a bit unclear what users will expect, maybe nothing much if they never checked the prefix...).

[rmannibucau]

@laeubi ok, so if I rephrase the questions are IMHO:

1. Do we want to let people go a prefix they didn't anticipate since it will fully be part of the exposed UI to the end user (mvn foo:bar) - the fact you said you didn't know what to put whereas it was in your doc kind of corroborate that part?
2. For tempting cases where you flag your plugin "plugin", "maven-plugin" you were getting an empty prefix which was wrong and must fail by design - it really happent (happens :().

So discussion is mainly about 1, I think a warning wouldnt help and an error is the right thing to do - warnings tend to be ignored or avoided as an error so both cases make it useless so it is either considered ok - half of the cases are not as mentionned - so I think error is better for that reason even if it means some people will have to explicit the prefix even if the heuristic was ok-ish for them (note that rat is one example it was not ok, the rat vs apache-rat prefix is still an issue for most users and highlights point 1 too).

Hope it helps to follow my reasoning a bit.

[laeubi]

As mentioned, I never used that prefix, and no one has ever complained it is wrong/to long / ... so it simply wasn't an issue before. I now learned it shows up in the docs, and I probably could have omitted some infos in the rare cases where I called a mojo from CLI.

So for (1) it seems a lot of users where happy with that, for (2) as said it seams reasonable to give an error as it is obviously not getting anything useful here.

note that rat is one example it was not ok, the rat vs apache-rat prefix is still an issue for most users

But why don't these user ever complained so the prefix was added before this change? why is apache-rat "wrong" and rat right? of course shorter is more convenient but again, no one has complained, so it was convenient enough as it seems.

Also the shorter the higher the risk of name-clashes can maven handle this if two plugins use the same prefix or will it make these two plugins incompatible to be used in the same build? Or will it even be "random" (maybe by some ordering in the pom) what is chosen?

[rmannibucau]

So for (1) it seems a lot of users where happy with that

I assume you mean for tycho case but it is really done under the carpet so 50-50 IMHO (implicit case is actually rare from what I saw so not sure it is numerous enough to do stats :D).

But why don't these user ever complained so the prefix was added before this change?

You mix two things there: the producer and consumer sides. There is no change for consumers (users), it only changes the way the producer sets the prefix.

why is apache-rat "wrong" and rat right

Don't think it is what you mean but didn't say apache-rat is wrong, what I meant is that the concurrent prefix is an issue for users cause they don't know which one to use. It is common to see an user using the wrong prefix on rat failure and not getting the expected result.

no one has complained

This is not true, not sure why you say that.

Also the shorter the higher the risk of name-clashes can maven handle this if two plugins use the same prefix or will it make these two plugins incompatible to be used in the same build

Only if referenced from the prefix, not in all other cases.

[laeubi]

You mix two things there: the producer and consumer sides. There is no change for consumers (users), it only changes the way the producer sets the prefix.

I don't mix anything, but that's actually the point, now the producer is forced to set something so it stays as before, because consumers before have never complained obviously that they want some change.

So neither producer nor consumer requested for such feature / change, but now are facing to take actions e.g. producer needs to replicate previously automatic info, consumer might need to change because producer has decided to if setting the prefix it should be changed to something else (like for rat vs apache-rat)....

e.g. for the tycho case I now need to add 24 useless duplication in the config that was previously perfectly derived without a problem and users never where unhappy enough to open an issue > 10 years:

• https://github.com/eclipse-tycho/tycho/pull/3363/files

and code does not seems become "better" from that change.

[rmannibucau]

I don't mix anything, but that's actually the point, now the producer is forced to set something so it stays as before, because consumers before have never complained obviously that they want some change.

I don't understand what you mean, I read it like "because [consumers] are not impacted they would have to be impacted", so not sure the point.
The key in what you said is now the producer is forced to set something so it stays as before which is exactly that, keep in mind part of the producers were not controlling at all that part and another part were setting invalid values so overall validating it the hard way is the safest solution for maven and we keep backward compatibility for consumers by explaining the producer to set the goal prefix so nobody is broken.

So neither producer nor consumer requested for such feature / change

Isn't it always the case when there was a design issue in a tool and it gets fixed? Nobody asked to configure a filter for RMI serialization, it is the same there.

e.g. for the tycho case I now need to add 24 useless duplication in the config that was previously perfectly derived without a problem and users never where unhappy enough to open an issue > 10 years:

on the user point please see previous comment, it is normal they never complained since this fix does not impact them

https://github.com/eclipse-tycho/tycho/pull/3363/files

Looks good and now you control your produced plugin and you will not break your command lines (ci) even if you refactor your gav to comply to maven recommended pattern (x-maven-plugin, not x-plugin), this sounds way better to me.