chore: bump-okhttp-4.12.0-to-5.0.0 #13521
Conversation
build-logic/src/main/kotlin/org.jabref.gradle.base.dependency-rules.gradle.kts
Outdated
Show resolved
Hide resolved
| module("com.squareup.okhttp3:okhttp", "okhttp3") | ||
| module("com.squareup.okhttp3:okhttp-sse", "okhttp3.sse") | ||
|
|
||
| module("com.squareup.okhttp3:okhttp-jvm", "okhttp3") |
There was a problem hiding this comment.
I think, now two modules have the same name - see line 149.
There was a problem hiding this comment.
I understand your concern. Even though two modules are assigned the same name here, there is no actual conflict because they refer to completely separate artifacts (okhttp vs. okhttp-jvm).
This duplication is currently necessary because some dependencies (such as langchain4j-hugging-face) still require the legacy okhttp artifact, while our project relies on the newer okhttp-jvm from OkHttp 5.x.
Since the Java Module System (JPMS) treats them as distinct jars, and as long as both modules are not loaded into the same named module path, this setup works without any issues.
There was a problem hiding this comment.
If there's anything wrong or anything I should fix in what I said, please feel free to let me know :)
There was a problem hiding this comment.
My gut feeling says that overloaded module names will cause issues. Especially when we will switch to use modules as the primary source of dependencies: https://github.com/gradlex-org/java-module-dependencies
There was a problem hiding this comment.
Overloaded module names could definitely cause issues, especially if we move towards using modules as the primary way of managing dependencies in the future.
I’ll try to look into this and hopefully resolve it by tomorrow or the day after. Please feel free to let me know if there’s anything I missed or should improve!
There was a problem hiding this comment.
I checked the dependency tree with the dependencies task, and you're right.
Gradle resolves okhttp to version 5.0.0 automatically because of other dependencies or version conflicts.
However, since langchain4j-hugging-face still relies on the 4.x API, upgrading to 5.0.0 breaks compatibility.
There was a problem hiding this comment.
Does a separte module name help to avoid module name overloading?
The modules will be on the same path - as JabRef needs all of them.
There was a problem hiding this comment.
In the current situation, module name conflicts are unavoidable, and using separate module names alone does not fundamentally resolve the issue.
There was a problem hiding this comment.
com.squareup.okhttp3:okhttp is not really a module anymore. The Jar is more or less empty now. This is Kotlin multi-platform stuff.
I think it does not end up the Module Path, but is needed by the mechanism that looks at the metadata to create the missing module-info files. That the checks are passing on this PR indicates that everything is ok.
I know that it looks like the name is used twice, but in practice only one of the Jars is used. It's confusing but its Kotlin Multi-Platform libraries "clashing" with JPMS. Maybe add a comment about this to the module("com.squareup.okhttp3:okhttp", "okhttp3") line.
We have a similar situation here with kotlinpoet:
There was a problem hiding this comment.
The name okhttp3 is correct because the MANIFEST of com.squareup.okhttp3:okhttp-jvm contains
Automatic-Module-Name: okhttp3
…rules.gradle.kts Co-authored-by: Oliver Kopp <[email protected]>
|
@trag-bot didn't find any issues in the code! ✅✨ |
* upstream/main: Also label PR if good first issue is made (#13526) New Crowdin updates (#13529) chore(deps): update dependency org.apache.logging.log4j:log4j-to-slf4j to v2.25.1 (#13528) chore: bump-okhttp-4.12.0-to-5.0.0 (#13521) Have the picker always on top (#13525) Refactor PushToApplications and split into logic and GUI (#13514) Add field to change HTTP port (#13479) Fix trigger of comment Use Java 11 isBlank (#13523) Add run openrewrite (#13524) Comment ion PR just opened (#13522) Improve merge logic to prefer valid year and entry type (#13506) Update dependency com.konghq:unirest-modules-gson to v4.4.12 (#13517) Add rpm target (#13516) Revert module name changes for remaining 'unnamed' Jars (#13515) fix: revert Java module names to restore Status Log compatibility in JabRef 5.15 (#13511) update java vendor in devcontainer and sdkmanrc (#13513)
3 participants
Closes #13509
Steps to test
Mandatory checks
CHANGELOG.mddescribed in a way that is understandable for the average user (if change is visible to the user)