Skip to content

Switch GROMACS build type to Release#36181

Merged
junghans merged 1 commit intospack:developfrom
pszi1ard:fix-gromacs-build-type
Mar 17, 2023
Merged

Switch GROMACS build type to Release#36181
junghans merged 1 commit intospack:developfrom
pszi1ard:fix-gromacs-build-type

Conversation

@pszi1ard
Copy link
Copy Markdown
Contributor

The current default RelWithDebInfo gives significantly slower builds so it should not be the default.

The current default RelWithDebInfo gives significantly slower builds
so it should not be the default.
@pszi1ard
Copy link
Copy Markdown
Contributor Author

@junghans @marvinbernhardt not sure about the spack release maintenance policies, but it would be good to backport this as much as possible to maintained releases since it can cause up to ~20% performance loss which I'm sure many will be happy to avoid.

Copy link
Copy Markdown
Member

@alalazo alalazo left a comment

Choose a reason for hiding this comment

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

Waiting for maintainers opinion on this. Honestly I'd be in favor of changing this in CMakePackage in general 🙂

@alalazo alalazo self-assigned this Mar 17, 2023
@pszi1ard
Copy link
Copy Markdown
Contributor Author

Waiting for maintainers opinion on this. Honestly I'd be in favor of changing this in CMakePackage in general slightly_smiling_face

That will still not change the default for cases where default="RelWithDebInfo" is specified in package.py, right (there are a few other cases other than GROMACS).

I'd be curious to know what lead to this strange choice of RelWithDebInfo build type as default instead of using whatever is the respective software's cmake default?

@junghans junghans merged commit 3897c13 into spack:develop Mar 17, 2023
@alalazo
Copy link
Copy Markdown
Member

alalazo commented Mar 18, 2023

@pszi1ard See #22918 and links therein

@pszi1ard
Copy link
Copy Markdown
Contributor Author

@pszi1ard See #22918 and links therein

Thanks for the link. The default is in this case not inherited but explicitly set (overriding the GROMACS default and with causing the reported performance loss), and it is still not clear what is the reasoning behind this choice.

alalazo added a commit to alalazo/spack that referenced this pull request Apr 6, 2023
alalazo added a commit to alalazo/spack that referenced this pull request May 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants