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

valgrind: disable mpicc and make Linux only #67341

Merged
merged 2 commits into from
Dec 22, 2020

Conversation

danielnachun
Copy link
Member

  • Have you followed the guidelines for contributing?
  • Have you checked that there aren't other open pull requests for the same formula update/change?
  • Have you built your formula locally with 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 brew install <formula>)?

This disables mpicc support in valgrind, which seems to cause build failures on host systems with mpich or OpenMPI installed. If we ever get a request to add support for mpicc to valgrind, we'd probably have to add mpich or open-mpi as a dependency. Additionally the configure argument --build=amd64-darwin should only be used on macOS.

Moved to homebrew-core from https://github.com/Homebrew/linuxbrew-core/pull/21870.

@BrewTestBot BrewTestBot added the deprecated license Formula uses a deprecated SPDX license which should be updated label Dec 21, 2020
Formula/valgrind.rb Outdated Show resolved Hide resolved
@SeekingMeaning SeekingMeaning added the linux to homebrew-core Migration of linuxbrew-core to homebrew-core label Dec 21, 2020
@SMillerDev
Copy link
Member

This formula should probably just be linux only because it doesn't run on any of our supported macOS versions

@carlocab
Copy link
Member

depends_on maximum_macos: :sierra would work too, though I can see the advantages of it being Linux-only.

There also seems to be a version of valgrind that works at least until Catalina: https://github.com/LouisBrunner/valgrind-macos

@danielnachun
Copy link
Member Author

Since there is a tap that provides a working version of valgrind for all versions of macOS, I'm inclined to make this Linux only. Hopefully the patches in that fork of valgrind can be integrated into the official release so that it will once again work in macOS, but that is an issue for upstream to deal with.

@danielnachun
Copy link
Member Author

Actually if we make this Linux only, should I close this pull request? Does homebrew-core allow Linux only formulae?

@SeekingMeaning
Copy link
Contributor

SeekingMeaning commented Dec 21, 2020

Does homebrew-core allow Linux only formulae?

Yep, for example: #65300

This is how I would convert it to Linux-only: https://gist.github.com/SeekingMeaning/10db0c2f3a8a29fa760b6903ad808671/revisions

We might even be able to delete the skip_clean stanza, but I'm not completely certain (Homebrew/legacy-homebrew#2150 might shed some light)

@danielnachun
Copy link
Member Author

Thanks for the revisions, I'll try removing skip_clean on my Linux install and see if it breaks anything. Everything else looks pretty straightforward.

@SeekingMeaning
Copy link
Contributor

Also add Formula/valgrind.rb to .github/workflows/tests-linux.yml

@iMichka
Copy link
Member

iMichka commented Dec 21, 2020

I agree that this can be made linux-only. Though we also try to disable valgrind support when possible in all other formulae.

@SeekingMeaning SeekingMeaning changed the title valgrind: disable mpicc and only specify Darwin build on macOS valgrind: disable mpicc and make Linux only Dec 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge-skip `brew pr-automerge` will skip this pull request deprecated license Formula uses a deprecated SPDX license which should be updated linux to homebrew-core Migration of linuxbrew-core to homebrew-core
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants