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

JPMS: missing jakarta dependencies #41203

Closed
xenoterracide opened this issue Jun 21, 2024 · 15 comments
Closed

JPMS: missing jakarta dependencies #41203

xenoterracide opened this issue Jun 21, 2024 · 15 comments
Labels
status: declined A suggestion or change that we don't feel we should currently apply

Comments

@xenoterracide
Copy link
Contributor

xenoterracide commented Jun 21, 2024

So I currently have code that doesn't directly use jakarta.transaction but a test is failing to start on missing it and modules for it and those modules for it. Some of these modules rely on modules I can't lock using the spring bom.

So this is at least a request to add those modules to the bom so I can match versions properly.

This is jakarta.transaction module info but jakarta.enterprise:jakarta.cdi-api is not available in the bom. jakarta.interceptor:jakarta.interceptor-api is also missing.

module jakarta.transaction {
    requires java.rmi;
    requires transitive java.transaction.xa;
    requires transitive jakarta.cdi;
    requires jakarta.interceptor;

    exports jakarta.transaction;
}

this is the maven pom from jakarta, which means these are expected to be provided by the consuming environment, and they are not marked optional. The version here is only for the consumer.

    <dependencies>
        <dependency>
            <groupId>jakarta.enterprise</groupId>
            <artifactId>jakarta.enterprise.cdi-api</artifactId>
            <version>3.0.1</version>
            <scope>provided</scope>
        </dependency>
        <dependency>
            <groupId>jakarta.interceptor</groupId>
            <artifactId>jakarta.interceptor-api</artifactId>
            <version>2.0.1</version>
            <scope>provided</scope>
        </dependency>
    </dependencies>

edit: done with additional details in ticket

repro.tar.gz

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Jun 21, 2024
@philwebb philwebb added the for: team-meeting An issue we'd like to discuss as a team to make progress label Jun 22, 2024
@wilkinsona
Copy link
Member

wilkinsona commented Jun 22, 2024

We have a long-standing policy of only managing the versions of dependencies upon which we depend directly. This limits the scope of spring-boot-dependencies to something that is manageable and has served us well for several years.

We don't declare a direct dependency on jakarta.enterprise:jakarta.cdi-api or jakarta.interceptor:jakarta.interceptor-api which is why they're not managed. I've also checked the code and we don't have any accidental usage of code in a jakarta.enterprise.* package or the jakarta.interceptor package where a direct dependency would then have been warranted.

I don't think we should change our policy to accommodate the JPMS. AIUI, the logical conclusion of such a policy change would be that we have to manage the versions of the entire transitive dependency graph. I don't think that would be sustainable and we should decline this.

In addition to not wanting the scope of our dependency management to be unmanageable, as far as I can tell this is only a problem due to the oddities in the pom and module info of the transaction API.

The pom for jakarta.transaction:jakarta.transaction-api declares dependencies on both jakarta.enterprise:jakarta.cdi-api and jakarta.interceptor:jakarta.interceptor-api but declares them as <scope>provided</scope>. This appears to be at odds with its module info which states that they're both required. It also marks the jakarta.cdi module as transitive meaning that it should be exposed to a module that only requires jakarta.transaction. This mismatch between the pom.xml and the module-info.java feels like a bug to me. It may be possible to work around the bug with a component metadata rule in Gradle that changes the dependency scope from provided to runtime, but I think it should ultimately be raised with the maintainers of Jakarta Transactions so that they can consider aligning the dependency information in the module info and the pom.

@xenoterracide
Copy link
Contributor Author

xenoterracide commented Jun 22, 2024

Possibly... I'm going to try to create a minimal reproducer on Monday.

This results in additional problems. To be honest I actually don't have any direct dependency on the Jakarta stuff at all. Which is why I'm going to create a reproducer. I speculate, but I'm not certain that there may be an incorrect dependency on jakarta.transaction... I'm going to speculate that the reason That they Don't provide it is that they want to be able to match multiple versions. It requires some version not a specific one.

If that is the rationale then I don't think Jakarta transaction should be in the starters which is I assume where I'm pulling it in from since I have no direct dependency on it. Either that or the starter should also contain the full dependencies for the module. If it truly is optional code then the Java module is wrong and those should be declared as static. Right now I only work here but this feels kind of messy and yes I should reach out to the Jakarta people I haven't had time yet.

Anyways let's talk more about this once I have the reproducer I hope.

@xenoterracide
Copy link
Contributor Author

repro.tar.gz

here's the reproducer. You'll note that I'm not using jakarta transaction directly and my module info doesn't depend on it. I've commented out the required fix in the build.gradle.kts

I don't think we should change our policy to accommodate the JPMS. AIUI, the logical conclusion of such a policy change would be that we have to manage the versions of the entire transitive dependency graph. I don't think that would be sustainable and we should decline this.

to be fair this module doesn't define it as optional, although it might be that way for the pom, there might be other reasons for that. Maven and Gradle aren't really great at distinguishing required but alternative dependencies, and optional dependencies respectively. I need to investigate that bit further. I don't think ignoring JPMS required but not static dependencies is a good precedence as it arguably breaks the jvms expectations and will break in the future if spring decides to add its own module-info.

now that I have the reproducer, I'm off to figure out why jakarta module seems to be different from it's maven pom. That's an explicit for this case though, not the general policy.

@xenoterracide
Copy link
Contributor Author

xenoterracide commented Jun 24, 2024

I found an existing issue jakartaee/transactions#214 There appears that at this time there are no plans to make this optional, but that the community can use the community process to request the module-info change.

It has been brought up on the CDI dev list that the EE 10 release of Transactions has a hard dependency on CDI. This is due to the jakarta.transaction.Transactional and jakarta.transaction.TransactionScoped annotations. In the past it was possible to support Transactions without including CDI if a user did not make use of those annotations.

My understanding of maven is that provided mean required by the consuming environment at runtime, but that will not depend on a specific version. Without the optional flag it doesn't mean optional. So the starter providing it without providing the required dependencies seems highly inappropriate since it's kind of spring boot's starter "promise" that it'll give you everything you "need". I shouldn't need to provide a dependency for a dependency that I don't have any need of.

I do see correct 2 paths on this specific issue based on that ticket without a response and update to the pom to my comment there

  1. remove jakarta transaction from the starter in a future minor version of spring boot (not patch/bug). I *think? it's possible to update HibernateJpaConfiguration to make this behave better if it's missing? because it's only needed if you're using JTA? maybe it's not?
  2. add these dependencies because they are required to be provided at runtime at this time and make the request that they not be in a future version. This should/can be done as a bug fix.

If there's any more leg work I can do, let me know. I would ask that you not make a general policy of ignoring module-info requirements.

@philwebb philwebb removed status: waiting-for-triage An issue we've not yet triaged for: team-meeting An issue we'd like to discuss as a team to make progress labels Jul 4, 2024
@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Jul 4, 2024
@philwebb
Copy link
Member

philwebb commented Jul 4, 2024

We discussed this today and we feel that this is a bug with the module-info in jakarta.transaction:jakarta.transaction-api.

@philwebb philwebb closed this as not planned Won't fix, can't repro, duplicate, stale Jul 4, 2024
@philwebb philwebb added status: declined A suggestion or change that we don't feel we should currently apply and removed status: waiting-for-triage An issue we've not yet triaged labels Jul 4, 2024
@xenoterracide
Copy link
Contributor Author

xenoterracide commented Jul 4, 2024

Then please file a request for change the specification with Jakarta. It's their standard and they are very clearly stating that they will not be making a change to the standard in this version and that a request for change via the community process would be required to make such a change

If you're not going to conform to the standards then you should remove the dependency from the starter

@xenoterracide
Copy link
Contributor Author

I guess I'd love to know why you think it's a mistake on their end when they're saying it's not jakartaee/transactions#214 (comment)

@philwebb
Copy link
Member

philwebb commented Jul 5, 2024

I don't think there's much to say beyond the information Andy has already provided. We thought it was a bug since things appear work just fine without jakarta.enterprise:jakarta.cdi-api and jakarta.interceptor:jakarta.interceptor-api if you're not using JPMS.

Unfortunately we need to prioritize the little time we have on the issues that have the most impact. If you're going to choose to use the JPMS then I'm afraid you're going to need to take on the extra burden of managing a few more dependencies yourself.

@arjantijms
Copy link

arjantijms commented Jul 7, 2024

We thought it was a bug since things appear work just fine without

[partially copied from the other thread]
t's not really a bug. The module-info follows the spec, where the features that happen to use CDI and Interceptors are simply listed among all other features as mentioned in jakartaee/transactions#214 (comment)

But you could think of it as a design-bug in the spec, and we probably should have put the features that required CDI and Interceptors in a separate module. As you probably know, Spring is often a concern of big importance for a number of Jakarta specs (e.g. Servlet and Persistence), ahead of the concerns of the Jakarta platform itself. Transactions also should have been thinking about Spring first, but it wasn't detected since JPMS wasn't out then.

@philwebb out of curiousity how Spring is designed; do you also put everything that requires Spring Beans in a separate module, and make sure the features that don't require Spring Beans are usable with say CDI etc without using a Spring Beans dependency?

@philwebb
Copy link
Member

philwebb commented Jul 8, 2024

Hi @arjantijms,

Currently the extent of JPMS support is setting Automatic-Module-Name in the MANIFEST.MF file so we haven't really had to deal with the type of problem you're facing. The majority of our users are still running pretty classic setups so we generally declare optional dependencies and will fail if a user actually tries to use that code without the appropriate jar.

There's an open Spring Framework issue that has been looking into this (spring-projects/spring-framework#18079), but it's not got very far and given this comment by @jhoeller may not get traction anytime soon.

I suspect if we even did add complete module-info then we'd have a lot of optional declarations.

@xenoterracide
Copy link
Contributor Author

But you could think of it as a design-bug in the spec, and we probably should have put the features that required CDI and Interceptors in a separate module.

personally I think a single module is fine, but the pom should define these as optional dependencies (It doesn't and spring boot is choosing to ignore that, and instead supplying incomplete dependencies). Also, then the module-info should define them as static (optional). I think additionally though Jakarta EE should supply a maven BOM, so that spring can use that and versions will be supplied, which would also solve this headache.

@xenoterracide
Copy link
Contributor Author

@philwebb @wilkinsona I'm kind of making an assumption here; it's based on the fact that I didn't get versions when trying to add these myself. Is there a reason that Spring Boot doesn't use the Jakarta EE BOM? which would provide the necessary versions even if you don't allow substitution.

@philwebb
Copy link
Member

philwebb commented Aug 1, 2024

@xenoterracide I think the main reasons are:

  • it provides more than we really want to provide (e.g. we don't have any support for JSF in Spring Boot)
  • It's not always kept up to date (e.g. we use JSON bind 3.0.1, but the Jakarta EE 10 branch for the BOM still has 3.0.0).

@xenoterracide
Copy link
Contributor Author

Right, but you can force upgrade a dependency? Shouldn't you be asking them to do an upgrade?

As far as the first point goes. What is the negative for having a version of something that you don't otherwise intend to use? Like a starter doesn't have to contain that dependency. You don't have to add any additional support for it yourself aside from the fact that it's in the list and if someone were to add it for whatever reason they would already have a version.

@philwebb
Copy link
Member

philwebb commented Aug 2, 2024

Our current process is working quite well for the majority of our users and we don't really have the bandwidth to switch to the Jakarta BOM then immediately start to override versions or submit requests to them to do so. Dependency management is hard, and we really don't want to start including more than we need in our own BOM.

I would suggest using the Jakarta BOM directly in your own project if it solves your problem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: declined A suggestion or change that we don't feel we should currently apply
Projects
None yet
Development

No branches or pull requests

5 participants