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

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

Merged
merged 1 commit into from
Jan 27, 2022
Merged

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

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
13 changes: 12 additions & 1 deletion pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,10 +92,21 @@ under the License.
<maven.compiler.source>${maven.compiler.target}</maven.compiler.source>
<maven.compiler.target>1.7</maven.compiler.target>
<surefire.version>2.22.2</surefire.version><!-- for surefire, failsafe and surefire-report -->
<maven.plugin.tools.version>3.6.4</maven.plugin.tools.version><!-- for m-plugin-p and maven-plugin-annotations -->
<assembly.tarLongFileMode>posix</assembly.tarLongFileMode>
<project.build.outputTimestamp>2021-07-14T15:10:38Z</project.build.outputTimestamp>
</properties>

<dependencyManagement>
<dependencies>
<dependency>
<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>.

</dependencies>
</dependencyManagement>

<repositories>
<repository>
<id>apache.snapshots</id>
Expand Down Expand Up @@ -220,7 +231,7 @@ under the License.
<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-plugin-plugin</artifactId>
<version>3.6.2</version>
<version>${maven.plugin.tools.version}</version>
</plugin>
<plugin>
<groupId>org.apache.maven.plugins</groupId>
Expand Down