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.

Sign up for GitHub

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

[MNG-5695] document Maven 3.2.5+ scoped components usage #236

Merged
merged 1 commit into from
Jan 6, 2024
Merged

[MNG-5695] document Maven 3.2.5+ scoped components usage #236

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 27 additions & 5 deletions maven-plugin-tools-annotations/src/site/apt/index.apt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ public class MyMojo
@Parameter( name = "parameter",
alias = "myAlias",
property = "a.property",
defaultValue = "an expression, possibly with ${variables}",
defaultValue = "an expression, possibly with ${variables} and pseudo-parameter expressions ${project.xxx.yyy}",
readonly = <false|true>,
required = <false|true> )
private String parameter;
Expand All @@ -94,27 +94,42 @@ public class MyMojo
hint = "..." )
private MyComponent component;

// pseudo-parameters (marked read-only) permitting injection of Maven build context objects
// sample objects taken from Maven API through PluginParameterExpressionEvaluator
// https://maven.apache.org/ref/current/maven-core/apidocs/org/apache/maven/plugin/PluginParameterExpressionEvaluator.html
// plugins targetting Maven 3.2.5+ (after MNG-5695) should not use these pseudo-parameters any more,
// but @Component and Maven APIs to get better compiler-time checks

@Parameter( defaultValue = "${session}", readonly = true )
// @Parameter( defaultValue = "${session}", readonly = true )
@Component // since Maven 3.2.5, thanks to MNG-5695
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have a warning for such components ...

https://github.com/apache/maven-plugin-tools/blob/master/maven-plugin-tools-api/src/main/java/org/apache/maven/tools/plugin/util/PluginUtils.java#L61

and Component is replaced by expression
https://github.com/apache/maven-plugin-tools/blob/master/maven-plugin-tools-annotations/src/main/java/org/apache/maven/tools/plugin/extractor/annotations/JavaAnnotationsMojoDescriptorExtractor.java#L791

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We were discussing and leaning to the exact opposite.
Keep @Parameter for things injected from the config and @Component/@Inject for injecting beans, including those from the session (Session) and mojoExecution scopes (MojoExecution, Project)...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We were discussing and leaning to the exact opposite. Keep @Parameter for things injected from the config and @Component/@Inject for injecting beans, including those from the session (Session) and mojoExecution scopes (MojoExecution, Project)...

This is exactly what I remember. Use @Parameter exactly for this case.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We were discussing and leaning to the exact opposite. Keep @Parameter for things injected from the config and @Component/@Inject for injecting beans, including those from the session (Session) and mojoExecution scopes (MojoExecution, Project)...

This is exactly what I remember. Use @Parameter exactly for this case.

But those components can also be injected with @Component, so this is weird to warn about something that is supported by sisu (because those are actually bound to scopes), while pushing users to a custom solution. Why only a few components would be accessible by name then ?
I would agree if we had a single annotation for both use cases, but that's not the case in Maven 3. I suppose we could try for the v4 api though.

Copy link
Member

@michael-o michael-o Nov 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We were discussing and leaning to the exact opposite. Keep @Parameter for things injected from the config and @Component/@Inject for injecting beans, including those from the session (Session) and mojoExecution scopes (MojoExecution, Project)...

This is exactly what I remember. Use @Parameter exactly for this case.

But those components can also be injected with @Component, so this is weird to warn about something that is supported by sisu (because those are actually bound to scopes), while pushing users to a custom solution. Why only a few components would be accessible by name then ? I would agree if we had a single annotation for both use cases, but that's not the case in Maven 3. I suppose we could try for the v4 api though.

I agree that this is inconsistent. If both are valid and are functionally identical then the warning is wrong.

Copy link
Member Author

@hboutemy hboutemy Nov 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so do we all now agree that using @Component is better than using @Parameter( defaultValue = "${session}", readonly = true ) (because smaller and moreover we're sure there is no typo in the default value expression)?

if we agree, the warning could be to promote switch to @Component for ${session}, ${project} and ${mojoExecution}...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and do we all now agree that using @Component is better than using @Parameter( defaultValue = "${session}", readonly = true ) (because smaller and moreover we're sure there is no typo in the default value expression)?

if we agree, the warning could be to promote switch to @Component...

From my PoV, is this cannot be provided by POM or CLI means and it is unambiguous, then yes.

Copy link
Member Author

@hboutemy hboutemy Nov 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reading git blame on https://github.com/apache/maven-plugin-tools/blob/master/maven-plugin-tools-annotations/src/main/java/org/apache/maven/tools/plugin/extractor/annotations/JavaAnnotationsMojoDescriptorExtractor.java#L791 , I see that in the past, we (I :) ) implemented in plugin-tools a hack to replace @Component with an expression in the plugin descriptor: https://issues.apache.org/jira/browse/MPLUGIN-204

It was was done when we were with Maven 3.0.x, if it was before Maven 3.2.5 MNG-5695. With Maven 3.2.5, this hack is not useful any more for MavenSession, MavenProject nor MojoExecution: it remains useful for a few other types...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMHO there is no problem when JSR330 being used. Problem happens when Mojo APIs (java5 anno or javadoc annos are being used).

private MavenSession session;

@Parameter( defaultValue = "${project}", readonly = true )
// @Parameter( defaultValue = "${project}", readonly = true )
@Component // since Maven 3.2.5, thanks to MNG-5695
private MavenProject project;

@Parameter( defaultValue = "${mojoExecution}", readonly = true )
private MojoExecution mojo;
// @Parameter( defaultValue = "${mojoExecution}", readonly = true )
@Component // since Maven 3.2.5, thanks to MNG-5695
private MojoExecution mojoExecution;

@Parameter( defaultValue = "${reactorProjects}", readonly = true )
// prefer using session.getProjects()
private List<MavenProject> reactorProjects;

@Parameter( defaultValue = "${plugin}", readonly = true ) // Maven 3 only
// prefer using mojoExecution.getMojoDescriptor()
private PluginDescriptor plugin;

@Parameter( defaultValue = "${settings}", readonly = true )
// prefer using session.getSettings()
private Settings settings;

@Parameter( defaultValue = "${project.basedir}", readonly = true )
// prefer using project.getBasedir()
private File basedir;

@Parameter( defaultValue = "${project.build.directory}", readonly = true )
// prefer using project.getBuild().getDirectory()
private File target;

/**
Expand Down Expand Up @@ -144,3 +159,10 @@ public class MyMojo

* {{{/ref/current/maven-core/apidocs/org/apache/maven/plugin/PluginParameterExpressionEvaluator.html}PluginParameterExpressionEvaluator}},
used to evaluate plugin parameters values during Mojo configuration,

* pseudo parameters:

* <<<PluginParameterExpressionEvaluator>>> {{{https://maven.apache.org/ref/current/maven-core/apidocs/org/apache/maven/plugin/PluginParameterExpressionEvaluator.html}javadoc}} /
{{{https://maven.apache.org/ref/current/maven-core/xref/org/apache/maven/plugin/PluginParameterExpressionEvaluator.html}source}}

* {{{https://issues.apache.org/jira/browse/MNG-5695}MNG-5695}}: scoped objects added to Guice/Sisu in {{{https://maven.apache.org/ref/current/maven-core/}maven-core}} 3.2.5