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

Jump to bottom

Specify release flag to build on newer JDKs #2609

Open
wants to merge 5 commits into
base: integration
Choose a base branch
from
Open

Specify release flag to build on newer JDKs #2609

wants to merge 5 commits into from

Conversation

ctubbsii
Copy link
Collaborator

  • Add maven.compiler.release option to force correct cross-compilation to Java 11 when building with a JDK newer than 11, such as JDK 17
  • Remove unnecessary java source, target, and encoding config from plugins when those are already specified by properties
  • Fix submodule for core/in-memory-accumulo to point to in-memory-accumulo's main branch instead of the one being developed for the accumulo4 branch
  • Also fix the license name for Apache-2.0 (the recommended name is the SPDX name) and drop .txt from the license URL

@ctubbsii
Copy link
Collaborator Author

It looks like the checks failed because my user doesn't have permission to read from the package repository on GitHub. I'm not sure how to fix that.

But, I did test this using JDK 17 with mvn -Pdev -Ddeploy -Dtar clean package -DskipTests

@ctubbsii ctubbsii force-pushed the support-builds-with-jdk17-and-newer branch from 5725f6b to 9aa7475 Compare October 12, 2024 00:56
@ctubbsii
Specify release flag to build on newer JDKs
e7a0a59 
* Add maven.compiler.release option to force correct cross-compilation
  to Java 11 when building with a JDK newer than 11, such as JDK 17
* Remove unnecessary java source, target, and encoding config from
  plugins when those are already specified by properties
* Fix submodule for core/in-memory-accumulo to point to
  in-memory-accumulo's main branch instead of the one being developed
  for the accumulo4 branch
* Also fix the license name for Apache-2.0 (the recommended name is the
  SPDX name) and drop .txt from the license URL
@ctubbsii ctubbsii force-pushed the support-builds-with-jdk17-and-newer branch from 9aa7475 to e7a0a59 Compare October 12, 2024 01:42
@ctubbsii
Copy link
Collaborator Author

ctubbsii commented Oct 12, 2024 •
edited
Loading

I force-pushed a few times to try to re-trigger the tests. I couldn't figure out how to get the tests to work in the pull request. I ran the tests manually in my own repo, with it's own local secrets for reading the dependencies from the package repository for the build. Those results are at: https://github.com/ctubbsii/datawave/actions/runs/11301797381

@ctubbsii
Update GitHub Actions to test build with JDK 17
595ba5f
@ctubbsii
Bump spotbugs so it works with newer JDKs
7a58cd6 
* Update spotbugs-maven-plugin
* Configure javadoc doclint to 'all,-missing' to prevent noise from
  missing javadocs
@ctubbsii
Fix javadoc headers and html conventions
dc927e5 
* Fix javadoc issues about the use of uppercase header elements, which
  breaks the standard conventions of html5 that carried over from XHTML
  days, which was case-sensitive (and because uppercase looks ugly)
* Also fix a few other places where uppercase html5 was used, and remove
  an out-of-place closing paragraph element (that also used uppercase)
* Demote `<h1>` tags to `<h2>` in javadoc comments because javadoc
  reserves `<h1>` for its own generated headers
@ctubbsii
Fix some spotbugs issues and POM issues
3a060fd 
* Remove redundant items in POMs
* Fix spotbugs issues regarding unnecessary boxing/unboxing primitives
@ctubbsii
Copy link
Collaborator Author

I tried to include a few quality issues that were interfering with a clean build with spotbugs, and other things enabled that are activated in the GitHub Actions checks, but I was unable to fix everything, because there are similar quality problems with maven-compiler-plugin, and spotbugs, in some of the microservice dependencies, and it's not clear to me how to get a clean build here without first making commits to those other projects and deploying changes to them (which I'm not prepared to do today... but maybe I can submit some PRs later to fix those... it's kind of hard because there's so many and I don't know where to begin).

@ctubbsii
Copy link
Collaborator Author

Okay, I spent a bit more time with this. With these changes, I was able to get the command mvn -Pdev,examples,assemble,spotbugs,dist,deploy-ws clean verify -DskipTests to finish successfully. This is similar to the build executed by the tests.yml GitHub Actions workflow. However, the only way I could actually get this to work is if I made similar changes in the microservices-parent POM and a few others, and then update the <parent> tags of all those that depend on those updated POMs to use the SNAPSHOT version I edited, rather than the published one in the package repo.

This required changing 79 of the 128 pom.xml files I found in this project spanning 27 separate repos that were linked as submodules (which seems like a lot to me for a simple set of changes). Is there an easier way to make changes, or is this complexity inherent to the project's chosen build structure? If I want to make subsequent PRs to fix all the repos pointed to by the submodules, what is the preferred order for submitting those PRs, so that they can be available for the other repos?

ctubbsii
ctubbsii commented Oct 12, 2024
View reviewed changes
...use/query-core/src/main/java/datawave/query/language/parser/lucene/AccumuloSyntaxParser.java
@@ -358,7 +358,7 @@ final public QueryNode Clause(CharSequence field) throws ParseException {
if (boost != null) {
float f = (float) 1.0;
try {
f = Float.valueOf(boost.image).floatValue();
f = Float.parseFloat(boost.image);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This spotbugs issue was found with the updated spotbugs plugin. However, it appears that this file is generated code that has been checked in to the repository. Is this file frequently re-generated and clobbered, or is it okay to make these fixes here? Would the code-generation utilities generate better (non-buggy) code if it were updated? Where is that code generator executed?

@foster33
Copy link
Collaborator

I force-pushed a few times to try to re-trigger the tests. I couldn't figure out how to get the tests to work in the pull request. I ran the tests manually in my own repo, with it's own local secrets for reading the dependencies from the package repository for the build. Those results are at: https://github.com/ctubbsii/datawave/actions/runs/11301797381

AFAIK checks do not work with PRs based off of a fork

@ctubbsii
Copy link
Collaborator Author

Anybody able to code review this?

@mineralntl
Copy link
Collaborator

Can all the HTML tag stuff be removed from this? Or is that actually somehow related to the JDK part?

alerman
alerman reviewed Mar 11, 2025
View reviewed changes
.github/workflows/tests.yml
JAVA_VERSION: '11'
JAVA_DISTRIBUTION: 'zulu' #This is the default on v1 of the action for 1.8
JAVA_VERSION: '17'
JAVA_DISTRIBUTION: 'adopt'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why are you changing from Zulu to Adopt?

We use zulu in deployed applications and usually prefer to use the same version in ci as in production.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I believe it was because Java 17 zulu distribution wasn't available in GitHub Actions at the time I tried, or that version had a bug that caused a problem. It probably doesn't matter now and can go back.

@alerman
Copy link
Collaborator

alerman commented Mar 11, 2025

Okay, I spent a bit more time with this. With these changes, I was able to get the command mvn -Pdev,examples,assemble,spotbugs,dist,deploy-ws clean verify -DskipTests to finish successfully. This is similar to the build executed by the tests.yml GitHub Actions workflow. However, the only way I could actually get this to work is if I made similar changes in the microservices-parent POM and a few others, and then update the <parent> tags of all those that depend on those updated POMs to use the SNAPSHOT version I edited, rather than the published one in the package repo.

This required changing 79 of the 128 pom.xml files I found in this project spanning 27 separate repos that were linked as submodules (which seems like a lot to me for a simple set of changes). Is there an easier way to make changes, or is this complexity inherent to the project's chosen build structure? If I want to make subsequent PRs to fix all the repos pointed to by the submodules, what is the preferred order for submitting those PRs, so that they can be available for the other repos?

@ctubbsii Since you had put this PR in, we have moved the microservices back into the main repository to ease the burden on making changes precisely like this.

@ctubbsii
Copy link
Collaborator Author

@ctubbsii Since you had put this PR in, we have moved the microservices back into the main repository to ease the burden on making changes precisely like this.

I can rebase and issue a new PR from a side-branch in the repo, so we can get the GitHub Actions to run.

@ctubbsii
Copy link
Collaborator Author

Can all the HTML tag stuff be removed from this? Or is that actually somehow related to the JDK part?

At least some of them will need to be done because newer JDK javadoc tools will fail on some (but perhaps not all) of these. The main one is that <h1> is reserved for javadoc itself, and cannot be used in user javadoc code. The build will fail when building javadocs if that isn't fixed. Other quality check tools may check for compliance with html5 conventions, and fail on the others, but I did not wait to observe a failure before fixing them. I just fixed them all. I don't think it's worth trying to tease apart the strictly required fixes from the optional ones that any quality checks might let slide. The changes are trivial, and by conforming to html5 norms, the fixes ensure that it will pass any html5 quality checks whether this project is running those checks or not.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alerman alerman alerman left review comments

@jwomeara jwomeara Awaiting requested review from jwomeara

@ivakegg ivakegg Awaiting requested review from ivakegg

@keith-turner keith-turner Awaiting requested review from keith-turner

At least 2 approving reviews are required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ctubbsii @foster33 @mineralntl @alerman