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

RFC: depends: add release type to CMake builds #29962

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

theuni
Copy link
Member

@theuni theuni commented Apr 25, 2024

RFC because I'm not sure if this is the right thing to do in combination with our CFLAGS/CXXFLAGS/etc env overrides.

I believe it was suggested by @laanwj at some point.

@DrahtBot
Copy link
Contributor

DrahtBot commented Apr 25, 2024

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.
A summary of reviews will appear here.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #29960 (depends: pass verbose through to cmake based makefiles by m3dwards)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

@theuni
Copy link
Member Author

theuni commented Apr 25, 2024

Ping @hebasto

@hebasto
Copy link
Member

hebasto commented Apr 25, 2024

The defaults for the "Makefile" generator are:

  • "RelWithDebInfo" : -O2 -g -DNDEBUG
  • "Debug': -g

That differs from our flags, which are -O2 and -O1 -g for Linux respectively.

And CMake can change its defaults at any time (

However, it would be nice to be explicit about the used build type, for example, -DCMAKE_BUILD_TYPE=None.

@theuni
Copy link
Member Author

theuni commented Apr 25, 2024

Right, but a few more things to consider:

An upstream build could be following the docs and doing something like:

# Works correctly for both single and multi-config generators
target_compile_definitions(exe1 PRIVATE
  $<$<CONFIG:Debug>:DEBUG_BUILD>
)

In this case, we wouldn't currently pick up the extra debug opts.

The other thing to consider that is that we could be using
CMAKE_<LANG>_FLAGS_<CONFIG> for these options rather than CFLAGS/CXXFLAGS, potentially overriding the defaults you mentioned above but still being explicit about the build type.

Note that I don't necessarily believe these arguments and don't feel strongly either way, I just wanted to raise the discussion.

@laanwj
Copy link
Member

laanwj commented Apr 25, 2024

i think -O2 -g -DNDEBUG (RelWIthDebInfo) for the release makes sense, it means that the distributed binary will be optimized, with no extra runtime checks, but still get (as far as possible) descriptive debug information such as line numbers and symbols in case it's necessary for troubleshooting, getting a traceback, figuring out where some crash address comes from, and so on.

Edit: Oh, to not forget, debug metadata is also useful to figure out the details of differences between guix-built binaries, like where do they exactly come from. This is what i tend to most often use it for.

@DrahtBot
Copy link
Contributor

DrahtBot commented May 6, 2024

🐙 This pull request conflicts with the target branch and needs rebase.

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.

None yet

4 participants