Reduce complexity in dependencies setup (restore)#15194
Conversation
Siedlerchr
added
the
dev: binaries
This comment has been minimized.
This comment has been minimized.
|
To address #15169 (comment) I did some more manually testing in the GUI. There are some code paths not covered by tests that we missed for which Before this is merged again, I would appreciate one or two more people testing this manually. If some more issues like these are discovered, the fix is to add the opens that corresponds to what the error message says in |
|
Thanks for the follow up, There now seems to be something not correct with the fetcher tests, I am worried about the NPE there and at least arxiv should normally be working |
|
@Siedlerchr The info I got from @koppor is that these tests are unreliable and can be ignored. Which is of course in general not a good situation (but unrelated to the topic of this PR). The same failures appeared in the original PR. But I am happy to look at the problem, if you can point out which test exactly should pass that is failing now. |
Locally, it works: 
Thus, treating it as "hick-up" :) |
|
Launched using |
Review Summary by QodoRestore and refactor module dependencies with proper scopes and validation
WalkthroughsDescription• Restore and refactor module dependency declarations with proper scopes • Mark runtime-only dependencies with /*runtime*/ comments in module-info • Add missing opens statements and fix transitive dependency declarations • Simplify build.gradle files by removing explicit dependency declarations • Add new Gradle plugin for dependency scope validation and JavaFX metadata rules Diagramflowchart LR
A["Module-Info Files"] -->|Add runtime markers| B["Runtime Dependencies"]
A -->|Fix transitive| C["Transitive Dependencies"]
A -->|Add opens| D["Package Exports"]
E["Build Gradle Files"] -->|Remove duplicates| F["Simplified Config"]
G["New Gradle Rules"] -->|Validate scopes| H["Dependency Checker"]
G -->|Handle JavaFX| I["Platform Variants"]
File Changes1. jabgui/src/main/java/module-info.java
|
Code Review by Qodo
1.
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
| plugins { | ||
| id("com.autonomousapps.dependency-analysis") | ||
| } |
There was a problem hiding this comment.
2. New dependency-analysis plugin added 📘 Rule violation ⛯ Reliability
The PR introduces the com.autonomousapps.dependency-analysis Gradle plugin, which is a new dependency/tooling addition. Adding new dependencies without clear necessity/justification violates the dependency-minimization requirement.
Agent Prompt
## Issue description
A new Gradle plugin dependency (`com.autonomousapps.dependency-analysis`) was added, which violates the rule to avoid new dependencies without clear justification.
## Issue Context
This was introduced as a new build-logic plugin script.
## Fix Focus Areas
- build-logic/src/main/kotlin/org.jabref.gradle.check.dependencies.gradle.kts[1-3]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
koppor
added
the
status: ready-for-review
|
did you run the tests via gradle? |
|
Locally: 
Seems to be different. |
github-actions
bot
added
status: changes-required
|
Your pull request conflicts with the target branch. Please merge with your code. For a step-by-step guide to resolve merge conflicts, see https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/addressing-merge-conflicts/resolving-a-merge-conflict-using-the-command-line. |
This comment has been minimized.
This comment has been minimized.
jjohannes
force-pushed
the
reduce-complexity-in-dependencies-setup
branch
from
29b4850 to
f94052a
Compare
This comment has been minimized.
This comment has been minimized.
🚨 TestLens detected 46 failed tests 🚨Here is what you can do:
Test Summary
🏷️ Commit: 6858c69 Test Failures (first 5 of 46)ArXivFetcherTest > abstractIsCleanedUp() (:jablib:fetcherTest in Fetcher Tests / Fetcher tests)ArXivFetcherTest > findFullTextByDOI() (:jablib:fetcherTest in Fetcher Tests / Fetcher tests)
ArXivFetcherTest > findFullTextByTitle() (:jablib:fetcherTest in Fetcher Tests / Fetcher tests)
ArXivFetcherTest > findFullTextByTitleWithCurlyBracket() (:jablib:fetcherTest in Fetcher Tests / Fetcher tests)
ArXivFetcherTest > findFullTextByTitleWithCurlyBracketAndPartOfAuthor() (:jablib:fetcherTest in Fetcher Tests / Fetcher tests)
Muted TestsSelect tests to mute in this pull request:
Reuse successful test results:
Click the checkbox to trigger a rerun:
Learn more about TestLens at testlens.app. |
|
The build of this PR is available at https://builds.jabref.org/pull-15194/. |
koppor
left a comment
koppor
left a comment
There was a problem hiding this comment.
- Fetcher tests: I checked arXiv - runs locally. So some strange CI behavior
- JabKitLauncher: We need to check as soon as jablib-SNAPSHOT is on maven central.
| io.github.darvil.terminal.textformatter=io.github.darvil82:terminal-text-formatter | ||
| io.github.eadr=io.github.adr:e-adr | ||
| jul.to.slf4j=org.slf4j:jul-to-slf4j | ||
| mslinks=com.github.vatbub:mslinks |
There was a problem hiding this comment.
Future: org.jabref:mslinks - can I simply replace this or do I need to take care more?
There was a problem hiding this comment.
If you want to change the coordinates for mslink you need to change it here and here for the version:
jabref/versions/build.gradle.kts
Line 76 in 3977190
Explanation: For the version, we use the coordinates and not the module name (which we could, the plugin supports that) so that Dependabot can understand the versions/build.gradle.kts file. Hence the two places.
koppor
removed
the
status: changes-required
|
@jjohannes I would need some small howto how to add a new dependency. I was used to |
|
@koppor good point. I have something here that I can reusr and adjust for JabRef I believe. Where would be a good place for this? |
Nice!
https://github.com/JabRef/jabref/tree/main/docs/code-howtos --> new file Proposal: ---
parent: Code Howtos
---
# Dependency management
## Adding a dependency |
|
Proposal for docs: #15278 |
…rg.openrewrite.recipe-rewrite-recipe-bom-3.25.0 * upstream/main: (35 commits) Chore: add dependency-management.md (#15278) Chore(deps): Bump dev.langchain4j:langchain4j-bom in /versions (#15277) New Crowdin updates (#15274) Chore(deps): Bump actions/upload-artifact from 6 to 7 (#15271) Chore(deps): Bump actions/download-artifact from 7 to 8 (#15270) Chore(deps): Bump docker/login-action from 3 to 4 (#15268) Fix threading issues in citations relations tab (#15233) Fix: Citavi XML importer now preserves citation keys (#14658) (#15257) Preserve no break spaces in Latex to Unicode conversion (#15174) Fix: open javafx.scene.control.skin to controlsfx (#15260) Reduce complexity in dependencies setup (restore) (#15194) New translations jabref_en.properties (French) (#15256) Fix: exception dialog shows up when moving sidepanel down/up (#15248) Implement reset for Name Display Preferences (#15136) Chore(deps): Bump net.bytebuddy:byte-buddy in /versions (#15252) Chore(deps): Bump io.zonky.test.postgres:embedded-postgres-binaries-bom (#15253) Chore(deps): Bump io.zonky.test:embedded-postgres in /versions (#15254) Chore(deps): Bump net.ltgt.errorprone from 5.0.0 to 5.1.0 in /jablib (#15251) New Crowdin updates (#15247) Refined the "Select files to import" page in "Search for unlinked local files" dialog (#15110) ...
|
I think, this broke my Docker image optimization Line 38 in 3b4f715 Maybe, I just need one more copy statement... |
|
@koppor what is the exception? I think it is missing from the message above. You can also run I don't know your full setup, so I am not sure if this is the right advice. In general, you can do something like this: In general, something more you can do is running |
|
I checked your logs. This is the problem your are facing, right? IIUC it is caused, because you only do a partial copy of the project. The current setup is build around the Alternatively, we could adjust |
|
Thank you for the quik reaction - |
| // depending on the JDK version, 'jsobject' is pulled in as extra dependency or not | ||
| withDependencies { | ||
| if (minJavaVersion >= 26) { | ||
| add("org.openjfx:jdk-jsobject") | ||
| } else { | ||
| removeIf { it.name == "jdk-jsobject" } | ||
| } | ||
| } |
There was a problem hiding this comment.
Doesn't work for JabLib
Refs adoptium/temurin-build#2883 (comment)
Not 100% sure how to fix - just adding as hard dependency? Some monkey patching at jvmDependencyConflicts.patch ? But this is not transported downstream I suppose.
There was a problem hiding this comment.
It should work if you run via Gradle.
Can you remind me how the dependencies are found by the jbang command?
There was a problem hiding this comment.
It creates a pom and then uses maven AFAIK
There was a problem hiding this comment.
Did it work, before the change in this PR?
Can you please explain, which pom.xml files jbang picks up from where? Do you first run the Gradle build and then publish the Jars/POMs somewhere? I would like to understand the complete flow to suggest a proper fix. Feel free to point me at the places in corresponding places in the GH pipeline.
3 participants
Related issues and pull requests
Restores and fixes #15169
PR Description
Steps to test
Checklist
CHANGELOG.mdin a way that can be understood by the average user (if change is visible to the user)