-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
mkosi: Sanitizers #32866
mkosi: Sanitizers #32866
Conversation
Important An -rc1 tag has been created and a release is being prepared, so please note that PRs introducing new features and APIs will be held back until the new version has been released. |
f94fda7
to
914e92c
Compare
This comment was marked as off-topic.
This comment was marked as off-topic.
|
24096e4
to
7dcfadb
Compare
Without sanitizer the integration test runs takes about 50 minutes, with sanitizers about 90 minutes - that's quite a bump. Also, I am very worried that by making things slower, we'll hide more race conditions (like the ones we found in the past few weeks). Hence, I'd prefer if we had one job that runs with sanitizers, and the rest without. We could maybe to a weekly run with all jobs. |
Isn't it the opposite? I'm pretty sure that the reason we uncovered all those race conditions is because the tests in Github Actions run with drastically fewer resources than they do in CentOS CI. |
I thought they were faster here? But anyway, same thing other way around: a mix of the two would be best, so that we have more variations in the base environment |
6ef154e
to
bdb09f1
Compare
src/test/test-execute.c
Outdated
@@ -1339,7 +1151,6 @@ static void run_tests(RuntimeScope scope, char **patterns) { | |||
entry(test_exec_ignoresigpipe), | |||
entry(test_exec_inaccessiblepaths), | |||
entry(test_exec_ioschedulingclass), | |||
entry(test_exec_mount_apivfs), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be possible to just skip this test case under sanitizers? I quite like having this as part of the unit test run, rather than the integration test run, as it's much faster feedback
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A unit test really shouldn't be doing what this test is doing, I think this just fits much better as an integration test. I'd much rather have us focus on speeding up the integration tests rather than trying to fit stuff that should be an integration test in a unit test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An integration test running in a VM will never be able to run in a handful of seconds though, and it also won't be able to run at package build time. Of course I'm not asking to make it work under sanitizers, that would be way too involved, and having the same test case also in the integration test is nice. But I'd really like to keep this, and just have it skipped if sanitizers are enabled, that should be simple enough?
7e630ac
to
3cb7239
Compare
dbus-broker and dbus-daemon have not been made interchangable on OpenSUSE so we currently end up with dbus-broker used for the system bus and dbus-daemon for the session bus. Let's stick to dbus-daemon on OpenSUSE until they switch to dbus-broker.
When running with sanitizers we need more memory otherwise the unit gets OOM killed.
The test does not work under sanitizers as strace is used. Until the test is fixed to not use strace let's skip it when running with sanitizers.
Required since we run with DynamicUser=1.
Some tests (e.g. test-udev.py) might trigger one of our NSS modules which means LD_PRELOAD has to be configured properly.
The test fails when running under sanitizers due to missing sanitizer libraries. For now, let's skip the test until we can make the necessary changes to run it under sanitizers.
System call filtering is incompatible with sanitizers so let's skip these tests when we're built with sanitizers.
When DynamicUser= is enabled, we need LD_PRELOAD to be configured correctly as the tests will load systemd's nss module which will complain when built with sanitizers if the sanitizer libraries were not loaded first.
Let's not fail if directories already exist in cp_r().
- Let's set the environment on the kernel command line so it applies to initrd and main system. - Let's add the necessary wrappers that are also added in test-functions. Unlike test-functions we don't use gcc/clang to get the library path as that requires installing gcc/clang in the initrd. - Let's drop the hack to get journald writing to the console and have it write to kmsg instead. We'll get the output either way. - Stop removing libstdc++ and sanitizer libraries from Arch Linux initrds and other images as it's required by the sanitizer libraries. - Add a workaround for specifying extra meson options for opensuse - Add a leak sanitizer suppression file as a workaround for a false positive leak in verify_selinuxmnt() in libselinux. We do a soname match because the stacktrace can't be properly symbolized on Debian.
No description provided.