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

build: do not include debug symbols by default #410

Closed
wants to merge 2 commits into from
Closed

build: do not include debug symbols by default #410

wants to merge 2 commits into from

Conversation

dougsland
Copy link
Contributor

@dougsland dougsland commented Jul 31, 2023

By default debug symbols are included into the hirte binaries. It should included only for development and tests.

GitHub-Issue: #409

Signed-off-by: Douglas Schilling Landgraf [email protected]

By default debug symbols are included into the hirte
binaries. It should included only for development and tests.

GitHub-Issue: #409

Signed-off-by: Douglas Schilling Landgraf <[email protected]>
Just a quick target to use a sed command to include the
debug sysmbols and call build target (meson).

Signed-off-by: Douglas Schilling Landgraf <[email protected]>
Comment on lines +62 to +64
$(MAKE) clean
@sed -i 's/debug=false/debug=true/g' meson.build
$(MAKE) build

Choose a reason for hiding this comment

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

Please avoid running sed on the files checked into git -- just pass the -Ddebug=true option to meson setup, perhaps by making the make build rule utilize a $(MESONARGS) variable or something and setting that here.

Copy link
Member

Choose a reason for hiding this comment

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

I agree. Also, wouldn't it be sufficient to use meson configure? For example:

# disable debug
meson configure builddir -Ddebug=false
# enable debug
meson configure builddir -Ddebug=true

This should let you switch without the need to re-setup the project. For the make users, we can also add a new target for it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@engelmi that is super nice.

@engelmi
Copy link
Member

engelmi commented Aug 1, 2023

The changes in PR #408 for describing how to enable debugging depend on this PR. Could you move the changes from #408 to this one? This would make it a bit easier to review. You can use a distinct commit, of course.

@engelmi
Copy link
Member

engelmi commented Aug 1, 2023

FYI:
This PR should issue #409, so it should be closed as soon as this PR is merged, right? In that case, please change the reference in the description from GitHub-Issue: https://github.com/containers/hirte/issues/409 to something with "fixes", e.g. This PR fixes https://github.com/containers/hirte/issues/409 - otherwise GitHub does not create such dependency and we have to close the issue manually.

@engelmi
Copy link
Member

engelmi commented Aug 1, 2023

Thinking about it, the direct meson build of hirte is (or should be) only used by developers on the project - and for those it makes a lot of sense to have the debug symbols included by default. Everybody else should get hirte via the RPMs we provide. So if we want to exclude the debug symbols, we should do this in the hirte.spec - in L176 we already statically override options for meson. Not sure, though, if it makes sense since the RPM build also includes debug symbols as far as I know.
In conclusion, I wouldn't change the default value for the debug symbols in meson.

@mwperina
Copy link
Member

mwperina commented Aug 1, 2023

Several notes:

  1. hirte builds created according to README.developer.md should be used for development only and that's why I think debug should be enabled by default
  2. hirte installed from RPMs is meant for production and the binaries from RPM should not include debug symbols. When you look at the RPM build meson build is executed with following parameters:
/usr/bin/meson --buildtype=plain --prefix=/usr --libdir=/usr/lib64 --libexecdir=/usr/libexec --bindir=/usr/bin --sbindir=/usr/sbin --includedir=/usr/include --datadir=/usr/share --mandir=/usr/share/man --infodir=/usr/share/info --localedir=/usr/share/locale --sysconfdir=/etc --localstatedir=/var --sharedstatedir=/var/lib --wrap-mode=nodownload --auto-features=enabled . redhat-linux-build -Dapi_bus=system

The important option is --buildtype=plain which should turn off debug symbols.
But if you look further you can see that that gcc is invoked with -g option that enables storing debug info. That's why because RPM build default flags for the platform enables generating debug information:

CFLAGS='-O2 -flto=auto -ffat-lto-objects -fexceptions -g -grecord-gcc-switches -pipe -Wall -Werror=format-security -Wp,-D_FORTIFY_SOURCE=2 -Wp,-D_GLIBCXX_ASSERTIONS -specs=/usr/lib/rpm/redhat/redhat-hardened-cc1 -fstack-protector-strong -specs=/usr/lib/rpm/redhat/redhat-annobin-cc1  -m64 -march=x86-64-v2 -mtune=generic -fasynchronous-unwind-tables -fstack-clash-protection -fcf-protection'

But after finishing the build the RPM process extracts debug information from the binaries using find-debuginfo.sh and stores debug information in a standalone package (for example hirte-debuginfo-...rpm).

So my comments:

  1. RPM builds always contain binaries without debug symbols (unless you explicitly install debuginfo package), because RPM builds are meant for production
  2. RPM builds are not affected by enabling/disabling debug in meson.build, but as hirte binaries created by direct meson invocation are primarily targeted by developers, they should have debug information included by default. The only question is if we do it by setting debug: true in meson.build or changing our developer documentation to suggest using meson compile --build-type=debug by default.

@dougsland
Copy link
Contributor Author

Thinking about it, the direct meson build of hirte is (or should be) only used by developers on the project - and > for those it makes a lot of sense to have the debug symbols included by default. Everybody else should get
hirte via the RPMs we provide. So if we want to exclude the debug symbols, we should do this in the hirte.spec

  • in L176 we already statically override
    options for meson. Not sure, though, if it makes sense since the RPM build also includes debug symbols as far
    as I know. In conclusion, I wouldn't change the default value for the debug symbols in meson.

Let's imagine another project, like kernel linux (we can pick up another established project), it's C and also shipped in rpm. Still, not enables by default the debug objects. To be honest, it's little weird to see debug=True in the source code released and do a change in the spec. Every distro must be notified to do such change.

release v0.4.0:
https://github.com/containers/hirte/archive/refs/tags/v0.4.0.tar.gz

@dougsland
Copy link
Contributor Author

Several notes:

  1. hirte builds created according to README.developer.md should be used for development only and that's why I think debug should be enabled by default
  2. hirte installed from RPMs is meant for production and the binaries from RPM should not include debug symbols. When you look at the RPM build meson build is executed with following parameters:
/usr/bin/meson --buildtype=plain --prefix=/usr --libdir=/usr/lib64 --libexecdir=/usr/libexec --bindir=/usr/bin --sbindir=/usr/sbin --includedir=/usr/include --datadir=/usr/share --mandir=/usr/share/man --infodir=/usr/share/info --localedir=/usr/share/locale --sysconfdir=/etc --localstatedir=/var --sharedstatedir=/var/lib --wrap-mode=nodownload --auto-features=enabled . redhat-linux-build -Dapi_bus=system

The important option is --buildtype=plain which should turn off debug symbols. But if you look further you can see that that gcc is invoked with -g option that enables storing debug info. That's why because RPM build default flags for the platform enables generating debug information:

CFLAGS='-O2 -flto=auto -ffat-lto-objects -fexceptions -g -grecord-gcc-switches -pipe -Wall -Werror=format-security -Wp,-D_FORTIFY_SOURCE=2 -Wp,-D_GLIBCXX_ASSERTIONS -specs=/usr/lib/rpm/redhat/redhat-hardened-cc1 -fstack-protector-strong -specs=/usr/lib/rpm/redhat/redhat-annobin-cc1  -m64 -march=x86-64-v2 -mtune=generic -fasynchronous-unwind-tables -fstack-clash-protection -fcf-protection'

But after finishing the build the RPM process extracts debug information from the binaries using find
-debuginfo.sh
and
stores debug information in a standalone package (for example hirte-debuginfo-...rpm).

So my comments:

  1. RPM builds always contain binaries without debug symbols (unless you explicitly install debuginfo package),
    because RPM builds are meant for production
  2. RPM builds are not affected by enabling/disabling debug in meson.build, but as hirte binaries created by
    direct meson invocation are primarily targeted by developers, they should have debug information included by
    default. The only question is if we do it by setting debug: true in meson.build or changing our developer
    documentation to suggest using meson compile --build-type=debug by default.

Nice explanation but it seems very distro specific solution for a open source project.

@engelmi
Copy link
Member

engelmi commented Aug 1, 2023

Nice explanation but it seems very distro specific solution for a open source project.

By distro specific you mean RPM? Currently, we'll package hirte only for it (might change in the future).

Let's imagine another project, like kernel linux (we can pick up another established project), it's C and also shipped in rpm. Still, not enables by default the debug objects. To be honest, it's little weird to see debug=True in the source code released and do a change in the spec. Every distro must be notified to do such change.

Again, using meson directly is for developers and usually debug - so having debug symbols as a default just simplifies developer life. The final product is built using a RPM spec (or whatever target might be added) and for this we can set the debug symbols off if needed. Using established projects as a reference is good, but I'd not do everything the same and rather tailor solutions/approaches to our project.

@dougsland dougsland closed this Aug 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants