Skip to content

Fix OpenBLAS pkg-config OpenMP flag on macOS #230188

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

derekchiang
Copy link

The OpenBLAS pkg-config file includes -fopenmp which is not supported by macOS's default clang compiler, causing build failures for dependent packages. This patch removes the unsupported flag on macOS while preserving OpenMP support on other platforms.

Fixes issues with packages that depend on OpenBLAS and use pkg-config for configuration.

🤖 Generated with Claude Code

The OpenBLAS pkg-config file includes `-fopenmp` which is not supported
by macOS's default clang compiler, causing build failures for dependent
packages. This patch removes the unsupported flag on macOS while
preserving OpenMP support on other platforms.

Fixes issues with packages that depend on OpenBLAS and use pkg-config
for configuration.

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <[email protected]>
@github-actions github-actions bot added autosquash Automatically squash pull request commits according to Homebrew style. long dependent tests Set a long timeout for dependent testing labels Jul 16, 2025
@derekchiang
Copy link
Author

This patch fixes ocaml/opam-repository#27483

Copy link
Contributor

Thanks for contributing to Homebrew! 🎉 It looks like you're having trouble with a CI failure. See our contribution guide for help. You may be most interested in the section on dealing with CI failures. You can find the CI logs in the Checks tab of your pull request.

@ywwry66
Copy link
Contributor

ywwry66 commented Jul 16, 2025

Related PR: #219339

Copy link
Member

@carlocab carlocab left a comment

Choose a reason for hiding this comment

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

I'm not convinced just deleting this flag is a good idea. You should probably use a compiler that supports -fopenmp instead.

Comment on lines +67 to +69
pkgconfig_file = lib/"pkgconfig/openblas.pc"
pkgconfig_content = pkgconfig_file.read
pkgconfig_file.write pkgconfig_content.gsub(/^omp_opt=-fopenmp$/, "omp_opt=")
Copy link
Member

Choose a reason for hiding this comment

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

You should use inreplace here.

@@ -61,6 +61,13 @@ def install

lib.install_symlink shared_library("libopenblas") => shared_library("libblas")
lib.install_symlink shared_library("libopenblas") => shared_library("liblapack")

# Fix pkg-config file on macOS to remove unsupported -fopenmp flag
if OS.mac?
Copy link
Member

Choose a reason for hiding this comment

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

Do an early return (i.e. return unless OS.mac?) so you don't have to indent everything below here.

@carlocab carlocab requested review from Bo98 and fxcoudert July 16, 2025 06:35
@Bo98
Copy link
Member

Bo98 commented Jul 16, 2025

Yeah the Clang equivalent is -Xclang -fopenmp and unfortunately compiler-specific flags don't really work with pkg-config.

I suspect the flag is only necessary for static linking, where you would need libgomp.a. In theory that means it should go in Cflags.private, though that's a pkgconf-only feature. But we've replaced pkg-config with pkgconf anyway so that downside probably does not matter for us and getting shared linking to work with Clang (where feasible) is somewhat important for macOS.

We presumably didn't notice the issue ourselves because our compiler shims will filter out -fopenmp. Which I guess proves shared linking works fine without it.

@fxcoudert
Copy link
Member

fxcoudert commented Jul 16, 2025

Right now we compile openblas against GCC (due to Fortran and OpenMP use). So that means our pkgconfig file ships GCC options. It works fine with all our packages. The problem arises from ocaml/opam wanting to combine our openblas with clang. They should use another openblas build, or switch to GCC. We aim to support consistent use cases, but can't support every compilation setup. Moreover, removing the -fopenmp will mean other users will loose multithread support.

So I'm 👎 on the PR.

@dimpase
Copy link
Contributor

dimpase commented Jul 16, 2025

Right now we compile openblas against GCC (due to Fortran and OpenMP use). So that means our pkgconfig file ships GCC options. It works fine with all our packages. The problem arises from ocaml/opam wanting to combine our openblas with clang. They should use another openblas build, or switch to GCC. We aim to support consistent use cases, but can't support every compilation setup. Moreover, removing the -fopenmp will mean other users will loose multithread support.

So I'm 👎 on the PR.

Yeah, it has been discussed on #209091 that the way forward would be to have another formula, which uses a build of openblas with clang (I am not sure whether this should be Apple's clang from xcode, or the one from and libomp (not libgomp). (One must not mix libomp and libgomp, and libgomp is not supported by clang).

gfortran works fine with libomp, and openbpas now has a cmake build option which allows this configuration to be used in build. (see OpenMathLib/OpenBLAS#5169 and OpenMathLib/OpenBLAS#5180)

It would be great to have such a formula available, either here (in homebrew core), or in a separate tap.

Copy link
Contributor

@dimpase dimpase left a comment

Choose a reason for hiding this comment

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

I suggest to close this in view of #209091

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autosquash Automatically squash pull request commits according to Homebrew style. long dependent tests Set a long timeout for dependent testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants