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

meson.eclass */*: EMESON_BUILDTYPE & deduplicate some debug logic #35593

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

mjsir911
Copy link
Contributor

@mjsir911 mjsir911 commented Mar 2, 2024

Not sure if this PR is an overstep or if the commits are grouped together but this will help to potentially avoid issues like https://bugs.gentoo.org/925949

Also, EMESON_BUILDTYPE hasn't been used anywhere previously?

15:47 < mjsir911> question about EMESON_BUILDTYPE: is anything actually using it? Is this intended to be used in ebuilds or specified by users calling emerge? I'm a bit confused
15:48 <@sam_> I had no idea it existed until you mentioned it
15:48 <@sam_> or i forgot about it
15:48 <@sam_> i think the former though ;)

Of note is that commit d066871 introduces a change in the build process for some packages
eg glib that didn't previously set -Db_ndebug when use debug

@gentoo-bot
Copy link

Pull Request assignment

Submitter: @mjsir911
Areas affected: ebuilds, eclasses
Packages affected: dev-libs/efl, dev-libs/glib, dev-util/intel_clc, media-libs/mesa, media-libs/mesa-amber...

dev-libs/efl: @juippis
dev-libs/glib: @gentoo/gnome
dev-util/intel_clc: @gentoo/x11
media-libs/mesa: @gentoo/x11
media-libs/mesa-amber: @gentoo/x11
media-libs/rubberband: @gentoo/proaudio
sys-auth/elogind: @a17r
x11-base/xorg-server: @gentoo/x11
x11-wm/mutter: @gentoo/gnome

Linked bugs

No bugs to link found. If your pull request references any of the Gentoo bug reports, please add appropriate GLEP 66 tags to the commit message and request reassignment.

If you do not receive any reply to this pull request, please open or link a bug to attract the attention of maintainers.

Missing GCO sign-off

Please read the terms of Gentoo Certificate of Origin and acknowledge them by adding a sign-off to all your commits.


In order to force reassignment and/or bug reference scan, please append [please reassign] to the pull request title.

Docs: Code of ConductCopyright policy (expl.) ● DevmanualGitHub PRsProxy-maint guide

@gentoo-bot gentoo-bot added assigned PR successfully assigned to the package maintainer(s). no bug found No Bug/Closes found in the commits. no signoff One or more commits do not indicate GCO sign-off. labels Mar 2, 2024
@mjsir911 mjsir911 marked this pull request as draft March 2, 2024 00:44
@mjsir911 mjsir911 marked this pull request as ready for review March 2, 2024 00:50
@gentoo-repo-qa-bot
Copy link
Collaborator

Pull request CI report

Report generated at: 2024-03-02 00:53 UTC
Newest commit scanned: a064f2f
Status: ✅ good

There are existing issues already. Please look into the report to make sure none of them affect the packages in question:
https://qa-reports.gentoo.org/output/gentoo-ci/ce470f0664/output.html

@gentoo-repo-qa-bot
Copy link
Collaborator

Pull request CI report

Report generated at: 2024-03-02 01:07 UTC
Newest commit scanned: aba9921
Status: ✅ good

There are existing issues already. Please look into the report to make sure none of them affect the packages in question:
https://qa-reports.gentoo.org/output/gentoo-ci/faf011a22a/output.html

@thesamesam
Copy link
Member

cc @eli-schwartz

Comment on lines 189 to 190
use debug && EMESON_BUILDTYPE=debug

local emesonargs=(
-Dbuildtype=$(usex debug debug plain)
Copy link
Contributor

Choose a reason for hiding this comment

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

This first commit makes sense. People really should have used it for consistency, indeed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can drop the two other commits and/or make a new PR and/or would it be possible to just merge in this (and the glib_debug) commit?

@@ -368,7 +368,8 @@ setup_meson_src_configure() {


if in_iuse debug; then
use debug && EMESON_BUILDTYPE=debug
MESONARGS+=( -Db_ndebug=$(usex debug false true) )
Copy link
Contributor

Choose a reason for hiding this comment

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

So this is setting -DNDEBUG now, for all ebuilds that have a USE="-debug".

How is this handled for other build systems?

Copy link
Member

@thesamesam thesamesam Mar 3, 2024

Choose a reason for hiding this comment

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

... it's very painful: https://bugs.gentoo.org/884791. We used to pass -DNDEBUG in cmake-utils.eclass and cmake.eclass for a time, then we stopped, and every so often, we get broken software as a result..

Comment on lines 370 to 373
if in_iuse debug; then
use debug && EMESON_BUILDTYPE=debug
fi
Copy link
Contributor

Choose a reason for hiding this comment

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

Are there meson.eclass using ebuilds that define IUSE=debug but didn't set the buildtype?

Note in general this feels a bit weird. The purpose of IUSE=debug is:

Enable extra debug codepaths, like asserts and extra output. If you want to get meaningful backtraces see https://wiki.gentoo.org/wiki/Project:Quality_Assurance/Backtraces

but the debug buildtype in meson simply adds things like -g, which is specifically intended to not be the same thing.

This is why ebuilds declare it locally if they need it, typically because the meson.build files themselves check the value of the "debug" buildtype to enable additional "debug codepaths, like asserts and extra output".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The -Dn_debug flag specifically enables/disables asserts:
https://mesonbuild.com/Builtin-options.html#base-options

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To answer the question though, yes there are packages that don't set the buildtype that define IUSE=debug:

  • app-emulation/dxvk
  • app-emulation/vkd3d-proton
  • dev-cpp/glibmm
  • dev-db/postgresql
  • games-emulation/dosbox-staging
  • gnome-base/gnome-control-center
  • gnome-base/gnome-desktop
  • gnome-base/gnome-settings-daemon
  • gnome-extra/nautilus-sendto
  • gui-libs/vte
  • media-gfx/geeqie
  • media-libs/gegl
  • media-libs/harfbuzz
  • media-libs/opus
  • media-libs/rlottie
  • net-irc/hexchat
  • net-libs/gnome-online-accounts
  • net-libs/libsrtp
  • net-misc/eventd
  • net-misc/networkmanager
  • net-p2p/transmission-remote-gtk
  • sys-apps/dbus
  • sys-apps/kmscon
  • sys-apps/openrc
  • sys-block/open-iscsi
  • www-client/elinks
  • x11-libs/cairo
  • x11-libs/pango
  • x11-libs/vte

Copy link
Contributor

Choose a reason for hiding this comment

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

The -Dn_debug flag specifically enables/disables asserts:
https://mesonbuild.com/Builtin-options.html#base-options

Yes, that would be why this review comment is NOT about the ndebug option, but about the buildtype option.

(It is b_ndebug, not n_debug, however.)

dev-libs/glib/glib-2.78.4-r1.ebuild Show resolved Hide resolved
@gentoo-repo-qa-bot
Copy link
Collaborator

Pull request CI report

Report generated at: 2024-03-03 04:04 UTC
Newest commit scanned: 78312c6
Status: ✅ good

There are existing issues already. Please look into the report to make sure none of them affect the packages in question:
https://qa-reports.gentoo.org/output/gentoo-ci/b4fe1b5724/output.html

@mjsir911
Copy link
Contributor Author

@eli-schwartz would it make sense to re-push the PR with only the first two commits?

@eli-schwartz
Copy link
Contributor

I think so, yes. They are uncontroversial, at least.

For the first commit in particular I think it makes sense to provide a more detailed commit message explaining why the change was made.

glib has a special flag signalling a debug build, without this, G_ENABLE_DEBUG
is only set iff --debug=true AND --optimization={0,g}
(which, admittedly, -Dbuildtype=debug *does* do. It's just better to be
explicit about these things)

Signed-off-by: Marco Sirabella <marco@sirabella.org>
Signed-off-by: Marco Sirabella <marco@sirabella.org>
@gentoo-repo-qa-bot
Copy link
Collaborator

Pull request CI report

Report generated at: 2024-03-14 15:28 UTC
Newest commit scanned: dcd9d1a
Status: ✅ good

There are existing issues already. Please look into the report to make sure none of them affect the packages in question:
https://qa-reports.gentoo.org/output/gentoo-ci/acd3fa1ffe/output.html

Copy link
Contributor

@eli-schwartz eli-schwartz left a comment

Choose a reason for hiding this comment

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

Sorry, forgot to actually respond.

@eli-schwartz
Copy link
Contributor

For batching reasons I opened a mega-PR above ^^. It cherry-picks these commits directly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
assigned PR successfully assigned to the package maintainer(s). no bug found No Bug/Closes found in the commits. no signoff One or more commits do not indicate GCO sign-off.
Projects
None yet
5 participants