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.

Sign up for GitHub

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

gromacs: don't force build with gcc #129648

Closed
wants to merge 2 commits into from
Closed

gromacs: don't force build with gcc #129648

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
62eb712
gromacs: build with clang
junghans Apr 28, 2023
8c47fc4
gromacs: run different test
junghans May 1, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 10 additions & 11 deletions Formula/gromacs.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,10 @@ class Gromacs < Formula
depends_on "gcc" # for OpenMP
Copy link

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link

@eirrgang eirrgang May 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

depends_on "openblas"
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.


fails_with :clang
Copy link

Choose a reason for hiding this comment

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

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?

Copy link

Choose a reason for hiding this comment

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

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

on_macos do
depends_on "libomp"
Copy link

Choose a reason for hiding this comment

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

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.

end

fails_with gcc: "5"
fails_with gcc: "6"

Expand All @@ -35,24 +38,20 @@ def install
"CMAKE_INSTALL_DATADIR"

# Avoid superenv shim reference
gcc = Formula["gcc"]
cc = gcc.opt_bin/"gcc-#{gcc.any_installed_version.major}"
cxx = gcc.opt_bin/"g++-#{gcc.any_installed_version.major}"
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
Copy link

Choose a reason for hiding this comment

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

Where does ENV come from?

s.gsub! "@CMAKE_CXX_COMPILER@", ENV.cxx
end

inreplace "src/buildinfo.h.cmakein" do |s|
s.gsub! "@BUILD_C_COMPILER@", cc
s.gsub! "@BUILD_CXX_COMPILER@", cxx
s.gsub! "@BUILD_C_COMPILER@", ENV.cc
s.gsub! "@BUILD_CXX_COMPILER@", ENV.cxx
end

inreplace "src/gromacs/gromacs-config.cmake.cmakein", "@GROMACS_CXX_COMPILER@", cxx
inreplace "src/gromacs/gromacs-config.cmake.cmakein", "@GROMACS_CXX_COMPILER@", ENV.cxx

args = %W[
-DGROMACS_CXX_COMPILER=#{cxx}
-DGMX_VERSION_STRING_OF_FORK=#{tap.user}
-DGMX_INSTALL_LEGACY_API=ON
]
Expand All @@ -76,6 +75,6 @@ def caveats
end

test do
system "#{bin}/gmx", "help"
system "#{bin}/gmx", "-version"
end
end