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

[MPOM-288] - Update m-plugin-p to 3.6.4 #63

Merged
merged 1 commit into from Jan 27, 2022

Conversation

kwin
Copy link
Member

@kwin kwin commented Jan 19, 2022

Manage version of maven-plugin-annotations as well

Manage version of maven-plugin-annotations as well
<groupId>org.apache.maven.plugin-tools</groupId>
<artifactId>maven-plugin-annotations</artifactId>
<version>${maven.plugin.tools.version}</version>
</dependency>
Copy link
Member Author

Choose a reason for hiding this comment

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

Should the scope be managed as "provided" for this case?

Copy link
Member

Choose a reason for hiding this comment

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

should be provided

Copy link
Member

Choose a reason for hiding this comment

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

If this was in the regular <dependencies>, I would agree with those users above who suggested adding the provided scope. However, this is in the <dependencyManagement> section. Adding the scope there would override the default compile scope that most people omit when they actually use a dependency in the regular <dependencies> section in their own POM. Changing that behavior so it's provided instead of compile can be very confusing to users, and to avoid that confusion, they will redundantly specify the scope themselves anyway. So, placing it here can create confusion, and doesn't really offer much benefit.

So, I don't think the parent POM should manage the scope for the users in a <dependencyManagement> section. Instead, the scope should be omitted here, as it currently is, so it defaults to compile like users expect when they omit the scope. If they want to mark it provided, or any other scope, they should specify the scope themselves when they declare the dependency in their regular <dependencies> section of their own POM.

Copy link
Member Author

Choose a reason for hiding this comment

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

This particular dependency should IMHO never be used with another scope, so this may warrant to set scope in depMgmt

Copy link
Member

Choose a reason for hiding this comment

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

@kwin Even if it is extremely unlikely (I'm not going to say never, because people are creative), it's still going to create confusion, because people assume if the scope isn't specified, it is compile. Maven is built around a certain amount of convention. The more you break those conventions, the more it gets confusing.

For the users who might actually want it in the compile scope, you're really creating a problem for them, because now they have to explicitly add <scope>compile</scope>... and they probably have to leave a comment in for other developers, because lots of developers would just delete that line because they know it's not necessary most of the time.

If users actually want it to be provided (and yeah, they probably would much of the time for this one), then they're probably going to set it explicitly anyway. When they do, the one in the <dependencyManagement> section is redundant and adds no value.

So, best case scenario, when users are following conventions, it does not serve a purpose because it's redundant. Worst case scenario, you're really screwing with people's understanding of Maven because you've broken a pretty widely known convention.

The <dependencyManagement> section is useful when it helps with dependency convergence (version management, transitive dependency exclusions, etc.). But setting the scope there severely disrupts conventions and has substantial downstream impact on anybody using this as a parent POM, because it quite literally alters their class paths.

I strongly advise against breaking conventions like this in such a widely used parent POM that could break people's understanding of how Maven works, or messes with which jars are available in which class paths. In general, I strongly recommend against setting the scope in any <dependencyManagement> section, unless it's <type>pom</type> and <scope>import</scope>.

@slawekjaranowski
Copy link
Member

Currently we have also defined dependency management for it in maven-parent with provided scope

https://github.com/apache/maven-parent/blob/master/pom.xml#L965-L970

And another property is used for version defined:
mavenPluginToolsVersion

@ctubbsii
Copy link
Member

@slawekjaranowski wrote:

Currently we have also defined dependency management for it in maven-parent with provided scope

https://github.com/apache/maven-parent/blob/master/pom.xml#L965-L970

And, just as I said, people can't trust the convention breakage, so they override it anyway, making it redundant. Here's two examples:

While I wouldn't recommend breaking conventions in maven-plugins parent POM either, it is at least limited to the Apache Maven plugin developers in that POM. If its scope were managed in this POM, it would break conventions for much wider set of projects across the ASF, and even projects outside the ASF.

@slawekjaranowski
Copy link
Member

Ok, for me is ok add dependencyManagement without scope.

@Tibor17
Copy link

Tibor17 commented Jan 25, 2022

The scope should not be in MPOM.
The MPOM has certain purpose to help and not to break via restrictions.
Simply dependency management declares versions, the same with plugin management.
Plugins section is different story.
The way that we declare versions of dependencies has certain purpose - we prove that the JDK version and Maven API/versions and plugins development would be in consistent state. It's the developer's responsibility to say what dependency would be on certain type of classpath (compilation, test, runtime). The more restrictions of scope we provide in MPOM the more investigation time would be spent on finding out this root cause in MPOM.

@kwin
Copy link
Member Author

kwin commented Jan 27, 2022

Can anybody approve then? Currently the scope is not managed and it seems to be consensus to leave it like that.

@slachiewicz slachiewicz merged commit 5a878dc into apache:master Jan 27, 2022
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.

None yet

5 participants