-
Notifications
You must be signed in to change notification settings - Fork 18.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
hack: explicitly control enabling the journald logging driver #47789
base: master
Are you sure you want to change the base?
hack: explicitly control enabling the journald logging driver #47789
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. |
fc6c657
to
221133b
Compare
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 |
Counterpoint: it also makes it harder to tell from the git log, where the related discussion is. |
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 <w.d.hubbs@gmail.com>
221133b
to
890be8a
Compare
I added the DCO signoff and removed the Closes keyword as requested. |
Ping, what is the status of this pr? Thanks much. |
@@ -83,7 +83,7 @@ if [ ! "$GOPATH" ]; then | |||
exit 1 | |||
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
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)