Skip to content

Add hdf5-native Maven JARs and SciJava native-lib-loader integration#6356

Draft
matteodg wants to merge 16 commits into
HDFGroup:developfrom
matteodg:6355-add-hdf5-native-maven-artifacts
Draft

Add hdf5-native Maven JARs and SciJava native-lib-loader integration#6356
matteodg wants to merge 16 commits into
HDFGroup:developfrom
matteodg:6355-add-hdf5-native-maven-artifacts

Conversation

@matteodg

@matteodg matteodg commented Apr 7, 2026

Copy link
Copy Markdown

Fixes #6355

I created this with Cursor, as my knowledge of CMake is basically zero: I tried to do it myself, but I was not able. I tested this in Linux x86_64 and Windows x86_64 and it is creating the proper jar and pom.xml file, that I can install in my local Maven repository and depend on them from an example application. This was there is not need of installing HDF5 library or setting up LD_LIBRARY_PATH anymore.

This will be useful for the future of HDFView as well.

@brtnfld

brtnfld commented Apr 8, 2026

Copy link
Copy Markdown
Collaborator

Must Fix:
FFM H5.java static block lacks try-catch around Hdf5NativeLoader call -- NoClassDefFoundError will permanently break H5 if the native-lib-loader JAR is missing

The FFM [H5.java:314] calls Hdf5NativeLoader.loadBundledNativesIfPresent(false) with no try-catch in the static block. If native-lib-loader-2.5.0.jar is missing from the classpath, loading Hdf5NativeLoader will throw NoClassDefFoundError (because it imports org.scijava.nativelib.NativeLoader), which will cause an ExceptionInInitializerError that permanently prevents H5 from being used in that JVM.

The JNI [H5.java:366-374] correctly wraps the call in catch (Throwable err), which handles this. The FFM version does not. The FFM path should have the same protection.

Nice to have:

  • Partial-load when JNI bridge fails but hdf5 succeeds -- works correctly but worth a comment
  • [docs] Transitive native deps (zlib, szip) not bundled -- "zero-install" doesn't hold if HDF5 was built with external filters

Moderate:

  • Duplicate POM profiles across two pom.xml.in files -- The 5 OS-activated profiles (~70 lines each) in pom.xml.in and pom-jni.xml.in) are nearly identical (JNI version adds hdf5-jni-native dependency). This is a lot of duplicated XML that will need to be kept in sync. Not easily avoidable with Maven profiles though.
    HDF5JavaNativeBundles.cmake is include()'d, not add_subdirectory() -- This means all its variables leak into the parent scope. The leading underscores help, but it's still a code smell. Consider using add_subdirectory() or wrapping in a function.

Low Severity

  • Regex in validate-maven-artifacts.sh is getting unwieldy ([line 30]

grep -qE "<artifactId>hdf5-java(-ffm|-jni)?</artifactId>|<artifactId>hdf5-native</artifactId>|<artifactId>hdf5-jni-native</artifactId>|<artifactId>hdf5-java-examples</artifactId>"

Consider using a simpler pattern like hdf5-(java|native|jni-native) or an array-based check.

  • find with -exec cp {} may clobber files -- In ctest.yml and maven-staging.yml, multiple POM files with different names (pom.xml, pom-hdf5-native.xml, etc.) are copied to a flat directory. If there are multiple pom.xml files from different build directories, later ones overwrite earlier ones. This was a pre-existing issue but is now slightly more likely to bite.

  • The _np and _jp variable names in maven-deploy.yml ([lines 112-116] are cryptic. Minor, but NATIVE_POM / JNI_NATIVE_POM would be clearer.

  • RELEASE_PROCESS.md change crams a lot of information into a single bullet point that's now very long and hard to parse.

Verify:

  • Vendored JAR should be checksum-verified against Maven Central; license acknowledged

@brtnfld brtnfld left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

"Must Fix" needs to be addressed.

@github-project-automation github-project-automation Bot moved this from To be triaged to In progress in HDF5 - TRIAGE & TRACK Apr 8, 2026
@hyoklee hyoklee added the Component - Wrappers C++, Java & Fortran wrappers label Apr 10, 2026
@hyoklee hyoklee added this to the Backlog milestone Apr 10, 2026
@matteodg matteodg force-pushed the 6355-add-hdf5-native-maven-artifacts branch 8 times, most recently from 4779e1e to f7e31e1 Compare April 27, 2026 21:54
@matteodg matteodg requested a review from brtnfld April 27, 2026 21:58
@matteodg matteodg force-pushed the 6355-add-hdf5-native-maven-artifacts branch from 2a04368 to cbf65fa Compare April 28, 2026 07:01
@brtnfld

brtnfld commented May 1, 2026

Copy link
Copy Markdown
Collaborator
  • Replace err.printStackTrace() with proper logging

In both java/hdf/hdf5lib/H5.java (FFM) and java/src-jni/hdf/hdf5lib/H5.java (JNI), replace:


catch (Throwable err) {
    err.printStackTrace();
}

with:


catch (Throwable err) {
    log.debug("Bundled HDF5 native library not loaded: " + err.getMessage());
}
  • Remove the unused _HDF5JAVA_NATIVE_POM_DATE variable

In java/cmake/HDF5JavaNativeBundles.cmake:


string (TIMESTAMP _HDF5JAVA_NATIVE_POM_DATE "%Y-%m-%d %H:%M:%S UTC" UTC)
This is computed at configure time but never used anywhere in the file

Nice to have:

  • Add trailing newline to both new POM template files

java/cmake/pom-native.xml.in and java/cmake/pom-jni-native.xml.in both end without a newline (\ No newline at end of file). Add a newline at the end of each.

  • Clarify isHdf5LibraryLoaded dual semantics
    File: java/src-jni/hdf/hdf5lib/H5.java

The problem: isHdf5LibraryLoaded is set in two unrelated places with different meanings:

In the bundled loader block — means "bundled hdf5 loaded"
At H5.H5dont_atexit() — means "hdf5 is confirmed loaded by any means"
A reader maintaining this code later will think the second assignment is a copy-paste error and delete it.

— split the flag's responsibilities):

Replace the two flags with three, each with a single clear meaning:


private static boolean isHdf5LibraryLoaded     = false; // bundled hdf5 load succeeded
private static boolean isHdf5JavaLibraryLoaded = false; // hdf5_java load succeeded
private static boolean isInitialized           = false; // full loadH5Lib() completed

Change the early-exit guard from:

if (isHdf5LibraryLoaded && isHdf5JavaLibraryLoaded)
    return;

to:

if (isInitialized)
    return;

Change the H5.H5dont_atexit() block from:

        try {
            H5.H5dont_atexit();
            isHdf5LibraryLoaded = true;
        }
        catch (HDF5LibraryException e) {
            System.exit(1);
        }

to:

        try {
            H5.H5dont_atexit();
            isInitialized = true;
        }
        catch (HDF5LibraryException e) {
            System.exit(1);
        }

This way isHdf5LibraryLoaded tracks only the bundled load result, isHdf5JavaLibraryLoaded tracks only the JNI bridge load result, and isInitialized guards against redundant re-entry.

brtnfld added a commit to brtnfld/hdf5 that referenced this pull request Jun 10, 2026
Add zizmor_config.yml to ignore template-injection findings in the
setup-jextract action and maven/ctest workflow files introduced in
PR HDFGroup#6356, where inputs are caller-controlled but not user-controlled.
@matteodg matteodg force-pushed the 6355-add-hdf5-native-maven-artifacts branch 2 times, most recently from b2b553f to 6031e92 Compare June 11, 2026 11:13
brtnfld added a commit that referenced this pull request Jun 11, 2026
…t display (#6439)

* review-checklist: show all requested owners per area in checklist

buildBody was using area.owners.find() — picking the first CODEOWNERS-listed
owner in the requested set. This broke reviewer swaps (removing @A and adding
@b would show @C, the next CODEOWNERS entry, not @b) and also failed to
reflect GitHub's CODEOWNERS auto-assignment, which requests all owners.

Switch to area.owners.filter() so every requested owner for an area is
mentioned in that row. Approval logic is unchanged: any owner approval
still checks the box.

* review-checklist: enforce one load-balanced reviewer per area

GitHub's CODEOWNERS auto-assignment requests all owners of touched files
when a PR opens, before the workflow runs. The script was then seeing them
already assigned and skipping its own selection entirely.

On opened/reopened: select one load-balanced reviewer per area (ignoring
GitHub's pre-assigned set), remove any auto-assigned CODEOWNERS not in the
selection, then add the chosen reviewer. Only code owners are removed —
manually-added non-owner reviewers are left untouched.

On synchronize: keep existing assignments (reviewer may have already started).

confirmedRequested now starts empty and is populated only with the script's
selection, so the checklist only mentions owners that were explicitly chosen.

* review-checklist: retry reviewer cleanup to handle GitHub auto-assign race

GitHub's CODEOWNERS auto-assignment can fire after the workflow starts,
re-adding extra reviewers after we remove them. Add a 15-second wait
followed by a second cleanup pass on opened/reopened events.

Extract the removal loop into enforceSelection() so both passes share
the same logic.

* zizmor: suppress template-injection for PR #6356 workflow files

Add zizmor_config.yml to ignore template-injection findings in the
setup-jextract action and maven/ctest workflow files introduced in
PR #6356, where inputs are caller-controlled but not user-controlled.

* review-checklist: don't re-assign reviewers on synchronize

On synchronize, chooseReviewers saw no owner assigned (because the
reviewer was manually removed) and re-added one via requestReviewers,
overriding the manual removal.

Reviewer assignment now only happens on opened/reopened. Synchronize
only updates the checklist display based on whoever is currently
assigned — manual removals are respected.

* zizmor: skip upload-sarif failure on fork PRs

* zizmor: move config out of workflows dir to fix GitHub Actions parse error

GitHub Actions parses all .yml files in .github/workflows/ as workflow
files; zizmor_config.yml there caused "unexpected value 'rules'" because
rules: is not valid workflow syntax. Moved to .github/zizmor.yml and
updated the --config path in zizmor.yml accordingly.

* review-checklist: retry on transient 401 from GitHub API

GitHub's API intermittently returns 401 on write operations
(issues.addAssignees, issues.createComment) even when the token has
Issues: write and PullRequests: write — read-only calls succeed in the
same run. The github-script action excludes 401 from retries by default.
Removing 401 from retry-exempt-status-codes and setting retries: 3
handles these transient failures with exponential backoff.

* review-checklist: fix checklist mention when non-requested owner approves

The mention in each checklist row was derived from the approver (if any),
so when a different area owner happened to approve first, the mention
changed from the assigned reviewer to the approver. Fix by decoupling
sign-off detection from display: signedOff now uses .some() so any
owner's approval checks the box, while the mention always shows the
confirmed-requested reviewer(s) via .filter(). Also shows multiple
reviewers when one is manually added alongside the load-balanced
selection.

* review-checklist: show approver name when signed off, requested reviewer(s) when pending

When an area is signed off, replace the mention with the approver so the
checklist shows who actually reviewed it (which may differ from whoever
was load-balanced as the requested reviewer). When pending, show all
confirmed-requested reviewers — the one load-balanced pick normally, but
two if a reviewer was manually added alongside it.

* zizmor: remove config file and --config flag

The ignored files (ctest.yml, maven-deploy.yml, maven-staging.yml,
setup-jextract/action.yml) had template-injection findings on
inputs.* references, which are not attacker-controllable. Rather than
suppressing them via a config file, let all findings surface in the
Security tab and address them individually if needed.
@lrknox lrknox removed their request for review June 11, 2026 21:18
@matteodg matteodg force-pushed the 6355-add-hdf5-native-maven-artifacts branch 2 times, most recently from 7148db4 to 70290bf Compare June 21, 2026 09:38
@matteodg matteodg marked this pull request as ready for review June 21, 2026 09:38
matteodg and others added 9 commits June 23, 2026 09:51
Download only the jextract 25 Unix binaries with correct platform
detection and verify the installed version for consistent FFM CI output.

Co-authored-by: Cursor <cursoragent@cursor.com>
Download only the jextract 25 Windows binary, verify the installed version,
and use a forward-slash PATH entry on GitHub Windows runners.

Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Extract shared helpers for SciJava native JAR staging, manifest generation,
and OS-activated Maven profile snippets. Wire HDF5JavaNativeBundles.cmake from
FFM and JNI Java binding CMakeLists and move add_subdirectory(java) before the
Maven deploy status message.

Co-authored-by: Cursor <cursoragent@cursor.com>
Bundle libhdf5 under natives/<platform>/ with SciJava-mapped aliases and
install hdf5-native-*-{classifier}.jar plus pom-hdf5-native.xml.

Co-authored-by: Cursor <cursoragent@cursor.com>
Add optional Maven bundles for shared zlib and libaec/szip with platform
aliases and optional OS-activated binding-POM profile snippets.

Co-authored-by: Cursor <cursoragent@cursor.com>
Add JNI bridge native bundle when HDF5JAVA_MAVEN_NATIVE_JNI is enabled.
The standalone pom-jni-native template carries no Maven deps on libhdf5.

Co-authored-by: Cursor <cursoragent@cursor.com>
@matteodg matteodg force-pushed the 6355-add-hdf5-native-maven-artifacts branch 2 times, most recently from e869881 to 16ee5a1 Compare June 23, 2026 15:35
matteodg and others added 3 commits June 23, 2026 18:05
Consolidate bundled zlib/szip/libhdf5 loading in loadBundledDependenciesBeforeHdf5(),
use it from FFM/JNI H5.loadH5Lib and the slimmed hdf5_h_2 jextract patch, and layer
binding POM dependencies (required hdf5-jni-native on java-jni; optional libhdf5 stack).

Co-authored-by: Cursor <cursoragent@cursor.com>
Add artifact validation, Maven consumer smoke test with explicit native deps,
FFM deflate smoke test, and CI workflow updates for native bundle artifacts.

Co-authored-by: Cursor <cursoragent@cursor.com>
Describe optional libhdf5-stack artifacts, required hdf5-jni-native on java-jni,
classpath-only vs system-native consumer paths, and runtime init contract.

Co-authored-by: Cursor <cursoragent@cursor.com>
@matteodg matteodg force-pushed the 6355-add-hdf5-native-maven-artifacts branch from fd1c7ab to 776fccd Compare June 23, 2026 16:05
@matteodg matteodg marked this pull request as draft June 23, 2026 18:19
@matteodg

Copy link
Copy Markdown
Author

I'm trying to fix the failing builds due to this branch.

matteodg and others added 4 commits June 23, 2026 20:50
Centralize SciJava -link URL, native-lib-loader classpath handling, and
hdf5_java_doc target creation for FFM and JNI binding trees.

Co-authored-by: Cursor <cursoragent@cursor.com>
Wire HDF5JavaJavadoc.cmake with the FFM compile classpath so javadoc can
resolve SciJava {@link} references in Hdf5NativeLoader.

Co-authored-by: Cursor <cursoragent@cursor.com>
Wire HDF5JavaJavadoc.cmake with the JNI classpath so javadoc resolves
SciJava cross-references when building the JNI binding tree.

Co-authored-by: Cursor <cursoragent@cursor.com>
Ensure doc builds with HDF5_BUILD_JAVA and HDF5_BUILD_DOC also generate
hdf5_java_doc before the Doxygen hdf5lib_doc stamp target.

Co-authored-by: Cursor <cursoragent@cursor.com>
@matteodg matteodg force-pushed the 6355-add-hdf5-native-maven-artifacts branch from 39b6411 to 7815c86 Compare June 23, 2026 18:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Component - Wrappers C++, Java & Fortran wrappers

Projects

Status: In progress

Development

Successfully merging this pull request may close these issues.

Publish hdf5-native Maven artifacts (bundled libhdf5 for classpath loading)

4 participants