Skip to content
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

hdf5: update to use parallel version of hdf5 #167668

Closed
wants to merge 1 commit into from

Conversation

abhinavsns
Copy link
Contributor

hdf5 parallel is required for certain applications and should not conflict with serial applications as they are part of the same source code. Having both creates linking conflicts.
Please revert PR #167477 and remove the formula hdf5-mpi as it becomes a duplicate of this.

  • Have you followed the guidelines for contributing?
  • Have you ensured that your commits follow the commit style guide?
  • Have you checked that there aren't other open pull requests for the same formula update/change?
  • Have you built your formula locally with HOMEBREW_NO_INSTALL_FROM_API=1 brew install --build-from-source <formula>, where <formula> is the name of the formula you're submitting?
  • Is your test running fine brew test <formula>, where <formula> is the name of the formula you're submitting?
  • Does your build pass brew audit --strict <formula> (after doing HOMEBREW_NO_INSTALL_FROM_API=1 brew install --build-from-source <formula>)? If this is a new formula, does it pass brew audit --new <formula>?

@github-actions github-actions bot added the CI-linux-self-hosted Build on Linux self-hosted runner label Apr 1, 2024
Copy link
Member

@SMillerDev SMillerDev left a comment

Choose a reason for hiding this comment

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

There are some pretty big changes here that the description does not explain the reasons for. Please only change the serial vs parallel part in this review

Formula/h/hdf5.rb Outdated Show resolved Hide resolved
Formula/h/hdf5.rb Outdated Show resolved Hide resolved
Formula/h/hdf5.rb Outdated Show resolved Hide resolved
@abhinavsns abhinavsns force-pushed the patch-5 branch 3 times, most recently from 03bc1a0 to 6efd0c4 Compare April 1, 2024 09:23
@abhinavsns
Copy link
Contributor Author

Okay! I am starting to see the issues here. Hdf5 cannot be compiled with both libcpp-hdf5 (cpp bindings) and parallel option. Probably that is why hdf5-mpi exists. Maybe it is best to introduce petsc-mpi and petsc-complex-mpi for hdf5-mpi version? I am new to brew and unsure how to resolve this issue. Some help would be appreciated. @chenrui333 @fxcoudert

@abhinavsns abhinavsns force-pushed the patch-5 branch 3 times, most recently from 26930f6 to 3f69666 Compare April 2, 2024 18:21
hdf5 parallel is required for certain applications and should not conflict with serial applications as they are part of the same source code.
@iMichka
Copy link
Member

iMichka commented Apr 2, 2024

petsc has 262 downloads in the last 30 days; that's not much. And I am not sure how much success the two new ones would have.

Maybe this could be hosted in a tap: https://docs.brew.sh/Taps, https://docs.brew.sh/How-to-Create-and-Maintain-a-Tap

@cho-m
Copy link
Member

cho-m commented Apr 3, 2024

Going to close this PR as current change is not the right approach. It doesn't package the standard hdf5 and just creates a duplicate of hdf5-mpi.

hdf5 and hdf5-mpi should remain separate formulae, similar to how they are distributed by major Linux repositories (e.g. Debian, Fedora, Arch Linux, etc).


One thing you can explore is whether hdf5-mpi can be made keg-only.

Another alternative is to wait for new conflict handling (Homebrew/brew#16398), which would automatically unlink one of the formulae rather than hard blocking on conflicts. Behavior would be similar to manual approach to conflicts (i.e. brew unlink <conflicting-formula> should allow you to install the other formula)

@cho-m cho-m closed this Apr 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI-linux-self-hosted Build on Linux self-hosted runner
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants