Make the CDI/interceptor dependencies optional in module-info

[yrodiere]

Fixes #214

The "static" keyword means "required for compilation of this module, but not at runtime", which is pretty much the current situation.

Those dependencies are only required for the @Transactional and @TransactionScoped annotations, but jakarta.transaction-api can perfectly well be used without CDI, e.g. in Hibernate ORM in a Java SE environment.

Without this change, it's impossible to use jakarta.transaction-api (or anything relying on it, e.g. Hibernate ORM) without CDI in the modulepath.

[jta-bot]

Can one of the admins verify this patch?

[yrodiere]

FWIW I'm a Red Hat employee and thus covered by Red Hat's Eclipse Contributor Agreement, and that's visible on my Eclipse profile, so I don't know why the automated check on GitHub fails.

EDIT: The problem was a trailing space in the "First name" field in my Eclipse profile :facepalm:

[tomjenkinson]

Is the intention for this to be a service release of the API jar for EE10 or is would it be required to wait for EE11?

[yrodiere]

Is the intention for this to be a service release of the API jar for EE10 or is would it be required to wait for EE11?

From my point of view this is essentially a bugfix. As far as I'm concerned, this would need to be included in a bugfix release in EE10 (e.g. jakarta.transaction-api 2.0.2).

In my opinion there's also no need to wait for EE11, because this change shouldn't break any application. As I understand it, users either are not using CDI and then removing the dependency won't affect them negatively, or they are using CDI and then all the related modules are already in their modulepath and will continue to be exposed to users transitively through this module-info.

However, I am not familiar with the Jakarta release process at all. Maybe it's too late for such change in EE10? You probably know better than I do.

[tomjenkinson]

Thank you for the clarification

[tomjenkinson]

ok to test

[yrodiere]

Hey. It looks like the build got stuck or was aborted for some reason... can anyone restart it?

[tomjenkinson]

retest this please

[yrodiere]

The testing infra seems to be having problems:

[ERROR] Error executing Maven. [ERROR] The specified user settings file does not exist: /home/jenkins/agent/workspace/eclipse-ee4j_jta-api-pulls-openjdk-jdk11-latest/glassfish/snapshots/settings.xml Build step 'Execute shell' marked build as failure

[tomjenkinson]

retest this please

[tomjenkinson]

I have commented out a line of the build config mvn -s ./snapshots/settings.xml clean install -f ./snapshots/pom.xml - snapshots/ folder is not in the current glassfish master branch.

[yrodiere]

[ERROR] Rule 0: org.apache.maven.plugins.enforcer.RequireMavenVersion failed with message:
Detected Maven Version: 3.8.5 is not in the allowed range 3.8.6.

:)

[tomjenkinson]

retest this please

[tomjenkinson]

Thanks, the job was configured to use apache-maven-latest but maybe this is not updated yet to point to a version that would satisfy the check. I found apache-maven-3.8.6 in the list though and so have configured it to be that

[yrodiere]

Thank you for working on this :)

Next one...

[ERROR] Failed to execute goal org.apache.maven.plugins:maven-antrun-plugin:3.1.0:run (do stuff) on project glassfish: An Ant BuildException has occured: The following error occurred while executing this line:
[ERROR] /home/jenkins/agent/workspace/eclipse-ee4j_jta-api-pulls-openjdk-jdk11-latest/glassfish/appserver/distributions/glassfish/target/antrun/build-main.xml:9: The archive microprofile-jwt-auth-api.jar doesn't exist
[ERROR] around Ant part ...<jarupdate basedir="/home/jenkins/agent/workspace/eclipse-ee4j_jta-api-pulls-openjdk-jdk11-latest/glassfish/appserver/distributions/glassfish/src/main/patches/microprofile-jwt-auth-api" destfile="/home/jenkins/agent/workspace/eclipse-ee4j_jta-api-pulls-openjdk-jdk11-latest/glassfish/appserver/distributions/glassfish/target/stage/glassfish7/glassfish/modules/microprofile-jwt-auth-api.jar" includes="META-INF/MANIFEST.MF" />... @ 17:430 in /home/jenkins/agent/workspace/eclipse-ee4j_jta-api-pulls-openjdk-jdk11-latest/glassfish/appserver/distributions/glassfish/target/antrun/build-main.xml

[tomjenkinson]

You are welcome :)

I wonder if Glassfish is building fine on their main branch now. I will try to have a look.

The reason we use Glassfish is to run the CTS with changes to make sure nothing is unexpectedly breaking.

[tomjenkinson]

I have a feeling this might affect the current master branch of Glassfish. The most recent weekly build did pass (https://ci.eclipse.org/glassfish/view/GlassFish/job/glassfish_weekly_build/171/) but there are changes in Glassfish since 93176e2555176091c8522e43d1d32a0a30652d4a

[tomjenkinson]

I am not sure why those approvals were just provided from those accounts. I don't think they are committers though: https://projects.eclipse.org/projects/ee4j.jta/who

[NielsBerghenEveresst]

I am not sure why those approvals were just provided from those accounts. I don't think they are committers though: https://projects.eclipse.org/projects/ee4j.jta/who

Sorry Tom. We approved it because our team/project ran into exactly the same issue. Having these requires as static would solve these issues for us. We just upgraded our project from Spring boot 2 > 3 (which requires an javax > jakarta upgrade), but due to that upgrade, we were required to add a dependency on jakarta cdi as well (due to reason Yoann posted here).

We would've prefered a thumbs up / vote as it seemed this PR was a bit forgotten, but that wasn't possible. So we wanted to raise some awareness of this PR again. Although we're not familiar with the Jakarta release process, so this might still have been on the radar though.

[tomjenkinson]

Ah, great - thank you for clarifying!

[yrodiere]

Hello @tomjenkinson! Is there something I can help with to get this merged?

[yrodiere]

Hey @tomjenkinson , is there any chance to get this merged?

[tomjenkinson]

retest this please

[tomjenkinson]

@yrodiere I have requested CI to retest this, thanks

[yrodiere]

Looks like it's failing due to some Maven incompatibility:

+ mvn -Pstaging clean install -Djakarta.transaction-api.version=2.0.2-SNAPSHOT -DskipTests
[INFO] Scanning for projects...
[WARNING] Error injecting: org.glassfish.build.GlassFishJarLifecycle
com.google.inject.ProvisionException: Unable to provision, see the following errors:

1) Error injecting constructor, java.lang.NoSuchMethodError: org.apache.maven.lifecycle.mapping.DefaultLifecycleMapping.<init>(Ljava/util/List;)V
  at org.glassfish.build.GlassFishJarLifecycle.<init>(Unknown Source)
  while locating org.glassfish.build.GlassFishJarLifecycle

1 error
    at com.google.inject.internal.InternalProvisionException.toProvisionException (InternalProvisionException.java:226)

FWIW Glassfish's main branch requires Maven 3.9: https://github.com/eclipse-ee4j/glassfish/blob/c6ee22af21e2578f745a516a251c0c662124b178/pom.xml#L78-L90

[tomjenkinson]

Thank you, I have changed the configuration to use something that looked like it should expect to be latest. Will retry the build.

[tomjenkinson]

retest this please

[tomjenkinson]

retest this please

[tomjenkinson]

There was something failing in JWT. I found a SHA in CI of something I hope to pass. It might need JDK upgrade though to 17 as the job is currently using JDK 11.

[tomjenkinson]

I am trying a build locally. I tried a few versions of a build earlier but couldn't get the build to fail for a missing microprofile-jwt-auth-api.jar but I am using the same SHA now. Also with 78a3424568a59097da7c6479c74cb7ccc42a749e it does seems to be at least getting the build started with JDK 11

[tomjenkinson]

Ah, 78a3424568a59097da7c6479c74cb7ccc42a749e (Glassfish) has just failed for me locally with JDK 11

[tomjenkinson]

Trying locally with JDK 17, will see if that works

[tomjenkinson]

JDK 17 build worked locally but I think Glassfish is expecting to be able to build with JDK 11

[tomjenkinson]

I think there is something not working with the Glassfish build and JDK 11. I think that with 7.0.12 MVN_EXTRA=-DskipTests ./gfbuild.sh dev_build is not working and I think with JDK 17 it will.

[yrodiere]

Alright... does that mean we should ask the help from someone working on Glassfish?

Alternatively, maybe the master branch should use an older version of Glassfish for its builds? IMO this would make sense as long as this repo is not trying to build something for a new Jakarta EE release. After all, we currently want the code on the master branch to still work on JDK 11, no?

And it would make sense to not tie the specific change in this PR to a newer Jakarta EE release, since it's essentially a hotfix for something related to packaging that is not covered by the spec itself.

[tomjenkinson]

I did raise the problem on glassfish-dev on Thursday (from the timestamp on https://www.eclipse.org/lists/glassfish-dev/msg01541.html).

I think the version should be a 7 version as that is EE10 (https://github.com/eclipse-ee4j/glassfish/blob/master/README.md) - do you agree? If so I think this is probably still their master branch as I don't think I spot a different one we can use: https://github.com/eclipse-ee4j/glassfish/branches.

[yrodiere]

I did raise the problem on glassfish-dev on Thursday (from the timestamp on https://www.eclipse.org/lists/glassfish-dev/msg01541.html).

Great, thanks. Let's wait for answers then.

I think the version should be a 7 version as that is EE10 (https://github.com/eclipse-ee4j/glassfish/blob/master/README.md) - do you agree? If so I think this is probably still their master branch as I don't think I spot a different one we can use: https://github.com/eclipse-ee4j/glassfish/branches.

I'm not very familiar with Glassfish but from what I can see, you're right.

[tomjenkinson]

I have raised an issue on the Glassfish repo: https://github.com/eclipse-ee4j/glassfish/issues/24794

[tomjenkinson]

I have made a job change to create a patch to get GlassFish to build with JDK 11 and to apply it

[tomjenkinson]

retest this please

[tomjenkinson]

The good news is that I have seen a build that is running the TCK now, but the bad news is there are a number of failures (https://ci.eclipse.org/jta/view/Enabled/job/eclipse-ee4j_jta-api-pulls-openjdk-jdk11-latest/67/console). I have retriggered the run and thus a build will be hooked into the GitHub checks. I think it may well be not set up correctly yet still because it's consistently the "from_standalone" tests (exceptionally for the signature test which does pass in the "from_standalone" variant).

[tomjenkinson]

I think it is probably something to do with "Caused by: java.lang.ClassNotFoundException: org.glassfish.main.jul.GlassFishLogger"...

[tomjenkinson]

This class was added in GlassFish 7 (https://github.com/eclipse-ee4j/glassfish/commit/b68a6a8698ca8a533004662b6d71c912df63f626) but it looks like we used GlassFish 6 for EE 9 (https://jakarta.ee/specifications/transactions/2.0/). Perhaps something will need configuring to expose this Jar

[tomjenkinson]

retest this please

[tomjenkinson]

I am trying with to see if I can add ${webServerHome}/lib/bootstrap/glassfish-jul-extension.jar${pathsep} into something that hopefully controls a relevant classpath for the 2.0.2 TCK. If it works that seems OK for now but we would want a change in to get it into future TCKs (so master version of https://github.com/jakartaee/platform-tck/blob/aa2b698/install/jta/bin/ts.jte#L68)

[yrodiere]

Well I certainly didn't expect this PR to cause you so much trouble :] Thanks for staying on top of this, and let me know if I can help.

[tomjenkinson]

Even though we are closing in on getting the CI issue resolved, @arjantijms has raised a good point in https://github.com/jakartaee/transactions/issues/214 about the appropriateness of doing this in an update. @yrodiere please can I ask you to provide input on that thread too?

[tomjenkinson]

retest this please

[tomjenkinson]

Great, it passes CI - thank you :)

[ljnelson]

Input on this issue was solicited on the jta-dev email list. I have no stake at all in this issue one way or the other. My input is: see https://mail.openjdk.org/pipermail/jigsaw-dev/2023-April/014850.html (and following, and preceding), where the intent of requires static is discussed. Short version: it does not mean an optional dependency, apparently, but many users wish it would.