Make checks for 'feature-source'/'p2-metadata' executions more precise #912
Conversation
Using an execution's goal and not its id as criteria to check for its presence is more precise. The id can be chosen arbitrarily and requiring a specific id can easily lead to false positive and false negative results.
| @@ -61,10 +61,10 @@ | |||
| /** | |||
| * Goal to create a JAR-package containing all the source files of a osgi project. | |||
| */ | |||
| @Mojo(name = "plugin-source", defaultPhase = LifecyclePhase.PREPARE_PACKAGE, threadSafe = true) | |||
There was a problem hiding this comment.
Do you think it is in general a reasonable pattern to use a constant as mojo-name (if available)?
There was a problem hiding this comment.
Do you think it would be a good pattern to be applied generally to have a public static final String GOAL field for each Mojo class that could be referenced wherever the goal id is required? It would avoid using 'magic' string literals in the code base.
There was a problem hiding this comment.
I think it does not makes much of a difference if it is in a GOAL="..." or in a name = "..." unless we reference it somewhere else (in which case it might be more useful in TychoConstants or a new TychoMojos interface)
There was a problem hiding this comment.
Sure it only makes a difference when the goal-id is used somewhere else.
The good think about such constants is that in Eclipse one can simply check the call-hierarchy to see where the goal is used and does not have to search for the String and sort out all false positive results.
I'm not sure where such constant would be placed best. My first thought was the mojo because it's a property of it.
But I'm not planning to apply this to the entire code base at the moment. So there is some time to keep this in the back of our minds.
There was a problem hiding this comment.
The mojo ids are at least used in the compontents.xml and we cant reference the constants there.
|
Thanks hannes, I think this fix is even easy enough to be cherry picket to the 2.7.2 bugfix release. |
You're welcome and thanks for reviewing/submitting/back-porting. I re-trigger the m2e-build to test Tycho 2.7.2 and the warning has vanished. So part works well. |
Using a Plugin execution's goal and not its id as criteria to check for its presence is more precise. The id can be chosen arbitrarily and requiring a specific id can easily lead to false positive and false negative results.
For example it took me some time to find out in eclipse-equinox/equinox.bundles#20 that using the 'wrong' id for the execution of the
tycho-source-plugin:feature-sourcegoal is the reason the source-feature is not found when resolving a feature that includes it. The id was actually chosen badly due to copy-pasting but the success of a build should not depend on if I name the idsource-featureorfeature-source.Furthermore the m2e build constantly displays a warning that the
tycho-p2-plugin:p2-metadatawere not executed and that this would lead to problems when including the generated source-feature in an update-site just because the execution-id isp2-metadatainstead ofattach-p2-metadata.Fixes #85