Skip to content

Conversation

ywwry66
Copy link
Contributor

@ywwry66 ywwry66 commented Apr 11, 2025

  • 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>?

Related to #209091, #129648, #129100. Upstream has added support for buiding with libomp through OpenMathLib/OpenBLAS#5180, where one can also find multiple issues linked from downstream repositories that are caused by Homebrew's handling of OpenMP for OpenBLAS.

This change requires dependents of OpenBLAS also switched to libomp. For gromacs, cp2k and fftw, the switch seems relatively straightforward. However, I am not sure how to go about dynare. Currently, dynare is directly linked to libgomp and it also pulls in libomp through suite-sparse.

@github-actions github-actions bot added the long dependent tests Set a long timeout for dependent testing label Apr 11, 2025
@ywwry66 ywwry66 force-pushed the openblas_use_cmake branch from a509697 to ff2aa4d Compare April 11, 2025 04:18
@ywwry66

This comment was marked as resolved.

@ywwry66 ywwry66 force-pushed the openblas_use_cmake branch from ff2aa4d to 017f197 Compare April 11, 2025 21:01
@github-actions github-actions bot added the autosquash Automatically squash pull request commits according to Homebrew style. label Apr 11, 2025
@ywwry66 ywwry66 force-pushed the openblas_use_cmake branch 3 times, most recently from af08516 to 8cb793b Compare April 19, 2025 16:25
@ywwry66 ywwry66 force-pushed the openblas_use_cmake branch 2 times, most recently from 6450d0c to 7d01829 Compare April 19, 2025 20:10
@ywwry66
Copy link
Contributor Author

ywwry66 commented Apr 20, 2025

The failures on macOS 15-arm64 can be reproduced on my Sequoia macbook with the current OpenBLAS bottle, so they are not related to this PR.

  1. arpack: Minitest::Assertion: Expected /reached/ to match " ** On entry to DLASCL parameter number 4 had an illegal value\n ** On entry to DLASCL parameter number 4 had an illegal value\n \n Error with _naupd, info = -9999\n Check the documentation of _naupd\n \n". (failed regression check for 3.9.1 on macos sequoia opencollab/arpack-ng#469)
  2. mlpack: /opt/homebrew/include/cereal/types/tuple.hpp:98:41: error: a template argument list is expected after a name prefixed by the template keyword [-Wmissing-template-arg-list-after-template-kw] (Fix instances of -Wmissing-template-arg-list-after-template-kw. USCiLab/cereal#835)

There is an error I didn't see before:

  • shtools: make: *** [run-fortran-tests-no-timing] Error 137 (GH Runner out of memory? Cannot reproduce locally.)

@ywwry66 ywwry66 force-pushed the openblas_use_cmake branch from 7d01829 to 3fb931b Compare April 20, 2025 04:54
@github-actions github-actions bot removed the autosquash Automatically squash pull request commits according to Homebrew style. label Apr 20, 2025
@ywwry66 ywwry66 force-pushed the openblas_use_cmake branch 2 times, most recently from d5ace47 to 23b371f Compare April 21, 2025 15:06
@ywwry66 ywwry66 changed the title openblas: test cmake build openblas: use cmake Apr 21, 2025
@ywwry66 ywwry66 force-pushed the openblas_use_cmake branch 3 times, most recently from d8e8d7a to 5d8ee9a Compare April 21, 2025 20:36
@ywwry66 ywwry66 changed the title openblas: use cmake openblas: use cmake and depend on libomp Apr 23, 2025
@ywwry66
Copy link
Contributor Author

ywwry66 commented Apr 23, 2025

Should switching to libomp for dependents be done in this same PR or in separate PRs?

@ywwry66
Copy link
Contributor Author

ywwry66 commented Apr 25, 2025

fftw's makefile does not support libomp directly, and its cmake system is not feature complete, e.g., no mpi support. Changes to cmake were not very well received upstream. So I am not sure how to proceed here. Ping @carlocab and @cho-m for suggestions.

@github-actions
Copy link
Contributor

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@github-actions github-actions bot added the stale No recent activity label May 17, 2025
@ywwry66
Copy link
Contributor Author

ywwry66 commented May 19, 2025

Keep the PR open for now.

@github-actions github-actions bot removed the stale No recent activity label May 19, 2025
@github-actions
Copy link
Contributor

github-actions bot commented Jun 9, 2025

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@ywwry66
Copy link
Contributor Author

ywwry66 commented Aug 31, 2025

Just a word of caution: the OpenBLAS upstream is considering reverting the change that has added and streamlined OpenMP customization enabled by cmake's built-in module (ref. OpenMathLib/OpenBLAS#5413). If that is merged, it will also invalidate this PR, at least in its current form.

@cho-m
Copy link
Member

cho-m commented Aug 31, 2025

A Makefile workaround could be:

# Workaround to use OpenMP with Apple Clang
if ENV.compiler == :clang
  inreplace "Makefile.system", "+= -fopenmp", "+= -Xpreprocessor -fopenmp"
  inreplace "Makefile.install" do |s|
    s.gsub! ":= -fopenmp", ":= -Xpreprocessor -fopenmp"
    s.gsub! "+= -lgomp", "+= -L#{Formula["libomp"].opt_lib} -lomp"
  end
end

Can't comment if this functionally behaves as expected. But at least no mixed linkage

brew linkage openblas
System libraries:
  /usr/lib/libSystem.B.dylib
Homebrew libraries:
  /opt/homebrew/opt/gcc/lib/gcc/current/libgfortran.5.dylib (gcc)
  /opt/homebrew/opt/gcc/lib/gcc/current/libquadmath.0.dylib (gcc)
  /opt/homebrew/opt/libomp/lib/libomp.dylib (libomp)
PKG_CONFIG_PATH=/opt/homebrew/opt/openblas/lib/pkgconfig pkgconf --cflags openblas
-I/opt/homebrew/Cellar/openblas/0.3.30/include -Xpreprocessor -fopenmpPKG_CONFIG_PATH=/opt/homebrew/opt/openblas/lib/pkgconfig pkgconf --libs openblas
-L/opt/homebrew/Cellar/openblas/0.3.30/lib -lopenblasPKG_CONFIG_PATH=/opt/homebrew/opt/openblas/lib/pkgconfig pkgconf --libs --static openblas
-L/opt/homebrew/Cellar/openblas/0.3.30/lib -lopenblas -lpthread -lgfortran -lpthread -lgfortran -L/opt/homebrew/opt/libomp/lib -lomp

@cho-m cho-m mentioned this pull request Sep 7, 2025
6 tasks
@cho-m
Copy link
Member

cho-m commented Sep 7, 2025

Experimenting with Makefile alternative in

@ywwry66
Copy link
Contributor Author

ywwry66 commented Sep 7, 2025

I am trying to keep the upstream cmake improvements from being reverted so hopefully we don't need the Makefile alternative.

BTW, does the arrayfire test failure on Linux X86_64 need to be addressed?

@cho-m
Copy link
Member

cho-m commented Sep 7, 2025

BTW, does the arrayfire test failure on Linux X86_64 need to be addressed?


Also noting that I will be updating openblas test in separate PR to run cpp_thread_test to verify OpenMP is still functional:

@github-actions
Copy link
Contributor

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@github-actions github-actions bot added the stale No recent activity label Sep 28, 2025
@cho-m cho-m added in progress Stale bot should stay away and removed stale No recent activity labels Sep 28, 2025
@dimpase
Copy link
Contributor

dimpase commented Sep 28, 2025

is it still being worked on?

@cho-m cho-m force-pushed the openblas_use_cmake branch from 18ffbc1 to aa8cd82 Compare September 29, 2025 00:01
system ENV.cc, "test.c", "-o", "test", *flags
system "./test"

flags += %W[-I#{Formula["libomp"].opt_include} -L#{Formula["libomp"].opt_lib} -lomp] if OS.mac?
Copy link
Member

@cho-m cho-m Sep 29, 2025

Choose a reason for hiding this comment

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

Will note that one problem with change is pkg-config file is it in an odd half-way state

Libs: -L${libdir} -l${libnameprefix}openblas${libnamesuffix}${libsuffix} 
Cflags: -I${includedir} -Xclang -fopenmp 

This won't work as is with libomp unless user provides the missing arguments like the above line whereas GCC worked with just the pkg-config output.

Similarly, installed CMake config file may not behave as expected (as OpenMP_Fortran will link libgomp without some extra hacks):

set_target_properties(OpenBLAS::OpenBLAS PROPERTIES
  INTERFACE_INCLUDE_DIRECTORIES "${_IMPORT_PREFIX}/include/openblas"
  INTERFACE_LINK_LIBRARIES "m;OpenMP::OpenMP_C;OpenMP::OpenMP_Fortran"
)

However, users are more likely to use FindBLAS and FindOpenMP so may not see issue with OpenBLASConfig.cmake

@cho-m
Copy link
Member

cho-m commented Sep 29, 2025

is it still being worked on?

We first need confirmation that upstream won't revert CMake changes.

Afterward will need to decide if the odd quirks this can introduce are worth the change.

@cho-m cho-m force-pushed the openblas_use_cmake branch from aa8cd82 to cb70ea0 Compare September 29, 2025 22:58
@cho-m cho-m force-pushed the openblas_use_cmake branch from cb70ea0 to ca9a5b9 Compare September 30, 2025 02:20
@cho-m cho-m added blocked upstream issue An upstream issue report is needed labels Sep 30, 2025

keg_only :shadowed_by_macos, "macOS provides BLAS in Accelerate.framework"

depends_on "cmake" => :build
Copy link
Member

Choose a reason for hiding this comment

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

blocked CI-linux-self-hosted Build on Linux self-hosted runner CI-no-fail-fast Continue CI tests despite failing GitHub Actions matrix builds. CI-no-fail-fast-deps Continue dependent tests despite failing GitHub Actions matrix tests. in progress Stale bot should stay away long build Set a long timeout for formula testing long dependent tests Set a long timeout for dependent testing maintainer feedback Additional maintainers' opinions may be needed upstream issue An upstream issue report is needed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants