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

Use oracle-actions/setup-java for cross-version workflow #2658

Merged
merged 1 commit into from Jun 14, 2022

Conversation

scordio
Copy link
Member

@scordio scordio commented Jun 14, 2022

No description provided.

@scordio scordio marked this pull request as ready for review June 14, 2022 20:09
@scordio scordio merged commit 341840d into main Jun 14, 2022
@scordio scordio deleted the oracle-actions_setup-java branch June 14, 2022 20:09
@vlsi
Copy link
Contributor

vlsi commented Jun 14, 2022

Just wondering: what are the benefits?

@scordio
Copy link
Member Author

scordio commented Jun 15, 2022

The benefits are mainly going to the source and using early access builds earlier than when using actions/setup-java.

@filiphr
Copy link
Contributor

filiphr commented Jun 15, 2022

The benefits are mainly going to the source and using early access builds earlier than when using actions/setup-java.

This is actually incorrect. You can get easy access builds using actions/setup-java as well. The Zulu JDK provides them, even after the finals have been released. Not sure if the oracle actions keep the EA builds after they are out, if they don't it can lead to actions failing because the JDK cannot be fetched.

@scordio
Copy link
Member Author

scordio commented Jun 15, 2022

This is actually incorrect. You can get easy access builds using actions/setup-java as well. The Zulu JDK provides them, even after the finals have been released.

I don't see any incorrect parts in my statement 😉

I didn't say it cannot be done with actions/setup-java, I'm saying we can get even earlier access to the latest EA versions with oracle-actions/setup-java. Take as an example 20-ea, currently not available with Zulu but available on jdk.java.net.

Not sure if the oracle actions keep the EA builds after they are out, if they don't it can lead to actions failing because the JDK cannot be fetched.

If you check the code changes, there is nothing saying that 19 and 20 are EA builds, there is just a latest version parameter so I'd expect the action to switch automatically to the GA build once available.

Also mentioned in their README:

jdk.java.net for the current OpenJDK General Availability build and for OpenJDK Early-Access builds.

Considering the target of our cross-version workflow, this is exactly what we need. I wouldn't mind having the build fail if a version is removed because it's no longer the current GA build. We would get an immediate trigger for keeping our build config up-to-date and the effort to fix it would be minimal.

@sormuras did I get the idea behind it correctly?

@vlsi
Copy link
Contributor

vlsi commented Jun 15, 2022

19 and 20

Testing with EA is cool, however, I believe it would be beneficial to add tests with JVMs from different vendors or even different JIT.

I've explained the suggestion in #2660

@delabassee
Copy link

I'm saying we can get even earlier access to the latest EA versions with oracle-actions/setup-java. Take as an example 20-ea, currently not available with Zulu but available on jdk.java.net.

In addition to 20-EA+1, 19-EA+26 builds are also missing from Azul. Both set of builds were released last Thursday on jdk.java.net.

But more importantly than the lag and/or the missing EA builds is the fact that the jdk.java.net builds are aligned with the upstream OpenJDK development, ex. an issue should be logged against a specific jdk.java.net build, not against another vendor's build.

@delabassee
Copy link

Testing with EA is cool

@vlsi What about actively participating in the OpenJDK Quality Outreach program?
It's more than just cool 😉 as it's a win-win for both Assert-J and the OpenJDK community at large. If you're interested or want more details, just ping me.

@scordio
Copy link
Member Author

scordio commented Jun 15, 2022

Hey @delabassee, I was looking into it a long time ago but got distracted and lazy so never completed it 🙂

Happy to get it done for AssertJ, I'll get in touch with you separately.

@filiphr
Copy link
Contributor

filiphr commented Jun 15, 2022

I don't see any incorrect parts in my statement 😉

I read it as they aren't available, that is my bad.

Considering the target of our cross-version workflow, this is exactly what we need. I wouldn't mind having the build fail if a version is removed because it's no longer the current GA build. We would get an immediate trigger for keeping our build config up-to-date and the effort to fix it would be minimal.

I went through your workflows and I realized that you are not testing with Java 8 nor with Java 11. Am I seeing this wrong, or is the goal of the AssertJ project not to validate against those Java versions?

But more importantly than the lag and/or the missing EA builds is the fact that the jdk.java.net builds are aligned with the upstream OpenJDK development, ex. an issue should be logged against a specific jdk.java.net build, not against another vendor's build.

I completely, understand your position @delabassee. This is not the right place to discuss this, but with you not providing the Oracle JDK through actions/setup-java you are putting the burden on the Open Source maintainers to maintain multiple workflows. I'll add more comments to actions/setup-java#69

@scordio
Copy link
Member Author

scordio commented Jun 15, 2022

I went through your workflows and I realized that you are not testing with Java 8 nor with Java 11. Am I seeing this wrong, or is the goal of the AssertJ project not to validate against those Java versions?

Dropping the older LTS version has been a conscious decision we took, mostly related to Javadoc stylesheet compatibility issues between different LTS versions. That's why now AssertJ requires Java 17 for the build (daef040).

In general, our experience is that the problems are never related to older LTS releases, so we accepted this as a reasonable compromise.

@vlsi
Copy link
Contributor

vlsi commented Jun 15, 2022

general, our experience is that the problems are never related to older LTS releases

Here's a set of issues where 1.8 produced invalid classfiles when code used type annotations:
policeman-tools/forbidden-apis#173 (comment)

@scordio
Copy link
Member Author

scordio commented Jun 15, 2022

general, our experience is that the problems are never related to older LTS releases

Here's a set of issues where 1.8 produced invalid classfiles when code used type annotations: policeman-tools/forbidden-apis#173 (comment)

I don't get your point: is this about the Java version AssertJ users would use, or about the version used to build the AssertJ artifact?

Maybe I should have said that AssertJ problems are usually not related to older LTS releases.

@filiphr
Copy link
Contributor

filiphr commented Jun 15, 2022

Dropping the older LTS version has been a conscious decision we took, mostly related to Javadoc stylesheet compatibility issues between different LTS versions. That's why now AssertJ requires Java 17 for the build

I missed that and I didn't see it in the readme. I completely agree about building, but I would expect for AssertJ to still test that the AssertJ build artifact (with JDK 17) is still working properly when testing with JDK 11 and JDK 8.

I haven't used the latest AssertJ versions, but it is something that might be useful for the project to validate that it works for your users on older JDKs.

@scordio
Copy link
Member Author

scordio commented Jun 15, 2022

I would expect for AssertJ to still test that the AssertJ build artifact (with JDK 17) is still working properly when testing with JDK 11 and JDK 8.

I fully agree, however that goes more in the direction of integration tests, something that it's currently not very flexible with the current Maven setup and would collide with the limitations I mentioned before.

We plan to revise the entire module config in #2424 with a special focus on integration tests.

@vlsi
Copy link
Contributor

vlsi commented Jun 16, 2022

@delabassee , what is the way to file a ticket for a bug in javac?

The issue is reproducible in Java 17, 18, 19, 20.

The test case is as follows:

# install Java 20 from jdk.java.net
# This makes Object.hashCode return 1 always
export _JAVA_OPTIONS=-XX:+UnlockExperimentalVMOptions -XX:hashCode=2
./mvnw -V --no-transfer-progress -e verify

Expected: AssertJ should compile
Actual:

Error:  COMPILATION ERROR : 
[INFO] -------------------------------------------------------------
Error:  /Users/runner/work/assertj-core/assertj-core/src/main/java/org/assertj/core/util/Lists.java:[64,36] Methode für collect(java.util.stream.Collector<java.lang.Object,capture#1 von ?,java.util.Collection<java.lang.Object>>) nicht geeignet
    Methode java.util.stream.Stream.<R>collect(java.util.function.Supplier<R>,java.util.function.BiConsumer<R,? super capture#2 von ? extends T>,java.util.function.BiConsumer<R,R>) ist nicht anwendbar
      (Typvariable(n) R nicht ableitbar
        (Liste der tatsächlichen Argumente hat eine andere Länge als die der formalen Argumente))
    Methode java.util.stream.Stream.<R,A>collect(java.util.stream.Collector<? super capture#2 von ? extends T,A,R>) ist nicht anwendbar
      (Inferenzvariable C hat inkompatible Grenzwerte
        Gleichheits-Constraints: java.lang.Object,R,capture#2 von ? extends T
        Untere Grenzwerte: java.lang.Object,java.util.Collection<T>)
Error:  /Users/runner/work/assertj-core/assertj-core/src/main/java/org/assertj/core/util/Lists.java:[78,36] Methode für collect(java.util.stream.Collector<java.lang.Object,capture#3 von ?,java.util.Collection<java.lang.Object>>) nicht geeignet
    Methode java.util.stream.Stream.<R>collect(java.util.function.Supplier<R>,java.util.function.BiConsumer<R,? super capture#4 von ? extends T>,java.util.function.BiConsumer<R,R>) ist nicht anwendbar
      (Typvariable(n) R nicht ableitbar
        (Liste der tatsächlichen Argumente hat eine andere Länge als die der formalen Argumente))
    Methode java.util.stream.Stream.<R,A>collect(java.util.stream.Collector<? super capture#4 von ? extends T,A,R>) ist nicht anwendbar
      (Inferenzvariable C hat inkompatible Grenzwerte
        Gleichheits-Constraints: java.lang.Object,R,capture#4 von ? extends T
        Untere Grenzwerte: java.lang.Object,java.util.Collection<T>)
Error:  /Users/runner/work/assertj-core/assertj-core/src/main/java/org/assertj/core/groups/FieldsOrPropertiesExtractor.java:[50,22] Methode für extract(java.util.ArrayList<F>,java.util.function.Function<capture#5 von ? super F,T>) nicht geeignet
    Methode org.assertj.core.groups.FieldsOrPropertiesExtractor.<F,T>extract(F[],java.util.function.Function<? super F,T>) ist nicht anwendbar
      (Typvariable(n) F,T nicht ableitbar
        (nicht übereinstimmende Argumente; Keine Instanzen von Typvariablen T vorhanden, sodass java.util.ArrayList<T> F[] entspricht))
    Methode org.assertj.core.groups.FieldsOrPropertiesExtractor.<F,T>extract(java.lang.Iterable<? extends F>,java.util.function.Function<? super F,T>) ist nicht anwendbar
      (Inferenzvariable T hat inkompatible Gleichheits-Constraints T,F)
Error:  /Users/runner/work/assertj-core/assertj-core/src/main/java/org/assertj/core/util/introspection/PropertySupport.java:[102,34] Inkompatible Typen: Inferenzvariable R hat inkompatible Grenzwerte
    Gleichheits-Constraints: java.lang.Object,A,T,A,RR
    Untere Grenzwerte: java.util.List<T>,java.lang.Object
Error:  /Users/runner/work/assertj-core/assertj-core/src/main/java/org/assertj/core/util/introspection/FieldSupport.java:[147,34] Inkompatible Typen: Inferenzvariable R hat inkompatible Grenzwerte
    Gleichheits-Constraints: java.lang.Object,A,T,A,RR
    Untere Grenzwerte: java.util.List<T>,java.lang.Object
....
lot of similar compilation issues

@delabassee
Copy link

@vlsi To fill a ticket on https://bugs.openjdk.org, one of you needs to have the OpenJDK Author role.
If you don't, you should report it via https://bugreport.java.com/.
If you send me a small reproducer, I can also fill it on JBS on your behalf.

@vlsi
Copy link
Contributor

vlsi commented Jun 16, 2022

I've filed https://bugs.openjdk.org/browse/JDK-8288590 for tracking javac issue.

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.

None yet

4 participants