Created attachment 267375 [details] Patch for meson.mk This is already true for other frameworks and release builds in general so do it also for Meson builds by default
Hum, on one hand this goes in line with CMake and makes sense overall, but on other hand it isn't Meson default for some reason. In the end, I think we should merge this, but I'd like to hear other people opinions.
I'll merge this for now given its been open for almost two months and we'll see if it needs to be adjusted later on
There was no approval given. Could you please instead contact Meson's devs and ask for the rationale for this default?
If you want it that way one could also say that there's no feedback either... Either way, https://github.com/mesonbuild/meson/commit/d88bf0eb80e2531a8017de4efd4eb02f1e3081ec
(In reply to Daniel Engberg from comment #4) That code only affects cases when b_ndebug=if-release or b_ndebug=true, which isn't default. My question is why Meson defaults to b_ndebug=false.
Feel free to assist
(In reply to Gleb Popov from comment #5) According to the docs at least (https://mesonbuild.com/Builtin-options.html), b_ndebug defaults to false. "Details for buildtype" in that documentation says "debug" should be false for "release" builds. But that (debug=false) doesn't explicitly mean that b_ndebug gets set to true (at least not anywhere that I could see in the doc). In fact, there is the following statement in that doc: "Note that -Ddebug=false does not cause the compiler preprocessor macro NDEBUG to be defined. The macro can be defined using the base option b_ndebug, described below." Later in that doc in "Base options", the default for b_ndebug is false. And the possible settings are false, true and if-release. Though this is not a direct question to meson upstream, I think this answers Gleb's question in a RTFM context. So assuming that documentation (untested by me for correctness), Daniel's patch seems mostly right. Or we could explicitly always set b_ndebug=if-release outside the .if .. .elif .. .else (rather only explicitly setting b_ndebug in the .else part as in Daniel's patch to meson.mk). Then, b_ndebug will be set according to the intent of the if-release setting - true if a release buildtype or false if it is one of the WITH_DEBUG* cases (where buildtype is set to debug or debugoptimized). I think if someone is building WITH_DEBUG or WITH_DEBUGINFO, it is appropriate and expected (POLA-wise) for assertions to be enabled (i.e., b_ndebug=false). I think using if-release does that for the user automatically. In other words: ======== --- a/Mk/Uses/meson.mk +++ b/Mk/Uses/meson.mk @@ -78,6 +78,7 @@ CONFIGURE_ARGS+= --buildtype release \ --optimization plain \ --strip . endif +MESON_ARGS+= -Db_ndebug=if-release . for _bool in true false enabled disabled . if defined(MESON_${_bool:tu}) ======== Daniel, do you have a fairly easy (even if contrived) reproduction case for a particular port that exhibits the problem (not getting built with disabled asserts)?
(In reply to John Hein from comment #7) > Though this is not a direct question to meson upstream, I think this answers Gleb's question in a RTFM context. No, my question is why Meson defaults to "false" rather than "if-release". If there is a reason why asserts are always enabled by default, then we should take it into consideration before making Ports deviate from upstream's default. > Daniel, do you have a fairly easy (even if contrived) reproduction case for a particular port that exhibits the problem (not getting built with disabled asserts)? Just building a port is enough only for static assertions. In my experience most assertions are run-time, so it is needed to actually run the code. Anyways, right now assertions are enabled, so disabling them should not break anything (unless a bug assert() usages that contain important code inside).
(In reply to Gleb Popov from comment #8) For the same reason as it defaults to debug for buildtype?
(In reply to Daniel Engberg from comment #9) > For the same reason as it defaults to debug for buildtype? And what's that reason? No need to talk in riddles. Meanwhile, I created a question on SO: https://stackoverflow.com/questions/79907793/why-meson-unconditionally-defaults-the-b-ndebug-option-to-false
All right, I think we should set b_ndebug to if-release unconditionally, like John suggested.
Which is the same thing in practice the end as it's within the release block
So can we merge this or is it yet another glacial pace project?
Yes, the variant with "if-release" outside any conditional blocks.
(In reply to Gleb Popov from comment #14) Please commit whatever variant is preferred.
(In reply to Daniel Engberg from comment #15) To clarify, I don't see the potential benefit of "polluting" builds by adding more global flags given that we're already clear of the purpose with release builds.
(In reply to Daniel Engberg from comment #15) I didn't need this change in the first place, it was you who started this PR. If you do not agree to commit your change with adjustments I asked for, then let's close this PR. Or you can bug someone else from desktop@ with this, I'm in no way trying to block your work.
(In reply to Gleb Popov from comment #17) I'm asking what's the point of polluting with more global flags
(In reply to Daniel Engberg from comment #18) I see how "globalness" of a flag hurts. To the contrary, I find global unconditional knobs clearer and easier to process in the brain. Looking at the single MESON_ARGS= -Db_ndebug=if-release line, it gives me an understanding how it affects all ports in all build modes. But when I see .ifndef WITH_DEBUG MESON_ARGS= -Db_ndebug=false ... I also start looking for an .else branch just in case it also has a corresponding setting.
(In reply to Daniel Engberg from comment #1) (In reply to John Hein from comment #4) Both the patch setting b_ndebug=true for "release" builds and b_ndebug=if-release for any build result in x11-toolkits/vte3 failing in 'make configure'. It turns out that the vte3 meson.build EXPECTS assertions to be enabled: ====== # Asserts must not be disabled assert(get_option('b_ndebug') == 'false', 'assertions may not be disabled') ====== Just added MESON_ARGS+=-Db_ndebug=false in x11-toolkits/vte3/Makefile is not enough (unless you add it AFTER the .include <bsd.port.mk>). That's because currently Mk/Uses/meson.mk adds to MESON_ARGS, and if meson.mk defines -Db_ndebug=<whatever>, it will take precedence as it is the later command line argument for -Db_ndebug - that is, it overrides whatever vte3/Makefile might set for the b_ndebug parameter (or any of the MESON_ARGS set in meson.mk). Clearly, the right thing for vte3 to do is explicitly specify that b_ndebug=false. In this case, the vte3 port "knows better" than any default value that the global meson.mk might choose to use as a default for b_ndebug. Whether it is "right" for vte3 to require asserts is debatable, but it's the path they chose. If someone wants to engage with vte3 upstream about this, feel free. But that is a side topic. Someone could have that discussion with vte3 upstream. Right now, vte3 requires that asserts be enabled. Setting MESON_ARGS in meson.mk... ====== So anything that is added to meson.mk to globally define an "ndebug" policy for FreeBSD ports will have to allow any such setting by an individual port to take precedence over anything that meson.mk might want to set as a nominal default for other (perhaps most other) ports. Nothing else in Uses/meson.mk currently sets or adds to MESON_ARGS. And the current "documentation" (in the header block) in meson.mk says MESON_ARGS is a variable "for ports": # Variables for ports: # MESON_ARGS - Arguments passed to meson # format: -Denable_foo=true Maybe rather than explicitly adding -Db_ndebug to MESON_ARGS in meson.mk, the right thing is to set MESON_B_NDEBUG?=<something> in meson.mk where <something> may be different for the cases where WITH_DEBUG* is defined or not. I read the above comment as MESON_ARGS is for the port to own, not for meson.mk to muck with. ndebug wars and perils of 'auto'... ====== There is some dissonance upstream cautioning about meson being too "cmake-like" regarding setting a bunch of parameters based on buildtype. In contrast to that opinion, however, it allowed "if-release" to exist (which IS "cmake-like" - and did cause some confusion and misfires in meson - see below). See https://github.com/mesonbuild/meson/discussions/12720#discussioncomment-8069301 - although if you read more in various threads of discussion, that is not the only place where there is some angst about this topic. But the gist is that the "buildtypes" define "debug" and "optimization" levels, and overloading the buildtype to also dictate how b_ndebug is out of scope (although like I said, using "if-release" implements something that is in some ways the opposite of that thinking). Also see https://github.com/mesonbuild/meson/pull/1896 which also mirrors some of the same discussion points here - and ensuing fallout. Notably, but not exclusively, reference the comments https://github.com/mesonbuild/meson/pull/1896#issuecomment-310011593 and https://github.com/mesonbuild/meson/pull/1896#issuecomment-310342923 And: https://github.com/MusicPlayerDaemon/ncmpc/pull/23#issuecomment-409128775 https://github.com/mesonbuild/meson/issues/2566 https://github.com/mesonbuild/meson/issues/3258 https://github.com/mesonbuild/meson/pull/3274 So maybe we don't rigidly define b_ndebug to true or if-release (or false, for that matter). But use ?= to allow ports to override the default in meson.mk. In other words, don't force b_ndebug to be didactically defined depending on WITH_DEBUG* or not (which currently meson.mk treats as the same as being tied to a particular meson "buildtype"). In other words, either: ====== --- a/Mk/Uses/meson.mk +++ b/Mk/Uses/meson.mk @@ -71,13 +71,17 @@ INSTALL_TARGET= install # should we have strip separate from WITH_DEBUG? . if defined(WITH_DEBUG) CONFIGURE_ARGS+= --buildtype debug +MESON_NDEBUG?= false . elif defined(WITH_DEBUGINFO) CONFIGURE_ARGS+= --buildtype debugoptimized +MESON_NDEBUG?= false . else CONFIGURE_ARGS+= --buildtype release \ --optimization plain \ --strip +MESON_NDEBUG?= true . endif +MESON_ARGS+= -Db_ndebug=${MESON_NDEBUG} . for _bool in true false enabled disabled . if defined(MESON_${_bool:tu}) ====== or: ====== --- a/Mk/Uses/meson.mk +++ b/Mk/Uses/meson.mk @@ -78,6 +78,8 @@ CONFIGURE_ARGS+= --buildtype release \ --optimization plain \ --strip . endif +MESON_NDEBUG?= if-release +MESON_ARGS+= -Db_ndebug=${MESON_NDEBUG} . for _bool in true false enabled disabled . if defined(MESON_${_bool:tu}) ====== I'm not sure I feel strongly that either one is more correct. But one of them is shorter ; ) And maybe neither is optimal... Both of them will require at least one port (vte3) to define MESON_NDEBUG=false. Or you could have meson.mk add b_ndebug setting to CONFIGURE_ARGS before MESON_ARGS are added to CONFIGURE_ARGS. That would let individual ports override b_ndebug in MESON_ARGS. That is simple enough. One could also add support for a "ndebug=<true|if-release>" arg to USES=meson instead and the absence of the "ndebug" arg implies b_ndebug=false (mirroring the current upstream default). So: USES=meson # -Db_ndebug=false (upstream default) USES=meson:ndebug # -Db_ndebug=true USES=meson:ndebug=if-release # -Dndebug=if-release I think I like using an arg for USES=meson better than the above two options in this comment (with MESON_NDEBUG). I know Daniel's main thinking (the reason for this bug) is to build most ports with b_ndebug=true. I'm not sure whether that seems more right or less right than letting the upstream default for b_ndebug just rule the day as it stands now - and [importantly] letting individual ports set it explicitly if desired / needed (like the current incarnation of vte3, such as it is).
(In reply to John Hein from comment #20) I think you're overengineering with MESON_NDEBUG. All we need is to make MESON_ARGS specified in Makefile to take precedence over args added by meson.mk. This is done simply by putting ${MESON_FLAGS} at the end of the command line.