search

jakartaee / transactions

Welcome to the Jakarta EE Transactions API Project (formerly JTA)
https://jakartaee.github.io/transactions/
Other
29 stars 30 forks source link

Fix javadoc build with JDK11 #198

Closed pzygielo closed 2 years ago

jta-bot commented 2 years ago

Can one of the admins verify this patch?

pzygielo commented 2 years ago

But this change is not needed for JDK 15+

tomjenkinson commented 2 years ago

ok to test

tomjenkinson commented 2 years ago

I need to find something that would cite that we explicitly support JDK 11 in EE10. Do you have a reference you can share? But I can also take a look for that reference myself too.

Thanks!

pzygielo commented 2 years ago

This change does not require JDK11, but enables its usage. And it works fine with JDK8.

pzygielo commented 2 years ago

Using newer JDK would still produce 1.8 classes: https://github.com/eclipse-ee4j/jta-api/blob/7de86ad2c05276e034b0ede18ad8201586806f00/api/pom.xml#L37-L38

tomjenkinson commented 2 years ago

Sorry, I rather meant that there is some actual requirement to support JDK 11 in Jakarta EE 10. In theory otherwise we would not have a precedent to make a change.

tomjenkinson commented 2 years ago

Here we go I think: https://jakarta.ee/specifications/platform/9.1/jakarta-platform-spec-9.1.html#changes-in-jakarta-ee-9-1 and https://jakarta.ee/specifications/platform/9.1/jakarta-platform-spec-9.1.html#editorial-changes

tomjenkinson commented 2 years ago

IIUC then a build would fail with JDK 11 as it stands. I will configure CI to run with JDK 11 for the master branch.

tomjenkinson commented 2 years ago

I have done that, so if I followed correctly so far then I think https://ci.eclipse.org/jta/job/eclipse-ee4j_jta-api-openjdk-jdk11-latest/1/ will fail to build javadoc due to a <br>in api/src/main/java/jakarta/transaction/package.html until we can merge this PR

tomjenkinson commented 2 years ago

That looks to be the case. I do note some warnings too. I am going to try to set up PR testing for JDK 11 to verify the proposed change.

tomjenkinson commented 2 years ago

I might have been able to set this up I think (https://ci.eclipse.org/jta/job/eclipse-ee4j_jta-api-pulls-openjdk-11-latest/)

tomjenkinson commented 2 years ago

I think the output:


[WARNING] /home/jenkins/agent/workspace/eclipse-ee4j_jta-api-pulls-openjdk-11-latest/api/src/main/java/jakarta/transaction/package.html:34: warning: empty <p> tag
[WARNING] <p>
[WARNING] ^
[WARNING] /home/jenkins/agent/workspace/eclipse-ee4j_jta-api-pulls-openjdk-11-latest/api/src/main/java/jakarta/transaction/Transactional.java:68: warning: no @return
[WARNING] TxType value() default TxType.REQUIRED;
[WARNING] ^```
Should indicate the change for <br/> has allowed the build to proceed.
pzygielo commented 2 years ago

Thanks for checking and updating CI job. https://ci.eclipse.org/jta/job/eclipse-ee4j_jta-api-pulls-openjdk-11-latest/1/console:

[INFO] ------------------------------------------------------------------------

+ cd /home/jenkins/agent/workspace/eclipse-ee4j_jta-api-pulls-openjdk-11-latest
+ mv glassfish/appserver/distributions/glassfish/target/glassfish.zip .
+ rm -rf glassfish

+ unzip -q glassfish.zip
+ ls glassfish6/glassfish/modules/
ls: cannot access 'glassfish6/glassfish/modules/': No such file or directory

It looks that current GF master which produces glassfish-7.0.0-SNAPSHOT-nightly.zip (https://download.eclipse.org/ee4j/glassfish/) has modules in glassfish7/glassfish/modules directory now.

tomjenkinson commented 2 years ago

Thank you, for the heads up. I am not sure why but the javadocs were not archived but I made a change to the config there and to the glassfish as you mentioned.

I am going to retest this. Even if the glassfish part fails we could merge this but it would be good to get the PR job to have the javadocs for review.

tomjenkinson commented 2 years ago

retest this please

tomjenkinson commented 2 years ago

the build failed downloading the staged TCK (IIRC), I have updated the link

tomjenkinson commented 2 years ago

retest this please

pzygielo commented 2 years ago

Strange, build uses JDK 8 again:

[eclipse-ee4j_jta-api-pulls] $ /opt/tools/java/oracle/jdk-8/

tomjenkinson commented 2 years ago

Thank you for mentioning that - I have two jobs set up with the pull request configuration now so maybe it could be some quirk of that. I will try to trigger the JDK 11 one by hand and then maybe set up a matrix job instead with the two JDKs instead of the two separate ones.

tomjenkinson commented 2 years ago

In this case btw though I am not sure where you are getting the output you quoted from? Here is the most recent run and I think with JDK 11: https://ci.eclipse.org/jta/job/eclipse-ee4j_jta-api-pulls-openjdk-11-latest/9/consoleFull

tomjenkinson commented 2 years ago

retest this please

tomjenkinson commented 2 years ago

Most recent failure I hope I have corrected, there was a few paths hardcoded to /home/jenkins/agent/workspace/eclipse-ee4j_jta-api-pulls and I think should/could be ${WORKSPACE}

pzygielo commented 2 years ago

In this case btw though I am not sure where you are getting the output you quoted from? Here is the most recent run and I think with JDK 11: https://ci.eclipse.org/jta/job/eclipse-ee4j_jta-api-pulls-openjdk-11-latest/9/consoleFull

Fron this run: https://ci.eclipse.org/jta/job/eclipse-ee4j_jta-api-pulls/278/ that was linked as "Details" in sumary for "default" job in this PR.

pzygielo commented 2 years ago

And now https://ci.eclipse.org/jta/job/eclipse-ee4j_jta-api-pulls/279/ is linked as failure.

tomjenkinson commented 2 years ago

I guess the default gets replaced by the two completing jobs as they finish. I might be able to choose a different word for those which might help. BTW eclipse-ee4j_jta-api-pulls is the JDK 8 version of the job (which I would keep) but I am going to change it's name to be clearer

tomjenkinson commented 2 years ago

BTW, I don't expect [ERROR] Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:3.8.1:compile (default-compile) on project tiger-types: Fatal error compiling: invalid target release: 11 -> [Help 1] is caused by this PR - I need to look into that. I do think that "default" is getting replaced by the last job. If you would like to check on just the JDK 11 version of your job you could look into https://ci.eclipse.org/jta/job/eclipse-ee4j_jta-api-pulls-openjdk-11-latest/ for those starting PR #198 I expect.

pzygielo commented 2 years ago

Just a note - GF introduced profile fastest, I think, with checkstyle, tests etc. disabled. So if this job just needs current glassfish only to proceed with TCK - perhaps its activation in mvn call for GF could be considered?

pzygielo commented 2 years ago

BTW, I don't expect [ERROR] Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:3.8.1:compile (default-compile) on project tiger-types: Fatal error compiling: invalid target release: 11 -> [Help 1] is caused by this PR - I need to look into that.

It's in GlassFish build, not jta-api. It's caused by the fact that javac from jdk8 does not support --release flag that is set in GF.

I do think that "default" is getting replaced by the last job.

I also think that both of them report status for "default" label. Sadly, :red_square: https://ci.eclipse.org/jta/job/eclipse-ee4j_jta-api-pulls-openjdk-11-latest/10/

tomjenkinson commented 2 years ago

I will disable the testing of TCK on JDK8 for the master branch because the GlassFish EE 9.1+ implementation requires JDK11: https://github.com/eclipse-ee4j/glassfish/blob/master/README.md

    Eclipse GlassFish 7.0.0 is Jakarta EE 10 compatible, requires Java 11, supports Java 18
    Eclipse GlassFish 6.2.0 is Jakarta EE 9.1 compatible, requires Java 11, supports Java 17
    Eclipse GlassFish 6.1.0 is Jakarta EE 9.1 compatible, requires Java 11
    Eclipse GlassFish 6.0.0 is Jakarta EE 9 compatible, requires Java 8
tomjenkinson commented 2 years ago

I have also temporarily disabled https://ci.eclipse.org/jta/job/eclipse-ee4j_jta-api-pulls-oracle-jdk8-latest/ so that the jdk11 results will reflect in the PR

tomjenkinson commented 2 years ago

retest this please

tomjenkinson commented 2 years ago

I hope to have updated the job to get some more files, including the logs from glassfish

tomjenkinson commented 2 years ago

The logs are there now, something to do with missing RemoteStatus (https://ci.eclipse.org/jta/job/eclipse-ee4j_jta-api-pulls-openjdk-11-latest/11/artifact/glassfish7/glassfish/domains/domain1/logs/server.log) - I wonder if the classpath is not quite right somewhere. I will look at the jte

tomjenkinson commented 2 years ago

Maybe something has changed in how the deployment works. I tried to diff a bit against a previous run on JDK8 that did work (https://ci.eclipse.org/jta/job/eclipse-ee4j_jta-api-pulls-oracle-jdk8-latest/267/consoleFull) with the JDK 11 run that failed (https://ci.eclipse.org/jta/job/eclipse-ee4j_jta-api-pulls-openjdk-11-latest/11/console) but it's not jumping out what the problem is