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

[MRESOLVER-408] Make master 2.0.0-SNAPSHOT #335

Merged
merged 13 commits into from
Oct 12, 2023
Merged

Conversation

cstamas
Copy link
Member

@cstamas cstamas commented Oct 10, 2023

Changes:

  • make version 2.0.0-SNAPSHOT
  • build time JDK requirement: raise to 11 (raise as needed, preferably use LTS versions)
  • existing modules are release=8
  • drop animal sniffer
  • redefine enforceBytecodeVersion: we do produce Java 8 stuff, but for testing we may want and will want to use something that is 8+ (ie. Jetty 12)
  • fix sisu (was invoked twice, last parent POM update)
  • compiler: proactively disable annotation processing (we do not use those, but Sisu APT is present, we do not want it auto-picked up, we explicitly index using sisu mojo)
  • compiler: be chatty about deprecations, to help us down the line removing them

https://issues.apache.org/jira/browse/MRESOLVER-408

@cstamas cstamas self-assigned this Oct 10, 2023
@cstamas cstamas added this to the 2.0.0 milestone Oct 10, 2023
pom.xml Outdated
<project.build.outputTimestamp>2023-09-22T18:13:46Z</project.build.outputTimestamp>
<mavenVersion>3.9.5</mavenVersion>
<minimalMavenBuildVersion>[3.8.8,)</minimalMavenBuildVersion>
<minimalJavaBuildVersion>[17.0.6,)</minimalJavaBuildVersion>
Copy link
Member

Choose a reason for hiding this comment

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

What is the benefit to require Java 17 to build it? Why is 11 not enough?

Copy link
Member Author

Choose a reason for hiding this comment

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

IMHO next to last LTS is good compromise for build time requirement for our anyway dated stack we use. In other words, I want to encourage experiments, and am willing even to go for Java 21 but there are some spotless issues (@slawekjaranowski plz help). Also, I must emphasize: most resolver modules will be still release=8 compiled, but probably not all. As we may have new modules, that do want all the new nice things we can get.

Am pretty much sure, that our users run Maven on latest Java versions (unlike "aligners"), so as resolver being one of the Maven fundamentals libraries, let's make a leap here.

Copy link
Member

Choose a reason for hiding this comment

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

Issue with spotless fix by workaround in parent apache/maven-parent#135

Copy link
Member

Choose a reason for hiding this comment

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

I still don't see a reason to artificially limit this. You still can build with newer ones.

Copy link
Member

Choose a reason for hiding this comment

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

It is limit only for minimum - you can build by newer versions.

Copy link
Member Author

@cstamas cstamas Oct 11, 2023

Choose a reason for hiding this comment

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

There ARE technical (and human) justifications. For example, remember this thread? 😄
https://lists.apache.org/thread/k5sc3w7w4xkgjfzyxxltg06722dvspk7

In short, we tend to say "Java 8" but there are important Java updates, hence there is java 8u172 from 2018 and Java 8u371 from 2023. This thread happened to us, and clearly shows there is sense to set "lower limit". I just dont want to waste my time and resources on things like happened in that thread anymore.

This limit simply says (and means) "recent Java 17" and NOT "just any java 17" as we used to say so far. This is far better, explicit, and even the error message makes sense as shown here:
https://lists.apache.org/thread/tz4qfqhvrvf2fr84l8loqsfpmcf5poz4

Copy link
Member

Choose a reason for hiding this comment

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

You can enforce 8u300+, no?

Copy link
Member Author

Choose a reason for hiding this comment

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

I can, but why would I? I want to add modules that do HTTP/2, and require Java11+, moreover, for testing those I may want to use dependencies (test scoped) that are Java17...

What would limiting build time Java requirement to 8u300 help with those above?

Copy link
Member

Choose a reason for hiding this comment

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

Do you have Java 17 test deps already?

Copy link
Contributor

Choose a reason for hiding this comment

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

Imho, I would keep it as low as needed, but certainly would not put any barrier in raising the build time requirement. This would allow using multi-release JARs if needed or even leverage new JEP such as FFM (for accessing native code or memory). Whatever is needed at build time should be fine.

Copy link
Member

@slawekjaranowski slawekjaranowski left a comment

Choose a reason for hiding this comment

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

for me ok - please also fix in Jenkinsfile

@cstamas cstamas requested a review from gnodet October 11, 2023 18:35
Jenkinsfile Outdated Show resolved Hide resolved
* sisu was invoked twice
* compiler: we do not use annotation processors (while Sisu APT is present, so we do not want it auto-picked up)
* compiler: be explicit about deprecations (to help us removing that cruft)
@cstamas cstamas requested review from michael-o, olamy and gnodet October 12, 2023 16:51
Copy link
Contributor

@gnodet gnodet left a comment

Choose a reason for hiding this comment

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

LGTM, though if there's no need to bump the build time JDK requirement, I would keep it low. Whenever the needs come, we can bump it.

@cstamas cstamas changed the title Make master 2.0.0-SNAPSHOT [MRESOLVER-408] Make master 2.0.0-SNAPSHOT Oct 12, 2023
@cstamas cstamas merged commit 67b2552 into apache:master Oct 12, 2023
10 checks passed
@cstamas cstamas deleted the resolver-2.x branch October 12, 2023 20:58
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.

5 participants