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

Fix compiling without procfs and ensure unit tests skip gracefully when procfs/sysfs/devtmpfs are not available #35101

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

bluca
Copy link
Member

@bluca bluca commented Nov 8, 2024

When running meson test in a chroot, compilation fails due to the D-Bus introspection script, and a number of unit tests fail as they assume procfs or devtmpfs are available. Fix D-Bus introspection script so that it can successfully run and the build stage can complete. Add a test parameter, copying the existing one that checks whether the system was booted with systemd, to check whether /proc/ is available and accessible, and skip otherwise. Add a CI job to ensure regressions do not sneak in.
It is not unheard of to build packages in minimal chroots, all that is required is building a rootfs (e.g.: deboostrap), installing the building dependencies and running ninja with chroot. Other competing projects can do this already just fine.

@bluca bluca added the tests label Nov 8, 2024
@bluca bluca changed the title tests: ensure unit tests skip gracefully when procfs/sysfs/devtmpfs are not available Fix compiling without procfs and ensure unit tests skip gracefully when procfs/sysfs/devtmpfs are not available Nov 9, 2024
@poettering
Copy link
Member

That's fine, because nobody is asking you to deal with anything.

you did, you seem to want to block #35072 because of this.

@poettering
Copy link
Member

I have various chroots around that I use to quickly check compilation in various debian and ubuntu versions when toolchain changes land.

btw, even debootstrap documents the procfs thing explicitly in its man page:

https://www.mankier.com/8/debootstrap#Examples

@bluca
Copy link
Member Author

bluca commented Nov 11, 2024

I have various chroots around that I use to quickly check compilation in various debian and ubuntu versions when toolchain changes land.

btw, even debootstrap documents the procfs thing explicitly in its man page:

https://www.mankier.com/8/debootstrap#Examples

Yes, because the principal use of debootstrap is the debian installer, which installs a full system on a machine (the hint is in that very same manpage: "Full process to create a complete Debian installation"). I'm not creating a full installed system, I'm compiling some stuff in a chroot.

@bluca
Copy link
Member Author

bluca commented Nov 11, 2024

That's fine, because nobody is asking you to deal with anything.

you did, you seem to want to block #35072 because of this.

and instead I've done the full work myself, so that you don't have to do anything, and nothing needs to be blocked

@poettering
Copy link
Member

Yes, because the principal use of debootstrap is the debian installer, which installs a full system on a machine (the hint is in that very same manpage: "Full process to create a complete Debian installation"). I'm not creating a full installed system, I'm compiling some stuff in a chroot.

but didn't you say that compiling works? why then this PR? this pr is about the test suite, and that's quite different..

also, again, even a pure builder without /proc/ is bogus, we should not normalize that.

@YHNdnzj
Copy link
Member

YHNdnzj commented Nov 11, 2024

As mentioned (and shown in the CI job), I just want to run 'chroot meson' and for it to compile the package successfully, without manual workarounds.

It's hard to justify any "manual workaround" when mounting /proc/ is trivial and almost always the right thing to do. Otherwise, w/ or w/o this PR, you'd be skipping a bunch of unit tests, and I have serious doubt in that regard. The build environment should be set up in a way that most tests can be executed rather than skipped, hence I agree with @poettering that what this is trying to "solve" is utterly not useful frankly...

@bluca
Copy link
Member Author

bluca commented Nov 11, 2024

but didn't you say that compiling works? why then this PR? this pr is about the test suite, and that's quite different..

No? First line in the description and first commit. I said compilation should work, and it doesn't.
Moreover, unit tests are part of the build step, that's why meson integrates them, and package builds run them.

also, again, even a pure builder without /proc/ is bogus, we should not normalize that.

It isn't. Just because you don't use or want something, it doesn't mean it's "bogus". Your "bogus" is someone else's "useful", and viceversa.

@bluca
Copy link
Member Author

bluca commented Nov 11, 2024

As mentioned (and shown in the CI job), I just want to run 'chroot meson' and for it to compile the package successfully, without manual workarounds.

It's hard to justify any "manual workaround" when mounting /proc/ is trivial and almost always the right thing to do. Otherwise, w/ or w/o this PR, you'd be skipping a bunch of unit tests, and I have serious doubt in that regard. The build environment should be set up in a way that most tests can be executed rather than skipped, hence I agree with @poettering that what this is trying to "solve" is utterly not useful frankly...

That's not really how it works today. Unit tests should and DO work in a variety of different conditions. There are plenty of conditions that already make a unit test skip gracefully: the fact that the system wasn't booted with systemd (.sdbooted = true), the fact that it's not running as root, the fact that it's not running in a VM, the fact that it's running in a specific container, etc. You wouldn't say "you must build as root" and drop all privileges checks+skips. You wouldn't say "must not run in a container" and drop all container checks+skips. You wouldn't say "must be run on a systemd booted system" and drop the sd_booted checks+skips.

@yuwata
Copy link
Member

yuwata commented Nov 11, 2024

Besides of building under chroot without /proc/ is useful or not, again, running tests without /proc/ is OK, I think. Then, we may notice something unexpectedly broken in the future e.g. when FORMAT_PROC_FD_PATH() is used without correct error handling or so.

Useful or not depends on person, projects, situations, and so on. If @bluca manages the way to run test, then I think it is enough. And, in the future if @bluca will not care the test environment, then we can simply disable the CI.

@yuwata
Copy link
Member

yuwata commented Nov 11, 2024

Note that the above comment does NOT mean we should support such fancy and exotic build environment. And in that sense, I agree with @poettering and @YHNdnzj, and I wonder why @bluca wants to build without /proc/.

@evverx
Copy link
Member

evverx commented Nov 12, 2024

The good news is that this makes no difference to anybody who doesn't care

It does. As mentioned in #35101 (review) it can prevent the fuzz targets from crashing in runtime environments that actually matter.

that's fine, you don't have to, but I do and so I've put in the work

Given that the fuzz targets can work in three different modes and with this patch they don't work correctly in any of them it doesn't look like a lot of work has been put. There is probably some sort of misunderstanding of what fuzz targets are and how they work (which probably can explain where things like polkit-org/polkit#506 come from) but at least in systemd it's all documented. It's documented that the fuzz targets can run in three different modes (where unit tests are just one of those modes) so it would be nice to read the documentation every now and then to be able to propose meaningful changes.

@yuwata
Copy link
Member

yuwata commented Nov 12, 2024

OK, then setting the needs-rework label, though I am not familiar with the modes of running fuzzers.

@yuwata yuwata added reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks and removed please-review PR is ready for (re-)review by a maintainer labels Nov 12, 2024
@evverx
Copy link
Member

evverx commented Nov 13, 2024

I think this PR should get past the "needs-dicsussion" label first and it looks like it's far from reaching that point. If it's decided that build environments like that are important enough to be supported one way or another it should be possible to start discussing how to make it possible to run as many test as possible there (which I assume what this PR is about). I hope it isn't about making sloppy build environments look less sloppy by blindly skipping things till stuff "works".

@github-actions github-actions bot added please-review PR is ready for (re-)review by a maintainer and removed reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks labels Nov 13, 2024
@evverx
Copy link
Member

evverx commented Nov 14, 2024

For the record #35101 (review) hasn't been addressed (apart from replacing return 0 with return EXIT_TEST_SKIP).

FWIW @yuwata I took a look at fuzz-dhcp-server and with no /proc it crashes in

assert_se(dhcp_server_save_leases(server) >= 0);

because it can't extract /proc/sys/kernel/random/boot_id to fill out the BootID field and in terms of running it as a unit test it's absolutely non-essential and there is no reason it should be skipped. I haven't looked at all the other places but I get the impression that this PR hasn't been thought trough and the failures haven't been investigated (they are just silenced for no reason to make stuff look green).

…be obtained

If /proc/ is not available, try to search in the current dir as
a fallback
Following the model of the existing .sd_booted property, which checks
that the system was booted with systemd and skips the unit test if it
was not, add a .proc_mounted property that checks whether /proc/ is
available and accessible, and skips otherwise.
Some tests, like the journald ones, do not use the TEST() macro for
one reason or another, so manually check procfs where needed.
@yuwata yuwata added needs-rebase and removed please-review PR is ready for (re-)review by a maintainer labels Dec 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

5 participants