LLNL / H5Z-ZFP

A registered ZFP compression plugin for HDF5
Other
50 stars 22 forks source link

rework feat-cmake_tests CMake and replace scripts #111

Closed byrnHDF closed 1 year ago

byrnHDF commented 1 year ago

Only the repack tests fail on local linux system.

Enable CMake testing:

byrnHDF commented 1 year ago

Forcing shared HDF5 allows the repack test to work correctly.

byrnHDF commented 1 year ago

Will leave test code comments to Scot.

byrnHDF commented 1 year ago

There is a macro that need to be added that will allow the various sources of hdf5 binaries to be used. This PR only checks for CMake built HDF5. I have a macro in use by other projects that will search for variations.

markcmiller86 commented 1 year ago

@byrnHDF how does this PR relate to #110

markcmiller86 commented 1 year ago

Looks like GCC build is having trouble finding HDF5 package.

byrnHDF commented 1 year ago

Looks like GCC build is having trouble finding HDF5 package.

Right - that is the need for the macro. To find hdf5 regardless of how it was built (autotools or two different CMake ways)

markcmiller86 commented 1 year ago

Right - that is the need for the macro. To find hdf5 regardless of how it was built (autotools or two different CMake ways)

Ok, so does that mean that we're still waiting on those finishing touches here in this PR?

byrnHDF commented 1 year ago

Correct. This PR gives a preview of the changes compared to Scot feat-cmake_tests PR. (For review) Plus it allows easier access to the code from other platforms (Like windows) for testing.

byrnHDF commented 1 year ago

Just added a macro that should now find the variety of ways that hdf5 is presented. Windows still needs debugging.

byrnHDF commented 1 year ago

The windows issue is with the plugin version under ctest environment - the same command passes if ran by hand.

markcmiller86 commented 1 year ago

Note...I am noticing I or someone else who has necessary privs here winds up having to hit the Approve and run button for CI checks on a PR from a fork. I think that is because we're triggering with pull_request instead of pull_request_target... here, https://github.com/LLNL/H5Z-ZFP/blob/1660d94a4f73a350d308e262f5ec022cc896a9c4/.github/workflows/main.yml#L8

ChatGPT explains the difference...

What is the difference between the pull_request and pull_request_target triggers in GitHub actions?

In GitHub Actions, pull_request and pull_request_target are both event triggers that are used to run workflows in response to pull request events. However, they have some differences in how they work.

The pull_request trigger is activated whenever a pull request is opened, reopened, or synchronized (when a new commit is added to the branch that the pull request is based on). This trigger runs the workflow on the head repository (i.e., the repository where the changes were made) and provides information about the pull request and its associated branch.

The pull_request_target trigger, on the other hand, is activated when a pull request is opened or synchronized, but it runs the workflow on the base repository (i.e., the repository where the pull request is being merged into). This trigger provides information about the pull request, its associated branch, and the repository that it is being merged into.

The key difference between the two triggers is that pull_request_target is more secure as it only runs when a pull request is opened or synchronized from a forked repository into the base repository. It ensures that the workflow is only triggered on the base repository, even if the pull request comes from an untrusted source.

In summary, if you are running a workflow that needs access to secrets or other sensitive information, it is recommended to use pull_request_target instead of pull_request to ensure security.

brtnfld commented 1 year ago

Github has a requirement that first-time contributors will require approval to run workflows. It's in the actions settings for the repo.

brtnfld commented 1 year ago

@markcmiller86 I'm working on addressing the various test issues you mentioned, I should have a PR up soon.