gromacs: don't force build with gcc #129648

junghans · 2023-04-28T16:45:25Z

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 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 --build-from-source <formula>)? If this is a new formula, does it pass brew audit --new <formula>?

For #129512

fxcoudert · 2023-04-28T16:58:09Z

libomp dep in itself won't be enough

fxcoudert · 2023-04-28T22:15:08Z

Have you tested the changes locally? And checked that they produce the correct linking? That does not seem likely.

carlocab · 2023-04-28T23:56:06Z

Formula/gromacs.rb

@@ -25,7 +25,10 @@ class Gromacs < Formula
  depends_on "gcc" # for OpenMP
  depends_on "openblas"


Note that OpenBLAS uses libgomp, so you'll be mixing OpenMP implementations here.

Hmm, not sure what do about this....

Note that OpenBLAS uses libgomp, so you'll be mixing OpenMP implementations here.

So is it appropriate to just replace libomp with libgomp below?

Or is it reasonable to entirely leave out the depends_on "libomp" entirely to just get transitive installation dependency or does it provide necessary build-time details?

I simply don't think we can build that with libomp

To be clearer: I think the issue is in the (proposed) votca formula and should be fixed there. We don't want to introduce this mixed-OpenMP linkage.

I simply don't think we can build that with libomp

What is the actual problem with that? I have seen openblas being built with clang using spack.io before.

To be clearer: I think the issue is in the (proposed) votca formula and should be fixed there. We don't want to introduce this mixed-OpenMP linkage.

Again I am not sure about this either, we build VOTCA on large variety different compiler and library combination without problems. And I of course tried to restrict votca to gcc as well, which led to this error: #129512 (comment)
because boost was build with clang.

I simply don't think we can build that with libomp

What is the actual problem with that? I have seen openblas being built with clang using spack.io before.

If I believe:
https://github.com/spack/spack/blob/develop/var/spack/repos/builtin/packages/openblas/package.py#L205-L209
clang+openmp should works for openblas>0.2.19.

junghans · 2023-04-29T00:46:53Z

Have you tested the changes locally? And checked that they produce the correct linking? That does not seem likely.

We had a user reporting the linking issue in #129512 and once they build gromacs outside homebrew, it worked. Adding votca to homebrew just seems like the better thing to do for future users.

fxcoudert

I repeat my question:

Have you tested the changes locally? And checked that they produce the correct linking? That does not seem likely.

I am 99% certain that this PR disables OpenMP completely in gromacs.

junghans · 2023-04-29T13:47:32Z

I repeat my question:

Have you tested the changes locally? And checked that they produce the correct linking? That does not seem likely.

I am 99% certain that this PR disables OpenMP completely in gromacs

Again I have no idea, where the build logs are, but we could run "gmx -version" as a test and would tell us.

junghans · 2023-05-13T17:14:44Z

@carlocab can you point out to me where the logs are?

SMillerDev · 2023-05-14T08:38:01Z

They are in the CI details below, but unless something fails they won't have much detail. These verifications that people are asking for does not seem like something you can do with CI though, it should be done locally.

cho-m · 2023-05-14T21:07:12Z

libomp dep in itself won't be enough

I do think CMake (w/ Makefile) is able to automatically handle linking Homebrew's libomp since https://gitlab.kitware.com/cmake/cmake/-/merge_requests/1812

Would still be good to get someone to locally install gromacs and do some basic tests to make sure nothing is broken.

Logs do show detection, e.g.

-- Found OpenMP_C: -Xclang -fopenmp (found version "5.0") 
-- Found OpenMP_CXX: -Xclang -fopenmp (found version "5.0") 
-- Found OpenMP: TRUE (found version "5.0")

[  3%] Building CXX object _deps/muparser-build/CMakeFiles/muparser.dir/src/muParser.cpp.o
cd /tmp/gromacs-20230501-9192-v38zqp/gromacs-2023.1/build/_deps/muparser-build && /usr/local/Homebrew/Library/Homebrew/shims/mac/super/clang++ -DGMX_DOUBLE=0 -DMUPARSERLIB_EXPORTS -DMUPARSER_DLL -DMUP_USE_OPENMP -Dmuparser_EXPORTS -isystem /tmp/gromacs-20230501-9192-v38zqp/gromacs-2023.1/src/external -isystem /tmp/gromacs-20230501-9192-v38zqp/gromacs-2023.1/src/external/muparser/include -O3 -DNDEBUG -std=c++11 -isysroot /Library/Developer/CommandLineTools/SDKs/MacOSX13.sdk -fPIC -Xclang -fopenmp -MD -MT _deps/muparser-build/CMakeFiles/muparser.dir/src/muParser.cpp.o -MF CMakeFiles/muparser.dir/src/muParser.cpp.o.d -o CMakeFiles/muparser.dir/src/muParser.cpp.o -c /tmp/gromacs-20230501-9192-v38zqp/gromacs-2023.1/src/external/muparser/src/muParser.cpp

//Enable OpenMP-based multithreading
GMX_OPENMP:BOOL=ON

//Maximum number of OpenMP Threads supported. Has to be 32 or a
// multiple of 64.
GMX_OPENMP_MAX_THREADS:STRING=128

And bottle lib has linkage:

❯ otool -L gromacs/2023.1/lib/libgromacs.8.0.0.dylib
gromacs/2023.1/lib/libgromacs.8.0.0.dylib:
	@@HOMEBREW_PREFIX@@/opt/gromacs/lib/libgromacs.8.dylib (compatibility version 8.0.0, current version 8.0.0)
	@@HOMEBREW_PREFIX@@/opt/fftw/lib/libfftw3f.3.dylib (compatibility version 10.0.0, current version 10.10.0)
	@@HOMEBREW_PREFIX@@/opt/openblas/lib/libopenblas.0.dylib (compatibility version 0.0.0, current version 0.0.0)
	/usr/lib/libSystem.B.dylib (compatibility version 1.0.0, current version 1319.100.3)
	@@HOMEBREW_PREFIX@@/opt/libomp/lib/libomp.dylib (compatibility version 5.0.0, current version 5.0.0)
	@rpath/libmuparser.2.dylib (compatibility version 2.0.0, current version 2.3.4)
	/usr/lib/libc++.1.dylib (compatibility version 1.0.0, current version 1500.65.0)

We should also discuss more on situation of mixing GCC vs LLVM implementations of libraries here and in #129100

junghans · 2023-05-15T00:45:17Z

@cho-m thanks for testing this, I don't have a Mac for development, but maybe @eirrgang can test this locally.

eirrgang · 2023-05-15T01:37:40Z

@cho-m thanks for testing this, I don't have a Mac for development, but maybe @eirrgang can test this locally.

I can take a look later in the week. Multiple competing openmp implementations sounds problematic.

Also, it doesn't seem like the title/description of this PR is accurate. As I understand, the PR removes the prohibition against clang, then uses whatever compiler comes from ENV. (What affects which compiler comes from ENV?)

Again, I apologize for my complete ignorance of the brew framework.

eirrgang · 2023-05-17T00:11:47Z

@cho-m thanks for testing this, I don't have a Mac for development, but maybe @eirrgang can test this locally.

I can take a look later in the week. Multiple competing openmp implementations sounds problematic.

Also, it doesn't seem like the title/description of this PR is accurate. As I understand, the PR removes the prohibition against clang, then uses whatever compiler comes from ENV. (What affects which compiler comes from ENV?)

Again, I apologize for my complete ignorance of the brew framework.

I built locally and ran a few simulation steps without crashing. I used 2 OpenMP threads and 4 tMPI threads.

Contrary to the issue title, gromacs is built with gcc 13

Both gromacs and libopenblas link to libgomp.

junghans · 2023-05-17T14:17:46Z

Hmm in @cho-m build it uses clang though, so you are right the PR title is misleading, I think this PR is mainly about not forcing gcc.

eirrgang · 2023-05-17T20:56:18Z

Formula/gromacs.rb

    inreplace "src/gromacs/gromacs-hints.in.cmake" do |s|
      s.gsub! "@CMAKE_LINKER@", "/usr/bin/ld"
-      s.gsub! "@CMAKE_C_COMPILER@", cc
-      s.gsub! "@CMAKE_CXX_COMPILER@", cxx
+      s.gsub! "@CMAKE_C_COMPILER@", ENV.cc


Where does ENV come from?

eirrgang · 2023-05-17T20:58:01Z

Formula/gromacs.rb

@@ -25,7 +25,10 @@ class Gromacs < Formula
  depends_on "gcc" # for OpenMP


Do we need depends_on "gcc" # for OpenMP or is it even the appropriate way to ensure an omp facility if we always depend on openblas? I mean, is the omp dependence of openblas sufficiently transitive?

We prefer dependencies to be explicit than transitive, when there is a direct dependency.

We prefer dependencies to be explicit than transitive, when there is a direct dependency.

Sounds good. In order to be both explicit and correct, how would gromacs.rb reference the (brew) package dependency to get appropriate CMake hints? (I think the only available CMake input variable is OpenMP_CXX_INCLUDE_DIR, since the details are normally so compiler-specific.)

It seems like we might need the choice of libomp vs. libgomp to be coupled to the choice of clang vs. gcc?

Or is there a way to inspect the openblas dependency and just use whatever it used?

There is no choice to be had: formulas don't really have options, and binaries shipped are built on our CI machines.

I think you are saying

the gromacs formula declares a dependency on openblas, so it can and should encode decisions needed to build compatibly, per the openblas formula

gromacs should therefore declare a dependency on libgomp

the formula encodes its toolchain, which in this case must be gcc based

Are all three of those points correct?

I'm particularly unclear on that last one, since it would seem to invalidate the whole premise of this pull request (which has not yet been outright rejected). It seems like any one of those points, though, would invalidate much of this change.

Yes (except there is no libgomp, it is a part of gcc). I have not outright closed the PR because I wanted to see if any other maintainer felt differently about it, but it seems not, so I'll close it now.

eirrgang · 2023-05-17T21:00:39Z

Formula/gromacs.rb

@@ -25,7 +25,10 @@ class Gromacs < Formula
  depends_on "gcc" # for OpenMP
  depends_on "openblas"

-  fails_with :clang
+  on_macos do
+    depends_on "libomp"


This change may have succeeded in establishing a package dependency, but without additional CMake arguments, the build system configuration is still free to find libgomp if it is available and finds it preferable. I assume that is what happened in my test.

eirrgang · 2023-05-17T21:04:05Z

Formula/gromacs.rb

@@ -25,7 +25,10 @@ class Gromacs < Formula
  depends_on "gcc" # for OpenMP
  depends_on "openblas"

-  fails_with :clang


I would be happy to test again, compiling with clang. It seems like it would be nice to remove this broad clang-avoidance. But what is the mechanism for choosing compiler toolchain for either brew install -s gromacs or for the built-package repositories?

update:

I tried brew install --cc llvm_clang -s gromacs, but I don't seem to be set up for that.

I tried brew install --cc clang -s gromacs but brew still invoked CMake with cmake -S . -B build -DGROMACS_CXX_COMPILER=/usr/local/opt/gcc/bin/g++-13 -DGMX_VERSION_STRING_OF_FORK=Homebrew -DGMX_INSTALL_LEGACY_API=ON

fxcoudert · 2023-05-18T19:53:02Z

This would bring mixed OpenMP runtime linkage, which is not suitable. Closing as per discussion.

junghans · 2023-05-19T00:18:29Z

I actually thought let’s build one blas with clang as this seems to work as well, but anyhow.

@fxcoudert can you help me on the votca formula then? I have no idea how the fox the boost linking issue, now the toolchain mixing issue bubbles up one layer more down.

github-actions bot added the autosquash Automatically squash pull request commits according to Homebrew style. label Apr 28, 2023

carlocab reviewed Apr 28, 2023

View reviewed changes

gromacs: build with clang

62eb712

junghans force-pushed the gromacs_clang branch from 3c09872 to 62eb712 Compare April 29, 2023 00:42

github-actions bot removed the autosquash Automatically squash pull request commits according to Homebrew style. label Apr 29, 2023

junghans marked this pull request as ready for review April 29, 2023 00:43

fxcoudert requested changes Apr 29, 2023

View reviewed changes

gromacs: run different test

8c47fc4

github-actions bot added the autosquash Automatically squash pull request commits according to Homebrew style. label May 1, 2023

junghans changed the title ~~gromacs: build with clang~~ gromacs: don't force build with gcc May 17, 2023

eirrgang reviewed May 17, 2023

View reviewed changes

fxcoudert closed this May 18, 2023

eirrgang mentioned this pull request May 19, 2023

votca 2022.1 (new formula) #129512

Merged

6 tasks

carlocab mentioned this pull request May 19, 2023

openblas: try clang build #131296

Closed

6 tasks

junghans deleted the gromacs_clang branch June 5, 2023 12:39

github-actions bot added the outdated PR was locked due to age label Jul 6, 2023

github-actions bot locked as resolved and limited conversation to collaborators Jul 6, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gromacs: don't force build with gcc #129648

gromacs: don't force build with gcc #129648

junghans commented Apr 28, 2023 •

edited

Loading

fxcoudert commented Apr 28, 2023

fxcoudert commented Apr 28, 2023

carlocab Apr 28, 2023

junghans Apr 29, 2023

eirrgang May 15, 2023

fxcoudert May 17, 2023

fxcoudert May 17, 2023

junghans May 18, 2023

junghans May 18, 2023

junghans May 18, 2023

junghans commented Apr 29, 2023

fxcoudert left a comment

junghans commented Apr 29, 2023

junghans commented May 13, 2023

SMillerDev commented May 14, 2023

cho-m commented May 14, 2023 •

edited

Loading

junghans commented May 15, 2023

eirrgang commented May 15, 2023

eirrgang commented May 17, 2023

junghans commented May 17, 2023

eirrgang May 17, 2023

eirrgang May 17, 2023

fxcoudert May 17, 2023

eirrgang May 17, 2023 •

edited

Loading

fxcoudert May 18, 2023

eirrgang May 18, 2023

fxcoudert May 18, 2023

eirrgang May 17, 2023

eirrgang May 17, 2023

eirrgang May 17, 2023

fxcoudert commented May 18, 2023

junghans commented May 19, 2023

		@@ -25,7 +25,10 @@ class Gromacs < Formula
		depends_on "gcc" # for OpenMP
		depends_on "openblas"

gromacs: don't force build with gcc #129648

gromacs: don't force build with gcc #129648

Conversation

junghans commented Apr 28, 2023 • edited Loading

fxcoudert commented Apr 28, 2023

fxcoudert commented Apr 28, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

junghans commented Apr 29, 2023

fxcoudert left a comment

Choose a reason for hiding this comment

junghans commented Apr 29, 2023

junghans commented May 13, 2023

SMillerDev commented May 14, 2023

cho-m commented May 14, 2023 • edited Loading

junghans commented May 15, 2023

eirrgang commented May 15, 2023

eirrgang commented May 17, 2023

junghans commented May 17, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eirrgang May 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fxcoudert commented May 18, 2023

junghans commented May 19, 2023

junghans commented Apr 28, 2023 •

edited

Loading

cho-m commented May 14, 2023 •

edited

Loading

eirrgang May 17, 2023 •

edited

Loading