CFE-4274: Have `protocol_test` depend explicitly on libsystemd #5075

sp1ff · 2022-10-09T22:54:42Z

On Debian, when I configure with argument --with-systemd-service=no, the unit test protocol_test fails to build with a link-time error:

/usr/bin/ld: cf-serverd-functions.o: undefined reference to symbol 'sd_notifyf@@LIBSYSTEMD_209'
/usr/bin/ld: /lib/x86_64-linux-gnu/libsystemd.so.0: error adding symbols: DSO missing from command line
collect2: error: ld returned 1 exit status

This change makes protocol_test depend unconditionally on libsystemd. TBH I'm not sure this is the proper way to fix this; submitting my local fix for posterity's sake as much as anything else.

craigcomstock · 2022-10-13T14:57:29Z

I think the issue is that you have libsystemd-dev installed and so configure.ac detects that availability and sets HAVE_SD_NOTIFY_BARRIER but... the Makefile.am files that need -lsystemd don't have the proper inclusion as I have done below.

Our github action "unit_test" is failing because we don't install libsystemd-dev and so it is not available.

Can you try out this diff instead of your fix?

diff --git a/cf-serverd/Makefile.am b/cf-serverd/Makefile.am
index b2b268d..9739ff8 100644
--- a/cf-serverd/Makefile.am
+++ b/cf-serverd/Makefile.am
@@ -35,6 +35,8 @@ AM_CFLAGS = \
        $(PTHREAD_CFLAGS) \
        $(ENTERPRISE_CFLAGS)
 
+LIBS += $(SYSTEMD_SOCKET_LIBS)
+
 libcf_serverd_la_LIBADD = ../libpromises/libpromises.la
 
 libcf_serverd_la_SOURCES = \
diff --git a/tests/unit/Makefile.am b/tests/unit/Makefile.am
index 6a72dce..44ba5a1 100644
--- a/tests/unit/Makefile.am
+++ b/tests/unit/Makefile.am
@@ -45,6 +45,8 @@ LDADD = ../../libpromises/libpromises.la libtest.la
 # implementation, then we are pretty sure that they will be overriden by
 # our local implementation. So we include *everything*...
 LIBS = $(CORE_LIBS)
+# protocol_test relies on cf-serverd-functions which can rely on libsystemd so need SYSTEMD_SOCKET_LIBS here
+LIBS += $(SYSTEMD_SOCKET_LIBS)
 AM_LDFLAGS = $(CORE_LDFLAGS)
 
 AM_CFLAGS = $(PTHREAD_CFLAGS)

sp1ff · 2022-11-12T22:36:05Z

Yes-- that works for me, @craigcomstock; want me to cut a new PR/update this one?

cfengine#5075

craigcomstock · 2022-11-14T16:38:17Z

Yes-- that works for me, @craigcomstock; want me to cut a new PR/update this one?

Yes, please do update this PR. I expect the checks will pass and we can merge. Thanks!

sp1ff · 2022-12-22T00:22:26Z

Updated, @craigcomstock -- thanks for the help.

craigcomstock · 2023-07-24T16:05:58Z

Goodness @sp1ff looks like we let this one get rather stale. How's it going? I will try and re-run the github checks and see where we are at by looking at the current change set.

craigcomstock · 2023-07-24T16:08:43Z

@cf-bottom jenkins please :) @sp1ff this will cause a pretty thorough run on our internal CI system and should give us an idea where we are at. I suppose you could push an empty commit here to bump github actions into re-running.

cf-bottom · 2023-07-24T16:09:14Z

Sure, I triggered a build:

Jenkins: https://ci.cfengine.com/job/pr-pipeline/9633/

Packages: http://buildcache.cfengine.com/packages/testing-pr/jenkins-pr-pipeline-9633/

craigcomstock · 2023-07-25T14:01:24Z

looks like our canaries: rhel7 hub and agent and ubuntu 20 all failed with the current PR

11:20:45   CCLD     mon_load_test
11:20:45 /usr/bin/ld: cannot find -lsystemd
11:20:45 collect2: error: ld returned 1 exit status
11:20:45 gmake[4]: *** [protocol_test] Error 1

I'll take a look when I can and see if I can fix this up.

sp1ff · 2023-08-27T21:20:21Z

Apologies, @craigcomstock ... life got busy. I rebased this PR & pushed it up-- let's see what happens now.

craigcomstock · 2023-08-29T13:22:25Z

Apologies, @craigcomstock ... life got busy. I rebased this PR & pushed it up-- let's see what happens now.

No worries! Thanks so much for coming back and trying some more.

craigcomstock · 2023-10-16T14:49:50Z

@sp1ff looks like the push 5 days ago still doesn't pass tests. I have created a ticket: https://northerntech.atlassian.net/browse/CFE-4274 and assigned it to myself so we can get this done. Thanks for the continued attention here. Appreciate it.

@craigcomstock

installed; thanks @craigcomstock for the proper fix. `protocol_test` needs `sd_notify()` regardless of the `--with-systemd-service` configuration option.

craigcomstock · 2024-01-11T20:40:34Z

@sp1ff I looked at this for a bit today. I started at the beginning and it seems like the "easiest" workaround is to use

./autogen.sh  --with-systemd-service=no --with-systemd-socket=no

It seems our configure.ac doesn't coordinate things quite enough. I will research wether it makes sense to assume --with-systemd-socket=no if --with-systemd-service=no. Seems reasonable but want to make sure.

Thanks for your patience and hope you are well.

craigcomstock · 2024-01-11T20:53:51Z

Looking at this a bit more I think we have two separate items, from configure --help:

  --with-systemd-socket[=PATH]
                          support systemd socket activation

and

  --with-systemd-service=PATH
                          Install systemd service file in given path. The
                          default is no, but if specified, the default path is
                          /usr/lib/systemd/system.

I could see wanting systemd service and not the systemd socket. I can't think of a reason we would want systemd socket support without systemd services. I will make this dependency in configure.ac and hopefully this will make it easier to handle.

if --with-systemd-socket=yes then force --with-systemd-service=yes
if --with-systemd-service=no then force --with-systemd-socket=no

Of course this also assumes that libsystemd-dev package or similar is available if you choose systemd stuff.

The problem for you was that the --with-systemd-socket defaults to yes regardless of presence of libsystemd-dev or --with-system-service=no.

craigcomstock · 2024-01-11T21:05:16Z

didn't want to squash your work here or our conversation so created a new PR with my idea. Will see if it sticks. :)

#5425

sp1ff · 2024-01-16T18:19:41Z

@craigcomstock thanks for taking a look!

craigcomstock · 2024-01-17T15:10:24Z

@sp1ff can you try out the changes I made in #5425 ? They are passing CI and I think they should "work" in your case. If so we can merge that one and close this one. :) Thanks again for the contribution.

sp1ff · 2024-01-17T18:14:57Z

Sure-- will give it a try this evening. Craig Comstock ***@***.***> writes:

…

@sp1ff can you try out the changes I made in #5425 ? They are passing CI and I think they should "work" in your case. If so we can merge that one and close this one. :) Thanks again for the contribution. — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.

-- Michael ***@***.***>

sp1ff · 2024-01-19T15:07:04Z

@craigcomstock wfm! Thank you very much.

craigcomstock · 2024-01-22T13:51:52Z

@craigcomstock wfm! Thank you very much.

Awesome. The fix is merged in master and our maintenance branches: 3.18.x and 3.21.x.

Be well and thanks again for the notice and contribution.

larsewi requested a review from Lex-2008 October 10, 2022 10:01

Lex-2008 previously approved these changes Oct 10, 2022

View reviewed changes

Lex-2008 requested a review from craigcomstock October 10, 2022 11:41

sp1ff pushed a commit to sp1ff/core that referenced this pull request Nov 12, 2022

Applying @craigcomstock's proposed fix in

10d317c

cfengine#5075

sp1ff dismissed Lex-2008’s stale review via ede8a30 December 22, 2022 00:16

sp1ff force-pushed the fix_protocol_test branch from ede8a30 to 12c2e44 Compare December 22, 2022 00:21

sp1ff requested review from Lex-2008 and removed request for craigcomstock January 25, 2023 00:46

sp1ff force-pushed the fix_protocol_test branch from 198e653 to f26807c Compare August 27, 2023 21:18

sp1ff force-pushed the fix_protocol_test branch from f26807c to f3f05e7 Compare October 12, 2023 01:09

craigcomstock requested review from craigcomstock and removed request for Lex-2008 October 16, 2023 14:46

craigcomstock self-assigned this Oct 16, 2023

craigcomstock changed the title ~~Have protocol_test depend explicitly on libsystemd~~ CFE-4274: Have protocol_test depend explicitly on libsystemd Oct 16, 2023

Make protocol_test build on Debian systems with libsystemd-dev

6a26442

installed; thanks @craigcomstock for the proper fix. `protocol_test` needs `sd_notify()` regardless of the `--with-systemd-service` configuration option.

sp1ff force-pushed the fix_protocol_test branch from f3f05e7 to 6a26442 Compare October 23, 2023 17:01

craigcomstock closed this Jan 22, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CFE-4274: Have `protocol_test` depend explicitly on libsystemd #5075

CFE-4274: Have `protocol_test` depend explicitly on libsystemd #5075

sp1ff commented Oct 9, 2022

craigcomstock commented Oct 13, 2022 •

edited

Loading

sp1ff commented Nov 12, 2022 •

edited

Loading

craigcomstock commented Nov 14, 2022

sp1ff commented Dec 22, 2022

craigcomstock commented Jul 24, 2023

craigcomstock commented Jul 24, 2023

cf-bottom commented Jul 24, 2023

craigcomstock commented Jul 25, 2023

sp1ff commented Aug 27, 2023

craigcomstock commented Aug 29, 2023

craigcomstock commented Oct 16, 2023

craigcomstock commented Jan 11, 2024

craigcomstock commented Jan 11, 2024

craigcomstock commented Jan 11, 2024

sp1ff commented Jan 16, 2024

craigcomstock commented Jan 17, 2024

sp1ff commented Jan 17, 2024 via email

sp1ff commented Jan 19, 2024

craigcomstock commented Jan 22, 2024

CFE-4274: Have protocol_test depend explicitly on libsystemd #5075

CFE-4274: Have protocol_test depend explicitly on libsystemd #5075

Conversation

sp1ff commented Oct 9, 2022

craigcomstock commented Oct 13, 2022 • edited Loading

sp1ff commented Nov 12, 2022 • edited Loading

craigcomstock commented Nov 14, 2022

sp1ff commented Dec 22, 2022

craigcomstock commented Jul 24, 2023

craigcomstock commented Jul 24, 2023

cf-bottom commented Jul 24, 2023

craigcomstock commented Jul 25, 2023

sp1ff commented Aug 27, 2023

craigcomstock commented Aug 29, 2023

craigcomstock commented Oct 16, 2023

craigcomstock commented Jan 11, 2024

craigcomstock commented Jan 11, 2024

craigcomstock commented Jan 11, 2024

sp1ff commented Jan 16, 2024

craigcomstock commented Jan 17, 2024

sp1ff commented Jan 17, 2024 via email

sp1ff commented Jan 19, 2024

craigcomstock commented Jan 22, 2024

CFE-4274: Have `protocol_test` depend explicitly on libsystemd #5075

CFE-4274: Have `protocol_test` depend explicitly on libsystemd #5075

craigcomstock commented Oct 13, 2022 •

edited

Loading

sp1ff commented Nov 12, 2022 •

edited

Loading