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

Add b_ndebug=auto because b_ndebug=if-release is misleading #8308

Open
dreamer opened this issue Feb 5, 2021 · 2 comments
Open

Add b_ndebug=auto because b_ndebug=if-release is misleading #8308

dreamer opened this issue Feb 5, 2021 · 2 comments

Comments

@dreamer
Copy link
Contributor

dreamer commented Feb 5, 2021

This is not a big problem but rather a papercut issue that will keep hurting users as the time moves on - perhaps it's time to design a way out of it?

Describe the bug

if-release name is confusing, that's the gist of it.

Background:

I am in the process of porting the project to Meson (dosbox-staging/dosbox-staging#854) - while working on it, I discovered that NDEBUG is not automatically turned on for release builds - we don't want to define it in release because we rely heavily on C asserts in code for detecting potential bugs - we expect to catch these asserts during testing and don't want them affecting end-users of our emulator. Until now (with autoconf buildsystem) our recommendation to packagers was to pass NDEBUG flag explicitly because some (all?) Linux distributions don't have this flag included in their default CPP flags (of course, some packagers promptly ignored our recommendation to the detriment of end users).

OK, so soon I discovered that there's b_ndebug=if-release option, which works perfectly for this usecase (thank you for including it! :)). However due to if-release name, few days after adding it I was already confused if this option works for plain builds or not.

Then I discovered that in meson < 0.51.0 the behaviour was different and plain builds required explicit NDEBUG CPP flag…

Then the first packager testing our meson buildsystem got confused about how NDEBUG, if-release, and plain interoperate… then my co-maintainer got confused as well… It seems like a pattern ;)

Suggested behavior
Right now this option works this way:

Base options            Default Value  Possible Values                                               Description
------------            -------------  ---------------                                               -----------
b_ndebug                false          [true, false, if-release]                                     Disable asserts

if-release turns it to true for buildtype=release and buildtype=plain (and maybe others? I don't know)

I suggest changing it to:

Base options            Default Value  Possible Values                                               Description
------------            -------------  ---------------                                               -----------
b_ndebug                false          [true, false, if-release, auto, release-only]                 Disable asserts

With

  • auto behaving exactly the same way as if-release does now.
  • release-only behaving like if-release worked in meson < 0.51.0 (other names could be ok, just please - nothing with if- prefix… iff-release might work)

And then deprecate usage of if-release after some time.

I think this solution would cover all the cases.

Similar bug reports

There's #2566 already - however I don't argue for changing the default. I believe each OS/distro had their reasons in designing the default set of flags they use, which probably means there are C/C++ projects out there that prefer to keep NDEBUG flag undefined for end-users (I imagine some projects might want to do it for security-related reasons). C compilers nor libc implementations don't define this flag automatically depending on other compiler flags, and I think such honest behaviour is fine in C world.

@kcgen
Copy link

kcgen commented Feb 5, 2021

Alternative KISS suggestion (from confused co-maintainer 😅 ):

Base options    Default Value  Possible Values
------------    -------------  ---------------
b_ndebug        false          [true, false, if-not-debug]

Where:

  • if-not-debug disables assertions in all build-types not prefixed with debug such as release, plain, and minsize. Assertions will be retained in all debug-prefixed build-types, which are currently debug and debugoptimized. This pattern will be maintained for future build-types as well.
  • if-release is deprecated, and users of b_ndebug=if-release are given a helpful message to move to b_ndebug=if-not-debug, as if-release will be dropped some releases later.

@dreamer
Copy link
Contributor Author

dreamer commented Feb 5, 2021

I don't like the names starting with if- because they read like some kind of special, untyped expression (they are only enum values, but are being used inside a string). iff- would work better, but personally I like auto the best, because it gives some flexibility going forward (if new build types will be added in the future).

bruchar1 added a commit to bruchar1/meson that referenced this issue Dec 1, 2023
bruchar1 added a commit to bruchar1/meson that referenced this issue Dec 2, 2023
bruchar1 added a commit to bruchar1/meson that referenced this issue Dec 5, 2023
jpakkane pushed a commit that referenced this issue Dec 23, 2023
This is a first step to make `buildtype` a true alias of `debug` and
`optimization` options.

See #10808.

Relates to:
- #11645
- #12096
- #5920
- #5814
- #8220
- #8493
- #9540
- #10487
- #12265
- #8308
- #8214
- #7194
- #11732
gerioldman pushed a commit to gerioldman/meson that referenced this issue Jul 2, 2024
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

No branches or pull requests

2 participants