Skip to content

Switch to gradlex for modularity #13112

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

Merged
merged 34 commits into from
Jun 1, 2025
Merged

Switch to gradlex for modularity #13112

merged 34 commits into from
Jun 1, 2025

Conversation

koppor
Copy link
Member

@koppor koppor commented May 13, 2025

Fixes https://github.com/JabRef/jabref-issue-melting-pot/issues/919

Replaces all by better alternatives

Mandatory checks

  • I own the copyright of the code submitted and I license it under the MIT license
  • [/] Change in CHANGELOG.md described in a way that is understandable for the average user (if change is visible to the user)
  • [/] Tests created for changes (if applicable)
  • Manually tested changed features in running JabRef (always required)
  • [/] Screenshots added in PR description (if change is visible to the user)
  • [/] Checked developer's documentation: Is the information available and up to date? If not, I outlined it in this pull request.
  • [/] Checked documentation: Is the information available and up to date? If not, I created an issue at https://github.com/JabRef/user-documentation/issues or, even better, I submitted a pull request to the documentation repository.


// --add-opens
opensTo.add("javafx.base/com.sun.javafx.beans=net.bytebuddy")
opensTo.add("javafx.fxml/javafx.fxml=org.jabref")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The opensTo here is oriented at what you can do in module-info.java files and not at the command line. The idea is to have this "block" look similar to how you would write a module-info.java if it were possible for whitebox tests.

The opensTo opens all packages of this test module (tests of org.jabref.jablib) to another module.

E.g. by default the plugin already does opensTo.add("org.junit.platform.commons").

You can consistently open packages in the existing modules javafx.base of javafx.fxml by patching them. See my comment here.

@koppor koppor changed the title [WIP] switch to gradlex for modularity Switch to gradlex for modularity May 31, 2025
Copy link

trag-bot bot commented Jun 1, 2025

@trag-bot didn't find any issues in the code! ✅✨

@koppor koppor added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Jun 1, 2025
@koppor koppor marked this pull request as ready for review June 1, 2025 00:43
@koppor
Copy link
Member Author

koppor commented Jun 1, 2025

Tried out locally - works.

image

Copy link

trag-bot bot commented Jun 1, 2025

@trag-bot didn't find any issues in the code! ✅✨

Copy link
Contributor

github-actions bot commented Jun 1, 2025

The build of this PR is available at https://builds.jabref.org/pull/13112/merge.

@@ -27,10 +25,16 @@ application{
)
}

val javafxVersion = "24.0.1"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this be moved to common build.gradle.kts or is the variable unreachable then?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Juli(a)?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IDK if dependabot will know it then...

@calixtus calixtus added this pull request to the merge queue Jun 1, 2025
Merged via the queue into main with commit 4f1b9fc Jun 1, 2025
60 of 63 checks passed
@calixtus calixtus deleted the ok-branch-3 branch June 1, 2025 17:43
Siedlerchr added a commit to brandon-lau0/jabref that referenced this pull request Jun 2, 2025
…or-test

* upstream/main: (102 commits)
  Try to fix output
  Improve AI preferences UI and templates (JabRef#13202)
  Bump jablib/src/main/abbrv.jabref.org from `6926b83` to `333c2f1` (JabRef#13216)
  Bump jablib/src/main/resources/csl-styles from `8a2317a` to `c3df987` (JabRef#13215)
  Fixed search result focus handling (JabRef#13174)
  New Crowdin updates (JabRef#13214)
  Add Pseudonymization to CLI (JabRef#13158)
  Try parallel gource build
  Update gource.yml
  Fix position of checkout
  Preapre: Enable gradle configuration cache (JabRef#13212)
  Add yml as YAML extension (JabRef#13213)
  Fix wrong detection of issue numbers (JabRef#13211)
  Miscellaneous refactoring - II (JabRef#13197)
  Run Windows tests only on main (and on demand) (JabRef#13210)
  Fix porcelain for consistency check (JabRef#13209)
  Use setup-jbang action (instead of custom call of .sh script) (JabRef#13208)
  Add link to JabRef guru (JabRef#13207)
  Switch to gradlex for modularity (JabRef#13112)
  feat(ci-cd): change issue URL pattern (JabRef#13206)
  ...
@Siedlerchr
Copy link
Member

Siedlerchr commented Jun 2, 2025

After a clean, I now get issues on macOS (m1, arm) with run
It seems like it downloads the wrong javafx version because we don't have the classifier:
implementation("org.openjfx:javafx-base:$javafxVersion:mac-aarch64")
this is what the openjfx plugin does

@jjohannes You use a mac as well, do you have a hint? Do we need to readd the openjfx plugin?

2025-06-02 21:29:21 [JavaFX-Launcher] sun.util.logging.internal.LoggingProviderImpl$JULWrapper.log()
WARN: Unsupported JavaFX configuration: classes were loaded from 'module javafx.graphics', isAutomatic: false, isOpen: true
Loading library prism_es2 from resource failed: java.lang.UnsatisfiedLinkError: /Users/christophs/.openjfx/cache/24.0.1+4/aarch64/libprism_es2.dylib: dlopen(/Users/christophs/.openjfx/cache/24.0.1+4/aarch64/libprism_es2.dylib, 0x0001): tried: '/Users/christophs/.openjfx/cache/24.0.1+4/aarch64/libprism_es2.dylib' (mach-o file, but is an incompatible architecture (have 'x86_64', need 'arm64e' or 'arm64')), '/System/Volumes/Preboot/Cryptexes/OS/Users/christophs/.openjfx/cache/24.0.1+4/aarch64/libprism_es2.dylib' (no such file), '/Users/christophs/.openjfx/cache/24.0.1+4/aarch64/libprism_es2.dylib' (mach-o file, but is an incompatible architecture (have 'x86_64', need 'arm64e' or 'arm64'))

build:

@koppor
Copy link
Member Author

koppor commented Jun 2, 2025

Maybe we need to partially revert 3f3e38e (#13112)?

I think, this is the snippet:

build-logic/src/main/kotlin/buildlogic.java-common-conventions.gradle.kts

+ val arch = System.getProperty("os.arch")
+ val javafxPlatform = when {
+     os.isWindows -> "win"
+     os.isMacOsX && arch == "aarch64" -> "mac-aarch64"
+     os.isMacOsX -> "mac"
+     os.isLinux && arch == "aarch64" -> "linux-aarch64"
+     os.isLinux -> "linux"
+     else -> error("Unsupported OS/arch: ${os.name} / $arch")
+ }

+ project.extra["javafxPlatform"] = javafxPlatform

(and in multiple files)

+    implementation("org.openjfx:javafx-base:$javafxVersion:$javafxPlatform")
+    implementation("org.openjfx:javafx-controls:$javafxVersion:$javafxPlatform")
+    implementation("org.openjfx:javafx-fxml:$javafxVersion:$javafxPlatform")
+    // implementation("org.openjfx:javafx-graphics:24.0.1:win")
+    implementation("org.openjfx:javafx-graphics:$javafxVersion:$javafxPlatform")
+    implementation("org.openjfx:javafx-swing:$javafxVersion:$javafxPlatform")
+    implementation("org.openjfx:javafx-web:$javafxVersion:$javafxPlatform")
-    implementation("org.openjfx:javafx-base:$javafxVersion")
-    implementation("org.openjfx:javafx-controls:$javafxVersion")
-    implementation("org.openjfx:javafx-fxml:$javafxVersion")
-    // implementation("org.openjfx:javafx-graphics:24.0.1")
-    implementation("org.openjfx:javafx-graphics:$javafxVersion")
-    implementation("org.openjfx:javafx-swing:$javafxVersion")
-    implementation("org.openjfx:javafx-web:$javafxVersion")
``´

@Siedlerchr
Copy link
Member

Yeaht that seems like the best option

@koppor koppor mentioned this pull request Jun 2, 2025
1 task
@jjohannes
Copy link
Collaborator

@Siedlerchr @koppor hi. No need to revert this. The issue is in the OS detection code. This:

val osVersion = System.getProperty("os.version")
if (osVersion.startsWith("14")) "macos-14" else "macos-13"

Should be something like this:

        val arch = System.getProperty("os.arch")
        if (arch.contains("aarch")) "macos-14" else "macos-13"

The names for the targets are taken from the current GitHub hosted runner names. macos-14 is ARM. I think that caused confusion. You can change the names of the targets to something "clearer" if you like.

(Note: Based on your feedback, this OS detection may work out of the box in the future - gradlex-org/java-module-packaging#51)

@koppor koppor mentioned this pull request Jun 7, 2025
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dev: binaries Binary builds should be uploaded to builds.jabref.org dev: ci-cd status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants