-
-
Notifications
You must be signed in to change notification settings - Fork 308
Add HDF5 Java bindings to Maven #5847
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
base: develop
Are you sure you want to change the base?
Conversation
How can I fix this error?
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See my last comment on PR.
Create a snapshot in your fork |
Why not jar for arm64 windows and linux? https://github.blog/changelog/2025-08-07-arm64-hosted-runners-for-public-repositories-are-now-generally-available/ |
Because we don't have binaries for those - create an issue. |
No, I would not change the naming at this time because of the intention to switch to FFM instead of JNI and history and compatibility. I think it would be too drastic of a change at this time. Also, it should be announced ahead of time. After this release, we should make announcement once we decide on plans. |
Changes suggested should be new issues. |
I love the |
📝 Inconsistent Quoting Inconsistent quoting stylesJAR_FILES=$(find ./artifacts -name "*.jar" -not -name "test" | tr '\n' ',' | sed 's/,$//') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove. We will add Claude.md files separately.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agrred
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔴 SECURITY VULNERABILITY - Secret Exposure
Location: .github/workflows/maven-deploy.yml:62-64
- name: Identify GPG Status
id: set-gpg-state
env:
gpg_secret: ${{ secrets.GPG_PRIVATE_KEY }}
run: |
if [[ '${{ env.gpg_secret }}' == '' ]] #⚠️ POTENTIAL SECRET LEAK
Problem: Using ${{ env.gpg_secret }} in conditional could expose secret in logs if GitHub Actions debug logging is enabled. Fix:
run: |
if [[ -z "$gpg_secret" ]]; then # Use env var directly, not expansion
GPG_VAL="notexists"
else
GPG_VAL="exists"
fi
Location: .github/workflows/maven-deploy.yml:69
if [[ '${{ env.gpg_secret }}' == '' ]]
then
GPG_VAL=$(echo 'notexists')
else
CPG_VAL=$(echo 'exists') #
fi
echo "HAVEGPG=$GPG_VAL" >> $GITHUB_OUTPUT # Uses GPG_VAL, but else branch sets CPG_VAL
Impact: When GPG key exists, $GPG_VAL is undefined, so output will be empty string → GPG signing will never work.
🔴 RACE CONDITION: Artifact Dependency
Location: .github/workflows/maven-deploy.yml:66-69
validate-artifacts:
runs-on: ubuntu-latest
needs: [] #
steps:
- name: Download Linux artifacts
uses: actions/download-artifact@v4
with:
name: Linux-${{ inputs.preset_name }}-artifacts
Problem: No guarantee artifacts exist when download runs. Workflow should depend on the job that creates these artifacts. Fix: Add needs: [build-artifacts] or similar dependency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again these comments are incorrect - the security comment I think is wrong as we are not using the value as a secret and only checking that it exists - I believe this is the recommended usage, unless proved otherwise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟡 INCOMPLETE ERROR HANDLING
Location: .github/workflows/maven-staging.yml:177-180
if [ -d "hdf5${{ github.workspace }}" ]; then
mv hdf5${{ github.workspace }}/* hdf5/ || true #
rmdir hdf5${{ github.workspace }} || true
fi
Problem: || true masks legitimate errors. If mv fails due to permissions/disk space, build continues silently.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will change in the next PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔴 RACE CONDITION: Artifact Dependency
Location: .github/workflows/maven-deploy.yml:66-69
validate-artifacts:
runs-on: ubuntu-latest
needs: [] #
steps:
- name: Download Linux artifacts
uses: actions/download-artifact@v4
with:
name: Linux-${{ inputs.preset_name }}-artifacts
Problem: No guarantee artifacts exist when download runs. Workflow should depend on the job that creates these artifacts. Fix: Add needs: [build-artifacts] or similar dependency.
- CODE QUALITY ISSUES (Medium Priority)
📝 Excessive Code Comments
Location: .github/scripts/validate-maven-artifacts.sh Issue: 516-line script has good structure but overly verbose comments. Example:
Java/Maven environment validation
validate_environment() {
log_info "Validating build environment..."
# Check Java availability # ⚠️ Function name already says this
if ! command -v java &> /dev/null; then
Recommendation: Remove redundant comments, keep only non-obvious logic explanations.
📝 Magic Numbers
Location: .github/scripts/validate-maven-artifacts.sh:107
if [[ ${file_size} -lt 1024 ]]; then #
Fix: Define constants:
readonly MIN_JAR_SIZE_BYTES=1024 # Minimum valid JAR file size
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maven-deploy is a callable workflow that is called after the artifacts are uploaded - test-maven-deployment.yml file. So these comments are out of context.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📝 Hard-Coded Paths
Location: java/src/hdf/hdf5lib/pom.xml.in
https://github.com/HDFGroup/hdf5/blob/develop/LICENSE
Problem: Hard-codes develop branch. Should use @HDF5_VERSION_TAG@ or similar variable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will investigate
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://github.com/HDFGroup/hdf5/blob/develop is used in documentation. This is only instance in the pom.xml files and only to the LICENSE file. Where is HDF5_VERSION_TAG set?
💡 Missing Source/Javadoc JARs
|
Fixes #3652
successfully implements a comprehensive HDF5 Java Library and Examples Maven integration with:
✅ Complete Maven Pipeline - All 62 Java examples as deployable Maven artifact
✅ Cross-Platform CI/CD - Linux, Windows, macOS x86_64, macOS aarch64
✅ Production Deployment - GitHub Packages integration with version conflict resolution
✅ Comprehensive Documentation - User guides, technical docs, and status reports
✅ Fork-Based Testing - Safe validation methodology before canonical deployment
📊 Final Status:
The Maven integration is production-ready and provides HDF5 Java developers with a modern, professional-grade experience using standard Maven dependency management.
Important
Adds comprehensive Maven integration for HDF5 Java bindings, including CI/CD workflows, cross-platform support, and detailed documentation.
HDF5_ENABLE_MAVEN_DEPLOY
andHDF5_MAVEN_SNAPSHOT
CMake options for Maven deployment.maven-staging.yml
andmaven-deploy.yml
for artifact generation and deployment.java-examples-maven-test.yml
for testing Java examples with Maven artifacts.ci-MinShar-GNUC-Maven
for Java artifact generation.README.md
,CHANGELOG.md
, andINSTALL_CMake.txt
with Maven integration details.README-MAVEN.md
inHDF5Examples/JAVA
for Maven usage instructions.This description was created by
for a37de45. You can customize this summary. It will automatically update as commits are pushed.