fix(core,developer): use `NDEBUG` flag to disable assertions in release build #12715

mcdurdin · 2024-11-27T08:35:29Z

This disables assertions in release builds for all meson-built projects.

See d71cb56 for deeper discussion on the yak shave that impacted all the unit tests.

Fixes: #12619
Fixes: #12453 (by implication)

…or VC++ This disables assertions in release builds in Windows only. Other platforms may have different outcomes. Fixes: #12619

keymanapp-test-bot · 2024-11-27T08:35:33Z

User Test Results

Test specification and instructions

User tests are not required

Test Artifacts

Linux

mcdurdin · 2024-11-27T08:37:09Z

common/windows/delphi/general/KeymanPaths.pas

+{$IFDEF DEBUG}
+    configuration := 'debug';
+{$ELSE}
+    configuration := 'release';
+{$ENDIF}
+    Exit(keyman_root + 'core\build\x86\'+configuration+'\src\' + Filename);


Allows us to test both release and debug builds of Core in the Debugger when running within the keymanapp/keyman repo

mcdurdin · 2024-11-27T08:59:01Z

Hmm, unit tests have very unexpectedly failed for Test: Keyman Core - Windows (Common) with this minimal change. More digging needed, tomorrow.

ermshiperete · 2024-11-27T11:02:09Z

Ah, you made the change in resources/build/meson/standard.meson.build and so it gets picked up by all the platform builds. I don't think anything else is needed on the Linux side.

ermshiperete

LGTM

mcdurdin · 2024-11-27T13:55:57Z

Ah, you made the change in resources/build/meson/standard.meson.build and so it gets picked up by all the platform builds. I don't think anything else is needed on the Linux side.

I constrained the check to msvc though

ermshiperete · 2024-11-27T15:46:49Z

Ah, right. For Linux at least it's the same, so I think we should move it outside of the msvc section.

rc-swag

LGTM

… on all platforms

Had a real yak shave this morning with disabling assertions in release builds in our C/C++ code. It turns out that our unit tests use `assert()` which we intended to use from `test_assert.h`, but in some cases `cassert` or `assert.h` had been #included after `test_assert.h`, overriding our special `assert()` macro. The chain of includes is somewhat hard to puzzle out -- it's often buried several levels deep. This meant that a release build would drop all test assertions, meaning most tests passed, unsurprisingly, as there were no assertions left to fail ... but some tests failed with crashes because we optimized out important lines such as `assert(some_important_function())`. I was quite unhappy with this fragility, so I have opted to rename `assert()` to `test_assert()` in all of our home-grown C/C++ unit tests, which further highlighted unit tests which were only using the C/C++ `assert()` and not ours, so then had to figure out which unit test executables needed to have `test_assert` added, and then ... then ... discovered a bug in `test_color.h`, where we were #including `io.h`/`unistd.h` inside a `namespace console_color {}` block, which just happened to be the first ref to those beautiful headers, and thus (because `#pragma once`) meant that useful little functions like `access()` were no longer accessible to us in the global namespace. I have also audited Every Single Call to `assert()` to verify that we do not do Important Work inside the parentheses, and, apart from those offending unit tests, now resolved with `test_assert()`, it looks like all is good. I would like to present one very well-shaved yak in this commit. Fixes: #12619

…ions-vcwin-release-build

mcdurdin · 2024-11-28T02:54:33Z

core/tests/meson.build

@@ -7,7 +7,7 @@
 #

 # Note: this version of cmpfiles ignores line endings, which is better for platform independence
-cmpfiles = ['-c', 'import sys; a = open(sys.argv[1], \'r\').read(); b = open(sys.argv[2], \'r\').read(); exit(not (a==b))']
+cmpfiles = ['-c', 'import sys; a = open(sys.argv[1], \'r\').read(); b = open(sys.argv[2], \'r\').read(); sys.exit(not (a==b))']


Unrelated ... but maybe due to my python version? In any case sys.exit is probably safer

big changes due to assert() in unit tests

mcdurdin · 2024-11-28T03:10:05Z

The yak shave isn't over:

  ../../../src/action.cpp: In function ‘bool km::core::action_item_list_to_actions_object(const km_core_action_item*, km_core_actions*)’:
  ../../../src/action.cpp:64:20: error: variable ‘last_context_item’ set but not used [-Werror=unused-but-set-variable]
     64 |               auto last_context_item = output.back();
        |                    ^~~~~~~~~~~~~~~~~
  ../../../src/action.cpp:74:20: error: variable ‘last_context_item’ set but not used [-Werror=unused-but-set-variable]
     74 |               auto last_context_item = output.back();
        |                    ^~~~~~~~~~~~~~~~~
  cc1plus: all warnings being treated as errors

Presumably last_context_item is only used in an assertion ... so need to #ifdef it away.

SabineSIL · 2024-11-28T03:28:44Z

..changing of assert_failed to test_assert_failed looks good in all files;
... can`t say much about_ ( but it seem OK)

removing libraries in common/include/test_color.h
changes in common/windows/delphi/general/KeymanPaths.pas
using sys.exit instead of exit in core/tests/meson.build

darcywong00 · 2024-11-28T03:50:36Z

Is the discussion on d71cb56 the kind we want to summarize into /core/docs/internal/ ?

Particularly the fragility of assert() and tests

mcdurdin · 2024-11-28T03:55:15Z

Is the discussion on d71cb56 the kind we want to summarize into /core/docs/internal/ ?

Particularly the fragility of assert() and tests

We could add something to our test wiki; it's not really internal Keyman-relevant documentation, more a trap-for-young-players-in-c++ (i.e. how to use tools properly)

darcywong00

questions about ldml_*.cpp

core/src/ldml/ldml_markers.cpp

darcywong00

lgtm

rc-swag

lgtm
re-review

…ions-vcwin-release-build

keyman-server · 2024-12-02T18:03:34Z

Changes in this pull request will be available for download in Keyman version 18.0.150-alpha

fix(core): use NDEBUG flag to disable assertions in release build f…

18d00c7

…or VC++ This disables assertions in release builds in Windows only. Other platforms may have different outcomes. Fixes: #12619

mcdurdin requested review from jahorton, srl295 and rc-swag as code owners November 27, 2024 08:35

keymanapp-test-bot bot added this to the A18S16 milestone Nov 27, 2024

github-actions bot added common/ common/resources/ Build infrastructure core/ Keyman Core fix labels Nov 27, 2024

mcdurdin commented Nov 27, 2024

View reviewed changes

ermshiperete previously approved these changes Nov 27, 2024

View reviewed changes

ermshiperete mentioned this pull request Nov 27, 2024

fix(linux): disable assertions in release builds of ibus-keyman #12725

Merged

rc-swag previously approved these changes Nov 28, 2024

View reviewed changes

mcdurdin added 2 commits November 28, 2024 09:41

chore(common): define NDEBUG for release builds to disable assertions…

8c4a835

… on all platforms

mcdurdin requested review from darcywong00, SabineSIL and markcsinclair as code owners November 28, 2024 02:46

chore(core): Merge branch 'master' into fix/core/12619-disable-assert…

e640caa

…ions-vcwin-release-build

github-actions bot added developer/ developer/compilers/ labels Nov 28, 2024

mcdurdin commented Nov 28, 2024

View reviewed changes

mcdurdin changed the title ~~fix(core): use NDEBUG flag to disable assertions in release build for VC++~~ fix(core,developer): use NDEBUG flag to disable assertions in release build Nov 28, 2024

mcdurdin added 2 commits November 28, 2024 10:12

chore(developer): ifdef variables only used in assertions

e2b8222

chore(developer): ifdef variables only used in assertions

b9b4ad4

chore(developer): ifdef variables only used in assertions

bcecedc

chore(linux): include common test source files

faf479c

github-actions bot added linux/ linux/engine/ labels Nov 28, 2024

chore(core): ifdef variables only used in assertions

36ddd79

darcywong00 reviewed Nov 28, 2024

View reviewed changes

core/src/ldml/ldml_markers.cpp Show resolved Hide resolved

mcdurdin added 2 commits November 28, 2024 11:56

chore(core): typo in meson

ffae5ba

chore(core): update assertion unit test for release builds

b0e87cb

darcywong00 approved these changes Nov 28, 2024

View reviewed changes

rc-swag approved these changes Nov 29, 2024

View reviewed changes

srl295 approved these changes Nov 29, 2024

View reviewed changes

mcdurdin added 2 commits December 1, 2024 16:17

chore(core): Merge branch 'master' into fix/core/12619-disable-assert…

9a01ac1

…ions-vcwin-release-build

chore(core): build parameters

f97a19d

mcdurdin merged commit f747984 into master Dec 2, 2024
28 checks passed

mcdurdin deleted the fix/core/12619-disable-assertions-vcwin-release-build branch December 2, 2024 01:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(core,developer): use `NDEBUG` flag to disable assertions in release build #12715

fix(core,developer): use `NDEBUG` flag to disable assertions in release build #12715

mcdurdin commented Nov 27, 2024 •

edited

Loading

keymanapp-test-bot bot commented Nov 27, 2024 •

edited

Loading

mcdurdin Nov 27, 2024

mcdurdin commented Nov 27, 2024

ermshiperete commented Nov 27, 2024

ermshiperete left a comment

mcdurdin commented Nov 27, 2024

ermshiperete commented Nov 27, 2024

rc-swag left a comment

mcdurdin Nov 28, 2024

mcdurdin commented Nov 28, 2024

SabineSIL commented Nov 28, 2024

darcywong00 commented Nov 28, 2024

mcdurdin commented Nov 28, 2024 •

edited

Loading

darcywong00 left a comment

darcywong00 left a comment

rc-swag left a comment

keyman-server commented Dec 2, 2024

fix(core,developer): use NDEBUG flag to disable assertions in release build #12715

fix(core,developer): use NDEBUG flag to disable assertions in release build #12715

Conversation

mcdurdin commented Nov 27, 2024 • edited Loading

keymanapp-test-bot bot commented Nov 27, 2024 • edited Loading

User Test Results

Test Artifacts

mcdurdin Nov 27, 2024

Choose a reason for hiding this comment

mcdurdin commented Nov 27, 2024

ermshiperete commented Nov 27, 2024

ermshiperete left a comment

Choose a reason for hiding this comment

mcdurdin commented Nov 27, 2024

ermshiperete commented Nov 27, 2024

rc-swag left a comment

Choose a reason for hiding this comment

mcdurdin Nov 28, 2024

Choose a reason for hiding this comment

mcdurdin commented Nov 28, 2024

SabineSIL commented Nov 28, 2024

darcywong00 commented Nov 28, 2024

mcdurdin commented Nov 28, 2024 • edited Loading

darcywong00 left a comment

Choose a reason for hiding this comment

darcywong00 left a comment

Choose a reason for hiding this comment

rc-swag left a comment

Choose a reason for hiding this comment

keyman-server commented Dec 2, 2024

fix(core,developer): use `NDEBUG` flag to disable assertions in release build #12715

fix(core,developer): use `NDEBUG` flag to disable assertions in release build #12715

mcdurdin commented Nov 27, 2024 •

edited

Loading

keymanapp-test-bot bot commented Nov 27, 2024 •

edited

Loading

mcdurdin commented Nov 28, 2024 •

edited

Loading