hack: explicitly control enabling the journald logging driver#47789
hack: explicitly control enabling the journald logging driver#47789tianon merged 1 commit intomoby:masterfrom williamh:47770-control-enable-journald-driver
Conversation
|
Sorry, I'm actually struggling to understand here. If you want to build a binary that works for users who may or may not be using systemd (and thus may or may not want the journald logging driver), you need to compile against libsystemd and libsystemd is then a dependency of that binary (and binary package). The "fix" here is to accurately make |
... but the fix broke functionality of the binary for the other half 😭 |
No, it didn't, because we wired up the packaging to pass a flag to enable/disable systemd based on a packaging option (and I verified libsystemd gets linked against correctly when it's on, and not when it's off). What we want is a toggle supported upstream to say "yes, I know systemd is installed on this system, please don't use it" to produce a binary which isn't linked against libsystemd, rather than deciding purely based on whether it's installed. The default doesn't matter for us either way, I presume you'd want it on. Our documentation on it is https://wiki.gentoo.org/wiki/Project:Quality_Assurance/Automagic_dependencies. |
|
@tianon this is a very big misunderstanding of feature flags in general, optional dependencies (and automatic ones) and overall is exactly the opposite of what Gentoo wants. Gentoo has a robust mechanism for linking feature flags on the distro package to compiled-in features in the binary. In order for this to work, the docker build script needs to expose the option. The current state of affairs is that Gentoo's binary caching layer marks some binaries as being linked to systemd and other binaries as being NOT linked to systemd, but the docker build system is ignoring that and building both against systemd. End result: people disable the Gentoo feature flag "systemd" but accidentally get a broken binary that was linked against systemd just because it happened to be installed on a buildbot VM. It works if they totally disable binary caching because then docker gets built from source and end users who don't have the "systemd" feature flag set on their Gentoo install usually don't also have systemd installed. (It's not a guarantee.) The goal of this PR is so that Gentoo can forward the package "systemd" toggle to the docker build script so that both the package manager and the docker binary agree on what feature flags are currently enabled. |
|
I updated the patch to have this on by default as @thesamesam suggested above. |
|
Looks like the commit is missing a DCO sign-off, causing CI to fail. If you update, could you also remove the |
thaJeztah
added
area/logging
status/2-code-review
area/systemd
dco/no
|
Counterpoint: it also makes it harder to tell from the git log, where the related discussion is. |
|
I added the DCO signoff and removed the Closes keyword as requested. |
|
Ping, what is the status of this pr? Thanks much. |
| fi | ||
|
|
||
| if ${PKG_CONFIG} 'libsystemd' 2> /dev/null; then | ||
| if [[ ${SYSTEMD:-1} == 1 ]] && ${PKG_CONFIG} 'libsystemd' 2> /dev/null; then |
There was a problem hiding this comment.
Instead of the very generic SYSTEMD name here, can we use something more descriptive/specific? I would suggest something like NO_AUTO_BUILDTAG_JOURNALD but open to other ideas/suggestions (especially opinions from other maintainers). Also, [[ isn't necessary here ([ is sufficient).
| if [[ ${SYSTEMD:-1} == 1 ]] && ${PKG_CONFIG} 'libsystemd' 2> /dev/null; then | |
| if [ -z "${NO_AUTO_BUILDTAG_JOURNALD:-}" ] && ${PKG_CONFIG} 'libsystemd' 2> /dev/null; then |
There was a problem hiding this comment.
Also,
[[isn't necessary here ([is sufficient).
The flip side is that the script explicitly uses bash, and the [[ keyword has no downsides but allows bash to process it much earlier, in time to know that variables are variables -- the single [ is a builtin program that is also available at /usr/bin/[ and only knows that it received "argv arguments". This is why the single [ needs you to quote variables that may be empty or contain spaces, while the [[ allows omitting the quotes. In fact, even if you omit them by accident, it still works. :)
There was a problem hiding this comment.
Sure, this is more of a consistency request than an argument for one or the other (the rest of hack/ minus three instances is consistently using bare [, which given we're in Bash, also doesn't shell out).
Without this, the dependency on systemd is said to be "automagic", which can lead to breakage, for example, if a binary package of docker is built on a system that has systemd installed then installed on a system that does not have systemd installed. for example: https://bugs.gentoo.org/914076 Signed-off-by: William Hubbs <[email protected]>
|
@tianon I used the variable name EXCLUDE_AUTO_BUILDTAG_JOURNALD to make the name consistent with the way other drivers are excluded in docker. |
vvoland
added
process/cherry-pick/27.0
and removed
dco/no
7 participants
Without this, the dependency on systemd is said to be "automagic", which can lead to breakage, for example, if a binary package of docker is built on a system that has systemd installed then installed on a system that does not have systemd installed.
for example: https://bugs.gentoo.org/914076
closes #47770
- What I did
- How I did it
- How to verify it
- Description for the changelog
- A picture of a cute animal (not mandatory but encouraged)