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

CMakeMake could default CMAKE_BUILD_TYPE to RelWithDebInfo instead of Release #3032

Closed
bartoldeman opened this issue Nov 14, 2023 · 12 comments
Closed
Milestone

Comments

@bartoldeman
Copy link
Contributor

Release sets -O3 -DNDEBUG which overrides the default -O2 from easybuild.
RelWithDebInfo sets -O2 -g -DNDEBUG so is more compatible, and the debug info doesn't hurt (it increases the size of the binary but not much compared to typical datasets many researchers deal with, and it does make profiling easier and debug backtraces much more legible).

One could also let it depend on toolchainopts, i.e. if 'opt': True is set, then use Release else RelWithDebInfo?

@bartoldeman bartoldeman added the EasyBuild-5.0 EasyBuild 5.0 label Nov 14, 2023
@boegel boegel added this to the 5.0 milestone Dec 6, 2023
@boegel boegel changed the title cmakemake.py could default CMAKE_BUILD_TYPE to RelWithDebInfo instead of Release CMakeMake could default CMAKE_BUILD_TYPE to RelWithDebInfo instead of Release Dec 6, 2023
@boegel
Copy link
Member

boegel commented Dec 6, 2023

@Flamefire Thoughts on this?

@Flamefire
Copy link
Contributor

I'm a bit worried given that the additional size is not only storage but also bandwidth from the shared filesystem on a cluster where the additional data gets transferred on every use although it is rarely used by most users and more of interest to developers/maintainers.

And we also don't add -g to ConfigureMake based ECs, do we? So why for CMakeMake?

Maybe we could have another flag which is used to decide whether we add -g (CMake honors $CXXFLAGS as well as configure-make, so we could add it there)

As for using another build type: We have the build_type option for CMakeMake which can be changed in ECs and/or via hooks. Maybe that is better for customization than having larger binaries for everyone by default?

@Micket
Copy link
Contributor

Micket commented Apr 5, 2024

Yes I agree we should be consistent, so it would be

  • CMakeMake/Ninja RelWithDebInfo
  • MesonNinja (debugoptimized) https://mesonbuild.com/Running-Meson.html#configuring-the-build-directory
  • MakeCp just have -g in CFLAGS is the best we can do i guess? (essentially just toolchainopts debug=true by default i guess)
  • ConfigureMake also just -g in CFLAGS, i'm not aware of anything standard here.
  • Cargo maybe? Not the same structure, but there is the possibility of RUSTFLAGS=-g

Having a global option also sounds good to me. I don't imagine the size impact to be all that significant; it would in total be far less than a single NVHPC install.

@Flamefire
Copy link
Contributor

As mentioned I don't worry only about the size but also about the additional bandwidth used when transferring the binaries from the global filesystem to the nodes on each use by many users with no use for most of them

(essentially just toolchainopts debug=true by default i guess)

Sounds like we already have a flag for that. So if we already add -g for MakeCp (only) when toolchainopts debug=true then this could be used for choosing RelWithDebInfo in CMakeMake

@boegel boegel moved this to Changed default in EasyBuild v5.0 Aug 28, 2024
@boegel
Copy link
Member

boegel commented Sep 18, 2024

We could introduce a --retain-debug-info EasyBuild configuration option, and let CMakeMake take it into account (some thing could then be done also for other easyblocks, where it makes sense)

@boegel
Copy link
Member

boegel commented Sep 18, 2024

Actually, CMakeMake should honor the debug toolchain option, and --retain-debug-info should determine the default value for the debug toolchain option (which is now False by default).

@Flamefire
Copy link
Contributor

Actually, CMakeMake should honor the debug toolchain option, and --retain-debug-info should determine the default value for the debug toolchain option (which is now False by default).

There are 3 possible options: Release, Release-with-debug-info, Debug (i.e. unoptimized)

Currently CMakeMake does honor the toolchain option and switches between Release and Debug.

The issue I see is that we haven't really defined/documented what that option should do: "Enable debug" is a bit vague

As for --retain-debug-info: With the current implementation this would cause unoptimized (CMake) builds. So I guess we should change that in CMakeMake to RelWithDebInfo and adjust the documentation of toolchainopts['debug'] to "Include debug information" or similar. Not sure if we need/want unoptimized, i.e. full "Debug builds"

@Micket
Copy link
Contributor

Micket commented Sep 19, 2024

Yes if we compare with what EB does with it's compiler flags in the build environment, the toolchain opts does:

  • debug = add -g if true
  • lowopt = use -O1
  • noopt = no -O
if noopt:  # no option but to keep the debug symbols here i think? 
    target = 'Debug'
elif debug:
    target = 'ReleaseWithDebugInfo'
else:
    target = 'Release'

is the correct options I.M.O.

So yes, docs should change to be clear it's about adding debug symbols only.

@Micket
Copy link
Contributor

Micket commented Sep 20, 2024

Made this PR for cmake and meson #3452

@boegel boegel changed the title CMakeMake could default CMAKE_BUILD_TYPE to RelWithDebInfo instead of Release CMakeMake could default CMAKE_BUILD_TYPE to RelWithDebInfo instead of Release Sep 25, 2024
@Flamefire
Copy link
Contributor

As this came up in the confcall I missed: Defaulting to RelWithDebInfo would IMO be a bad choice because the switch from Relase to RelWithDebInfo is exactly what setting toolchainopts.debug means. So to me the semantics outlined in #3032 (comment) are correct.

@Micket
Copy link
Contributor

Micket commented Sep 26, 2024

Yes I mentioned in the confcall (and there was no objections) that this is the plan. Easyblocks respect toolchainopts correctly, and as a separate thing to allow the default value of toolchainopts.debug to be set globally

@boegel
Copy link
Member

boegel commented Oct 8, 2024

@boegel boegel closed this as completed Oct 8, 2024
@boegel boegel removed this from EasyBuild v5.0 Oct 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants