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

gromacs: build with legacy api #126931

Merged
merged 2 commits into from
Mar 29, 2023
Merged

gromacs: build with legacy api #126931

merged 2 commits into from
Mar 29, 2023

Conversation

junghans
Copy link
Contributor

@junghans junghans commented Mar 28, 2023

  • 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 espressopp/espressopp#454

@@ -54,6 +54,7 @@ def install
args = %W[
-DGROMACS_CXX_COMPILER=#{cxx}
-DGMX_VERSION_STRING_OF_FORK=#{tap.user}
-DGMX_INSTALL_LEGACY_API=ON
Copy link
Member

Choose a reason for hiding this comment

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

Does the legacy API replace the modern API?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is no new API (yet), the main thing the switch does is to install the header files.

Copy link
Member

Choose a reason for hiding this comment

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

I'm confused: why would you install this without any headers? It seems very odd for the default build to install things without the headers.

Copy link

@eirrgang eirrgang Mar 29, 2023

Choose a reason for hiding this comment

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

why would you install this without any headers?

Primarily, the project provides a (suite of) command line tool(s).

Several libraries with various degrees of support are also bundled. One of them (libgmxapi) is enabled by default and installs gmxapi/ headers. The oldest (libgromacs) is primarily for internal use to support the command line tool(s). Some of its headers are available, optionally (the gromacs/ headers), but it is not well-supported for use by external software.

@carlocab carlocab changed the title Build Gromacs with legacy api gromacs: build with legacy api Mar 29, 2023
@SMillerDev
Copy link
Member

If it's a legacy API that is disabled by upstream by default, I'm not sure brew should enable it regardless.

@junghans
Copy link
Contributor Author

If it's a legacy API that is disabled by upstream by default, I'm not sure brew should enable it regardless.

Most other Gromacs packages have this enabled as it only installs a couple more header files and otherwise Gromacs cannot used a library due to the missing headers.

@junghans
Copy link
Contributor Author

Commit message updated!

@SMillerDev
Copy link
Member

Most other Gromacs packages have this enabled as it only installs a couple more header files and otherwise Gromacs cannot used a library due to the missing headers.

Can you ask upstream why they disable this by default? It seems like an oversight that a lot of people are compensating for.

@junghans
Copy link
Contributor Author

Most other Gromacs packages have this enabled as it only installs a couple more header files and otherwise Gromacs cannot used a library due to the missing headers.

Can you ask upstream why they disable this by default? It seems like an oversight that a lot of people are compensating for.

I am part of gromacs upstream as well, but the answer is simply that we haven't made any progress on the new API, see
https://gitlab.com/gromacs/gromacs/-/issues/3288.

@mabraham @eirrgang @pszi1ard any additional comments on this?

@carlocab
Copy link
Member

Commit message updated!

Actually, we put the names of formulae first in our commit messages, so it should be something like

gromacs: build with legacy C/C++ API

Will fix that up for you.

this will install additional headers for C/C++ consumer's of the
Gromacs library.
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.

Fine with this, but it seems like this should just be the upstream default for now if the library is not usable without this option.

@carlocab
Copy link
Member

Oops, CI didn't run. githubstatus says degraded performance for GHA.

@junghans
Copy link
Contributor Author

@carlocab thanks!

@github-actions
Copy link
Contributor

🤖 A scheduled task has requested bottles to be published to this PR.

@github-actions github-actions bot added the CI-published-bottle-commits The commits for the built bottles have been pushed to the PR branch. label Mar 29, 2023
@BrewTestBot BrewTestBot enabled auto-merge March 29, 2023 18:13
@BrewTestBot BrewTestBot merged commit 0b6d2cd into Homebrew:master Mar 29, 2023
@junghans junghans deleted the patch-1 branch March 29, 2023 18:29
@mabraham
Copy link
Contributor

Most other Gromacs packages have this enabled as it only installs a couple more header files and otherwise Gromacs cannot used a library due to the missing headers.

Can you ask upstream why they disable this by default? It seems like an oversight that a lot of people are compensating for.

I am part of gromacs upstream as well, but the answer is simply that we haven't made any progress on the new API, see https://gitlab.com/gromacs/gromacs/-/issues/3288.

@mabraham @eirrgang @pszi1ard any additional comments on this?

That's a fair statement. As Eric said, the primary use for the library is to support command-line tools. Some people make use of the library directly, via the legacy headers, which we don't install by default to underscore that these headers are deprecated, unsupported, and unstable.

@carlocab
Copy link
Member

these headers are deprecated, unsupported, and unstable.

That makes me inclined to revert this change then. We only ship stable and supported software/features in Homebrew/core.

@p-linnane
Copy link
Member

This should definitely be reverted based off that information.

@junghans
Copy link
Contributor Author

Well one more important note is, there is no replacement for the legacy headers yet and hence the options for library using Gromacs are use the legacy api or drop Gromacs support completely.

@carlocab
Copy link
Member

Well, as @mabraham says above, the only supported use of the library is in the command line tools, so that's the only thing we should use the library for.

@SMillerDev
Copy link
Member

these headers are deprecated, unsupported, and unstable.

That's a pretty clear indication this should be reverted

@mabraham
Copy link
Contributor

That said, the headers do little harm and only to the subset of people who opt into using them and perhaps later run into instability. It's your project, but I'd just leave things as they are. :-)

@junghans
Copy link
Contributor Author

@carlocab @p-linnane could one of your help me to create a Formula for the votca package?

@github-actions github-actions bot added the outdated PR was locked due to age label May 26, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 26, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
CI-published-bottle-commits The commits for the built bottles have been pushed to the PR branch. outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants