Add support for java 9 #7

Sineaggi · 2022-05-14T00:57:16Z

This adds support for the new javax.annotation.processing.Generated annotation under java 9. Under java 8 (and below) it will fall back to using the old annotation.

Fixes #6

skinny85 · 2022-05-14T01:27:01Z

Super clever, thanks for the contribution @Sineaggi!

Only one comment - any chance we can switch the GitHub Actions workflow to use matrix builds for the JDK version? Right now, we're only building for Java 1.8. Can we maybe make it build for 8, 9 and 17 (all of the stable ones)? That should allow us to exercise both paths of the change.

The workflow file is here.

Let me know if this makes sense!

Sineaggi · 2022-05-14T01:38:57Z

So off the top of my head we should be able to add support for 9 relatively easily, as 9 has been supported since gradle 4.3. Unfortunately for newer versions of java like 11 and 17, we'd either need to use java toolchains which were added in gradle 6.7 or upgrade all the way to gradle 7 (which removed the maven plugin in favor of the maven-publish plugin).

skinny85 · 2022-05-14T01:43:07Z

So off the top of my head we should be able to add support for 9 relatively easily, as 9 has been supported since gradle 4.3. Unfortunately for newer versions of java like 11 and 17, we'd either need to use java toolchains which were added in gradle 6.7 or upgrade all the way to gradle 7 (which removed the maven plugin in favor of the maven-publish plugin).

Yeah, asking you to upgrade Gradle would be taking advantage of your good nature, I'm pretty sure 😉. So let's only add Java 9 for now.

Sineaggi · 2022-05-14T02:54:31Z

@skinny85 I've got a first pass of a partial upgrade to gradle 6.x, not sure if the test matrix is correct.

The testing is a bit ugly since the import of javax.annotation.Generated requires the compiler for the main classes to be 8. Then we can set the test classes compiler/launcher to be the parameterized java version (since we need to validate that the annotation processor 1 runs under newer versions of java and 2 the tests pass under newer versions of java).

Sineaggi · 2022-05-14T03:04:07Z

Ugh just hit junit-team/junit4#1717, disabling 17 for now

skinny85 · 2022-05-14T03:04:25Z

.github/workflows/build.yaml

+      - name: Build and test the project
+        env:
+          JAVA_VERSION: ${{ matrix.version }}
+        run: ./gradlew check javadoc


Any reason we're introducing a new job here, instead of changing the existing one?

None at all, will collapse them.

skinny85 · 2022-05-14T03:05:02Z

.github/workflows/build.yaml

+    steps:
+      - uses: actions/checkout@v2
+
+      - name: Set up JDK 1.8


I think this name needs to be updated.

skinny85 · 2022-05-14T03:05:23Z

.github/workflows/build.yaml

+      - name: Set up JDK 1.8
+        uses: actions/setup-java@v1
+        with:
+          java-version: 1.8


I think we want to use ${{ matrix.version }} here?

We could, but gradle plays catch-up with every new version of java. By using the matrix version of java with the toolchain we can use unsupported versions of gradle (like 17 for example) in gradle 6 for compilation/testing.

I don't mind either way, but it does mean testing on java 17 would require gradle 7.

Which shouldn't be too hard, considering we're now on gradle 6.

I'm fine with skipping Java 17 for now.

build.gradle

skinny85 · 2022-05-14T03:08:18Z

The testing is a bit ugly since the import of javax.annotation.Generated requires the compiler for the main classes to be 8. Then we can set the test classes compiler/launcher to be the parameterized java version (since we need to validate that the annotation processor 1 runs under newer versions of java and 2 the tests pass under newer versions of java).

I'd be fine with changing to a Class.forName() for javax.annotation.Generated too, if it makes this easier.

Sineaggi · 2022-05-14T03:16:41Z

I'd be fine with changing to a Class.forName() for javax.annotation.Generated too, if it makes this easier.

That would make compiling under newer versions of java easier. We'd also be able to align main and test source sets, which would then make it easier to just use ${{ matrix.version }} for java-version.

Sineaggi · 2022-05-14T03:17:51Z

LMK what you'd prefer, I think the current way of using javaCompiler/javaLauncher ended up being pretty obtuse.

skinny85 · 2022-05-14T03:19:21Z

LMK what you'd prefer, I think the current way of using javaCompiler/javaLauncher ended up being pretty obtuse.

I agree. Let's simplify by switching to Class.forName().

Sineaggi · 2022-05-14T03:26:05Z

@skinny85 I've switched to using Class.forName for all java versions. Simplified the build.gradle and the test matrix. The build process should be clearer now.

skinny85

@skinny85 I've switched to using Class.forName for all java versions. Simplified the build.gradle and the test matrix. The build process should be clearer now.

Thanks for the contribution - it looks fantastic now!

I just have a few last small finishing comments, if you'd be so kind 🙂.

skinny85 · 2022-05-14T03:31:14Z

.github/workflows/build.yaml

@@ -5,17 +5,16 @@ on: [push, pull_request]
 jobs:
  build:
    runs-on: ubuntu-latest
-    env:
-      # disable the demon, as it's causing some problems
-      GRADLE_OPTS: -Dorg.gradle.daemon=false


Any reason to remove this?

Not sure how effective it is to use the daemon in CI.

I believe in modern gradle the daemon spawns regardless, in ci it will shut down at the end of the build.

I'm 99% sure that system property disables the daemon, but it's not a big deal.

When I was testing locally at least, these logs were printed.

To honour the JVM settings for this build a single-use Daemon process will be forked. See https://docs.gradle.org/6.9.2/userguide/gradle_daemon.html#sec:disabling_the_daemon. Daemon will be stopped at the end of the build

I can add the flag back in.

Nah, it's not a big deal. Let's just leave it as-is. Thanks!

build.gradle

skinny85 · 2022-05-14T04:22:45Z

build.gradle

    testCompile group: 'org.assertj', name: 'assertj-core', version: '3.6.2'
+    testAnnotationProcessor(getProject())
+    testAnnotationProcessor('com.squareup:javapoet:1.8.0')


Do we need this dependency for the third time? Would switching it from either compileOnly or testCompileOnly to compile / testCompile above would allow us to remove this declaration?

Or can we at least remove the second instance of it, above (the testCompileOnly 'com.squareup:javapoet:1.8.0' one)?

The testCompileOnly is no longer necessary, but since the annotation processor hasn't been repackaged by the shadow plugin yet, we'll need to keep the testAnnotationProcessor dependencies.

build.gradle

skinny85 · 2022-05-14T04:26:40Z

build.gradle

 dependencies {
    compileOnly 'com.squareup:javapoet:1.8.0'

    testCompileOnly 'com.squareup:javapoet:1.8.0'
-    testCompile group: 'junit', name: 'junit', version: '4.11'
+    testCompile group: 'junit', name: 'junit', version: '4.13.2'
    testCompile group: 'org.assertj', name: 'assertj-core', version: '3.6.2'


Same here:

Suggested change

testCompile group: 'org.assertj', name: 'assertj-core', version: '3.6.2'

testCompile 'org.assertj:assertj-core:3.6.2'

build.gradle

skinny85 · 2022-05-14T04:28:49Z

build.gradle

-        }
-    }
-}
-


Love these simplifications, thanks!

skinny85 · 2022-05-14T04:40:19Z

src/main/java/org/jilt/internal/AbstractBuilderGenerator.java

@@ -213,8 +212,20 @@ protected final String buildMethodName() {
    }

    protected final AnnotationSpec generatedAnnotation() {
+        Class<?> generatedAnnotation;


I'm not a huge fan of this variable name, mainly because it's the same as the method.

How about generatedAnnotationClass instead?

Done with separate helper method below.

skinny85 · 2022-05-14T04:41:20Z

src/main/java/org/jilt/internal/AbstractBuilderGenerator.java

+        Class<?> generatedAnnotation;
+        try {
+            // available since 9
+            generatedAnnotation = Class.forName("javax.annotation.processing.Generated");
+        } catch (ClassNotFoundException e) {
+            try {
+                generatedAnnotation = Class.forName("javax.annotation.Generated");
+            } catch (ClassNotFoundException ex) {
+                // Shouldn't be possible.
+                throw new IllegalStateException(ex);
+            }
+        }


Would you mind actually extracting this to a separate helper method? Something like private Class<?> determineGeneratedAnnotationClass() throws Exception { ...?

skinny85 · 2022-05-14T04:45:12Z

src/main/java/org/jilt/internal/AbstractBuilderGenerator.java

+            } catch (ClassNotFoundException ex) {
+                // Shouldn't be possible.
+                throw new IllegalStateException(ex);
+            }


Instead of this dead code, can you instead change this method to have throws Exception (its caller already has it, so it shouldn't cause any problems), and remove this entire catch statement?

skinny85 · 2022-05-14T05:08:09Z

.github/workflows/build.yaml

    steps:
      - uses: actions/checkout@v2

-      - name: Set up JDK 1.8
+      - name: Set up JDK


Is it legal to use dynamic expressions in step names? So this would become something like

Suggested change

- name: Set up JDK

- name: Set up JDK version ${{ matrix.version }}

?

Let's find out!

Co-authored-by: Adam Ruka <adamruka85@gmail.com>

skinny85

Looks perfect, thanks @Sineaggi!

der-R · 2023-01-03T13:43:35Z

Could you release a version with Java 9+ support as a maven artifact on the central maven repository? Would be really nice to be able to use it as a normal dependency without maintaining an own copy of the binary.

skinny85 · 2023-01-04T06:33:27Z

Yes! Apologies @der-R, I merged the PR, but completely blanked that I need to do a release!

I'll do it this week (this is a new laptop where I don't have all of the stupid PGP stuff for Maven Central set up, so it will take a while), and I'll let you know when it's done!

skinny85 · 2023-01-09T06:36:58Z

@der-R @Sineaggi Jilt version 1.2 has now been released with this fix (and also with #8). Thanks again for submitting these PRs, and apologies for the release taking so long!

Add support for java 9

2439ed0

Upgrade to gradle 6.9.2 and add test matrix

808a33f

skinny85 reviewed May 14, 2022

View reviewed changes

Sineaggi added 2 commits May 13, 2022 21:04

Disable java 17

feb3b47

Collapse test and build actions

5c416cb

skinny85 reviewed May 14, 2022

View reviewed changes

Simplify matrix

2dd3774

skinny85 reviewed May 14, 2022

View reviewed changes

Sineaggi and others added 5 commits May 14, 2022 09:50

Update build.gradle

38422ce

Co-authored-by: Adam Ruka <adamruka85@gmail.com>

Update build.gradle

7997adb

Co-authored-by: Adam Ruka <adamruka85@gmail.com>

Simplify dependency strings

ca29d07

Extract annotation method

8d272b0

Parameterize step name

bc18f2c

skinny85 approved these changes May 14, 2022

View reviewed changes

skinny85 merged commit 9474885 into skinny85:master May 14, 2022

Sineaggi deleted the add-java-9-support branch May 14, 2022 16:58

	testCompile group: 'org.assertj', name: 'assertj-core', version: '3.6.2'
	testCompile 'org.assertj:assertj-core:3.6.2'

	- name: Set up JDK
	- name: Set up JDK version ${{ matrix.version }}

Add support for java 9 #7

Add support for java 9 #7

Conversation

Sineaggi commented May 14, 2022 • edited

skinny85 commented May 14, 2022

Sineaggi commented May 14, 2022

skinny85 commented May 14, 2022

Sineaggi commented May 14, 2022

Sineaggi commented May 14, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skinny85 commented May 14, 2022

Sineaggi commented May 14, 2022

Sineaggi commented May 14, 2022

skinny85 commented May 14, 2022

Sineaggi commented May 14, 2022

skinny85 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sineaggi May 14, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skinny85 May 14, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skinny85 May 14, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skinny85 left a comment

Choose a reason for hiding this comment

der-R commented Jan 3, 2023

skinny85 commented Jan 4, 2023

skinny85 commented Jan 9, 2023

Sineaggi commented May 14, 2022 •

edited

Sineaggi commented May 14, 2022 •

edited

Sineaggi May 14, 2022 •

edited

skinny85 May 14, 2022 •

edited

skinny85 May 14, 2022 •

edited