Skip to content

Add script to test examples with h5*cc pkg-config wrappers#5486

Merged
lrknox merged 79 commits intoHDFGroup:developfrom
byrnHDF:develop-sh-exam
Jul 16, 2025
Merged

Add script to test examples with h5*cc pkg-config wrappers#5486
lrknox merged 79 commits intoHDFGroup:developfrom
byrnHDF:develop-sh-exam

Conversation

@byrnHDF
Copy link
Contributor

@byrnHDF byrnHDF commented Apr 25, 2025

Fixes #4605
Also corrects the h5cc generation to eliminate hl variations.
Fixes #5367


Important

Add script to test HDF5 examples with h5*cc pkg-config wrappers and update CMake configurations for pkg-config support.

  • Behavior:
    • Adds a new script test-pc.sh in HDF5Examples to test examples with h5*cc pkg-config wrappers.
    • Updates .github/workflows/cmake-bintest.yml to include a new job test_h5cc_linux for testing on Ubuntu with gcc and CMake.
    • Modifies HDF5Examples/README.md and Using_CMake.txt to document the new testing script.
  • CMake:
    • Updates CMakeLists.txt in src, hl/src, hl/c++/src, hl/fortran/src, fortran/src to configure pkg-config files and install h5*cc scripts if pkg-config is available.
    • Adds checks for pkg-config support in config/ConfigureChecks.cmake.
  • Misc:
    • Removes bin/h5cc.in and bin/h5redeploy.in as they are replaced by pkg-config based scripts.
    • Changes file permissions for HDF5Examples/C/H5VDS/test-pc.sh to make it executable.

This description was created by Ellipsis for bafacec. You can customize this summary. It will automatically update as commits are pushed.

@byrnHDF byrnHDF added Priority - 3. Low 🔽 Component - Wrappers C++, Java & Fortran wrappers Component - Testing Code in test or testpar directories, GitHub workflows labels Apr 25, 2025
@byrnHDF byrnHDF self-assigned this Apr 25, 2025
@byrnHDF byrnHDF marked this pull request as draft April 25, 2025 14:47

# HDF5 compile commands, assuming they are in your $PATH.
H5CC=$HDF5_HOME/bin/h5cc
H5CC=$HDF5_HOME/bin/h5hlcc
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is this h5hlcc? We want to test h5cc.

@brtnfld
Copy link
Collaborator

brtnfld commented Jul 7, 2025

@ellipsis-dev, review PR

Copy link

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

Caution

Changes requested ❌

Reviewed everything up to bafacec in 2 minutes and 25 seconds. Click for details.
  • Reviewed 2612 lines of code in 28 files
  • Skipped 0 files when reviewing.
  • Skipped posting 17 draft comments. View those below.
  • Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. src/CMakeLists.txt:25
  • Draft comment:
    The IDE_GENERATED_PROPERTIES macro is called with variables like H5_HDRS (and similarly for H5A, H5B, etc.) which are not explicitly defined in this file. Consider either defining these variables (perhaps as a concatenation of public and private header lists) or updating the macro calls to use the already defined H5_PUBLIC_HDRS and H5_PRIVATE_HDRS.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
2. src/CMakeLists.txt:7
  • Draft comment:
    This CMakeLists.txt file is very lengthy and covers many HDF5 subsystems. For maintainability and clarity, consider breaking it into modular components (e.g. a separate CMake file per subsystem such as H5, H5A, H5B, etc.) and including them from a top-level file.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
3. src/CMakeLists.txt:1240
  • Draft comment:
    The target_link_libraries call for the static library uses variables (LINK_LIBS, LINK_COMP_LIBS, LINK_PUB_LIBS) that are assumed to be defined externally. Verify that these variables are properly defined in the build configuration; otherwise, linking errors may arise.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
4. .github/workflows/cmake-bintest.yml:235
  • Draft comment:
    Typo: The step name "set hdf5lib name" uses a lowercase 'set' unlike other step names. Consider capitalizing it (e.g. "Set hdf5lib name") for consistency.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% While the comment is technically correct about the inconsistency, our rules state we should not make purely cosmetic or unimportant comments. Step names in GitHub Actions are primarily for human readability and don't affect functionality. This seems like a very minor style issue that doesn't warrant a comment. The inconsistency could make the workflow file harder to read and maintain over time. Consistent naming conventions are a part of good code quality. While consistency is good, this is an extremely minor issue that doesn't affect functionality or significantly impact readability. Our rules specifically say to avoid obvious or unimportant comments. This comment should be deleted as it addresses a very minor style issue that doesn't meaningfully impact code quality or functionality.
5. .github/workflows/cmake-bintest.yml:247
  • Draft comment:
    Possible typo: The step "List files for the space (Linux)" might be intended to refer to the "workspace". Verify if "space" should be corrected to "workspace".
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50% The consistent use across multiple sections suggests this is an intentional naming choice. The step simply lists files in both workspace and runner.workspace directories. While "workspace" might be more precise, changing it is not a functional improvement and would require changing multiple similar steps for consistency. The comment is suggesting a purely cosmetic change that would require updating multiple similar steps. The current name, while perhaps not optimal, is clear enough in context. This is exactly the kind of non-essential, purely cosmetic suggestion that our rules say to avoid. The current name works fine and is used consistently. Delete this comment as it suggests a non-essential rename that would affect multiple steps and provides no functional benefit.
6. .github/workflows/cmake-bintest.yml:252
  • Draft comment:
    Minor suggestion: The step name "Run testpc (Linux)" might be clearer if it read "Run test-pc (Linux)" to match the invoked script name (test-pc.sh).
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% While the comment is technically correct about the inconsistency, this is an extremely minor naming issue in a CI workflow file. The current name is clear enough to understand what it does. This feels like unnecessary nitpicking that doesn't materially improve the code quality. The inconsistency could potentially cause confusion when someone is trying to match the workflow step with the actual script being run. The current name "testpc" is still clear enough to understand what's happening, and this minor inconsistency won't impact functionality or maintainability in any meaningful way. This comment should be deleted as it suggests a trivial change that doesn't meaningfully improve the code.
7. HDF5Examples/C/H5D/test-pc.sh:14
  • Draft comment:
    Typographical suggestion: Consider removing the comma after 'Environment variable' to read as "Environment variable HDF5_HOME is expected to be set."
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% This is an extremely minor stylistic change to documentation. While technically correct that the comma isn't necessary, this kind of nitpicking about punctuation in comments doesn't improve code quality or functionality. The current comment is perfectly readable and clear. This falls under the "Do NOT make comments that are obvious or unimportant" rule. Perhaps maintaining consistent documentation style is more important than I'm giving credit for? Some teams have strict documentation standards. Even if documentation standards exist, a single optional comma in a shell script comment is too minor to warrant a PR comment. This would be noise in the review process. Delete this comment as it's an unimportant stylistic suggestion about punctuation in documentation that doesn't meaningfully improve the code.
8. HDF5Examples/C/H5G/test-pc.sh:65
  • Draft comment:
    Typo alert: The assignment on line 65 (ECHO_C=') appears to have an unclosed single quote, which might be a syntax error. Please confirm if this is intended or fix the missing quote.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
9. HDF5Examples/C/H5VDS/test-pc.sh:14
  • Draft comment:
    Typo: Consider removing the comma in "Environment variable, HDF5_HOME is expected to be set." for improved clarity. Perhaps change to "Environment variable HDF5_HOME is expected to be set."
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50% This is an extremely minor stylistic suggestion about punctuation in a comment. It doesn't affect code functionality at all. The existing comment is perfectly readable and clear. The rules specifically say not to make purely informative comments or unimportant suggestions. Perhaps improving documentation clarity could be considered important for maintainability? While documentation is important, this comma change is so minor that it doesn't meaningfully impact readability or understanding. The comment is already clear as-is. This comment should be deleted as it suggests an extremely minor stylistic change that doesn't meaningfully improve the code or documentation.
10. HDF5Examples/FORTRAN/H5D/test-pc.sh:143
  • Draft comment:
    Typo in file name: After updating diff output path to use $TESTDIR/tmp.diff, the grep command still refers to tmp.diff. Please update it to $TESTDIR/tmp.diff.
  • Reason this comment was not posted:
    Comment was on unchanged code.
11. HDF5Examples/FORTRAN/H5D/test-pc.sh:167
  • Draft comment:
    Typographical note: The grep command is referencing 'tmp.diff' without the $TESTDIR prefix, while the diff output is written to '$TESTDIR/tmp.diff'. Consider updating this to '$TESTDIR/tmp.diff' for consistency.
  • Reason this comment was not posted:
    Comment was on unchanged code.
12. HDF5Examples/FORTRAN/H5D/test-pc.sh:169
  • Draft comment:
    Typographical note: The wc command is reading 'tmp.diff' without the $TESTDIR prefix, even though the diff file is created in '$TESTDIR/tmp.diff'. It might be clearer to use '$TESTDIR/tmp.diff' here as well.
  • Reason this comment was not posted:
    Comment was on unchanged code.
13. HDF5Examples/FORTRAN/test-pc.sh:1
  • Draft comment:
    Typo: The shebang line on line 1 has an extra space after '#!'. Consider changing it from "#! /bin/sh" to "#!/bin/sh" for proper interpreter invocation.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% While technically correct, this is a very minor style issue. The script will work perfectly fine with either form. The space after #! is not a functional problem. This feels like an overly pedantic comment that doesn't meaningfully improve code quality. According to the rules, we should not make purely informative comments or comments that are obvious/unimportant. The space in shebang could theoretically cause issues on some very old Unix systems, and some style guides do recommend avoiding the space. While true, this is a modern codebase and the space after #! is widely supported. The benefit of fixing this is minimal compared to the noise it adds to the review. This comment should be deleted as it's too minor and doesn't require a code change that would meaningfully improve the codebase.
14. HDF5Examples/README.md:17
  • Draft comment:
    There's a duplicated preposition in the sentence: "The test-pc.sh script can test the examples with the h5*cc pkg-config wrappers with:". Consider removing one of the "with" words to improve clarity (e.g., "...wrappers:").
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% While the comment is technically correct about the duplicate preposition, this is a very minor grammatical issue in documentation. The meaning is still clear even with the duplicate "with". PR comments should focus on substantial issues requiring code changes, not minor documentation wording preferences. The duplicate preposition could make the documentation slightly less professional. Documentation quality is important for open source projects. While documentation quality matters, this is too minor of an issue to warrant a PR comment. The meaning is clear and this is just a style preference. Delete this comment as it's too minor and doesn't point out any significant issues that need to be fixed.
15. HDF5Examples/test-pc.sh:1
  • Draft comment:
    Typo: The shebang line '#! /bin/sh' has an extra space after '#!'. Consider changing it to '#!/bin/sh'.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% The comment is technically correct - there is an extra space. However, both '#!/bin/sh' and '#! /bin/sh' are valid and widely used forms. This is an extremely minor stylistic issue that doesn't affect functionality. The comment doesn't suggest any meaningful improvement to code quality or fix any actual problems. The extra space could theoretically be considered a deviation from the most common convention, and standardizing on one form could improve consistency. While consistency is good, this is such a minor stylistic issue that bringing it up creates more noise than value. Both forms are completely valid and widely used. This comment should be deleted as it points out an extremely minor stylistic issue that doesn't meaningfully impact code quality or functionality.
16. HDF5Examples/test-pc.sh:40
  • Draft comment:
    Notice: There is no newline at the end of the file. Adding one might help avoid issues with some tools.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50% The comment is purely informative, suggesting adding a newline at the end of the file to avoid potential issues with some tools. It doesn't provide a specific code suggestion or ask for a test to be written. It doesn't align with the rules for good comments, as it doesn't address a specific issue or provide a constructive suggestion.
17. doxygen/dox/IntroHDF5.dox:203
  • Draft comment:
    Typo suggestion: Consider hyphenating 'pkg-config based' to 'pkg-config-based' so that the compound modifier is clearer.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% This is a documentation file. The hyphenation suggestion is technically correct English grammar, but it's a very minor style issue. The meaning is perfectly clear either way. This kind of nitpicky grammar comment doesn't add enough value to justify interrupting the PR review process. The hyphenation would make the text slightly more correct grammatically. Documentation should strive to be well-written. While technically correct, this level of grammar nitpicking is too minor to be worth a PR comment. The current text is perfectly understandable. Delete this comment as it's too minor of a style suggestion to be worth raising in a PR review.

Workflow ID: wflow_sDtiR1zvNosddYqC

You can customize Ellipsis by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.

@brtnfld brtnfld self-requested a review July 15, 2025 22:22
@lrknox lrknox merged commit 968cc9d into HDFGroup:develop Jul 16, 2025
213 of 219 checks passed
@github-project-automation github-project-automation bot moved this from In progress to Done in HDF5 - TRIAGE & TRACK Jul 16, 2025
qkoziol pushed a commit to qkoziol/hdf5 that referenced this pull request Oct 11, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Component - Testing Code in test or testpar directories, GitHub workflows Component - Wrappers C++, Java & Fortran wrappers

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

Add GitHub workflow to test compile scripts such as h5cc generated by CMake Add h5* compiler wrapper testing for CMake

4 participants