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

[MPLUGIN-522] Prerequisites should be opt-in #282

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

cstamas
Copy link
Member

@cstamas cstamas commented May 7, 2024

But they are not. In fact, if user does not deal with them, they are way too aggressive and in fact, plain wrong.

Prerequisite was opt-in and should have remain opt-in.

This PR:

  • by default does NOT add prerequisite (as before)
  • user can set "auto" (when it is IMHO wrongly determined)
  • or can set any value needed

https://issues.apache.org/jira/browse/MPLUGIN-522

But they are not. In fact, if user does not deal with them, they
are way too aggressive and in fact, plain wrong.

Prerequisite was opt-in and should have remain opt-in.

---

https://issues.apache.org/jira/browse/MPLUGIN-522
@cstamas cstamas self-assigned this May 7, 2024
@kwin
Copy link
Member

kwin commented May 7, 2024

and in fact, plain wrong.

I disagree with this statement, and also disagree that they should be opt in. Details in the JIRA: https://issues.apache.org/jira/browse/MPLUGIN-522?focusedCommentId=17844188&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-17844188

Copy link
Member

@kwin kwin left a comment

Choose a reason for hiding this comment

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

I think enforcing and explicitly stating prerequisites is a good thing and the default matches for 99% of the cases. For other cases just overwrite with explicit values. Not stating prerequisites should no longer be supported because it is frustratring for users to figure out the implicit prerequisites through trial and error...

@cstamas
Copy link
Member Author

cstamas commented May 7, 2024

Ok, then we have similar situation as with prefix: fail the build.

Plugin should be modified to:

  • if none set, fail the build
  • if "auto" set by user, everything happens as now
  • if some value set by user, use that

We must make users aware of this requirement (hence the build failure), as otherwise they implicitly use "auto" and end result is wrong.

@gnodet
Copy link
Contributor

gnodet commented May 7, 2024

I think enforcing prerequisites is a good thing and the default match for 99% of the cases. For other cases just overwrite with explicit values. Not stating prerequisites should no longer be supported because it is frustratring for users to figure out the implicit prerequisites through trial and error...

Putting prerequisites which are greater than the one actually used to build are, at first, very weird. For example, if you project requires jdk 11, you build with jdk 11, test it with jdk 11, but your prerequisites ends up with jdk 22... ? That's really unexpected, and the user has no real knowledge about the value used.

The prerequisites should not be higher than the target JDK.

@cstamas
Copy link
Member Author

cstamas commented May 7, 2024

@kwin
Copy link
Member

kwin commented May 7, 2024

Putting prerequisites which are greater than the one actually used to build are, at first, very weird.

Let's focus on issue with the automatic detection. Please point me to a concrete example where this is the case and where it is wrong.

@cstamas
Copy link
Member Author

cstamas commented May 7, 2024

I think enforcing and explicitly stating prerequisites is a good thing and the default matches for 99% of the cases. For other cases just overwrite with explicit values. Not stating prerequisites should no longer be supported because it is frustratring for users to figure out the implicit prerequisites through trial and error...

Just to repeat myself, the prerequisites are documented, so no need for user trial-and-error. This table shows Maven and Java prerequisites clearly:
https://maven.apache.org/plugins/maven-jar-plugin/plugin-info.html#system-requirements-history

@cstamas
Copy link
Member Author

cstamas commented May 7, 2024

This PR should be redone once Maven3 and Maven4 m-p-p are split (into maven-3.x and master) branch. And then:

  • default Java prerequisite should pick up from compiler plugin (in pretty much similar way as report does)
  • default Maven prerequisite should be "3.2.5" on maven-3.x branch and "4.0.0" on master branch.

Originally, the lack of prerequisite meant "it works with Maven3" (no range specified). IMO, using "3.2.5" today is completely okay, it covers 10 years of history. If users wants to go more backward, it can explicitly configure prerequisite to "3.1.1" or whatever.

@cstamas
Copy link
Member Author

cstamas commented May 7, 2024

But just for the record: current m-p-p releases produce broken Maven Plugin XML, in a way that plugin works just fine with Maven3, while Maven4 explodes with invalid error messages.

@cstamas cstamas marked this pull request as draft May 7, 2024 09:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants