core icon indicating copy to clipboard operation
core copied to clipboard

Have `protocol_test` depend explicitly on libsystemd

Open sp1ff opened this issue 3 years ago • 1 comments

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.

sp1ff avatar Oct 09 '22 22:10 sp1ff

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)

craigcomstock avatar Oct 13 '22 14:10 craigcomstock

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

sp1ff avatar Nov 12 '22 22:11 sp1ff

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!

craigcomstock avatar Nov 14 '22 16:11 craigcomstock

Updated, @craigcomstock -- thanks for the help.

sp1ff avatar Dec 22 '22 00:12 sp1ff

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 avatar Jul 24 '23 16:07 craigcomstock

@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.

craigcomstock avatar Jul 24 '23 16:07 craigcomstock

Sure, I triggered a build:

Build Status

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

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

cf-bottom avatar Jul 24 '23 16:07 cf-bottom

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.

craigcomstock avatar Jul 25 '23 14:07 craigcomstock

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

sp1ff avatar Aug 27 '23 21:08 sp1ff

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 avatar Aug 29 '23 13:08 craigcomstock

@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 avatar Oct 16 '23 14:10 craigcomstock

@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 avatar Jan 11 '24 20:01 craigcomstock

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 avatar Jan 11 '24 20:01 craigcomstock

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

https://github.com/cfengine/core/pull/5425

craigcomstock avatar Jan 11 '24 21:01 craigcomstock

@craigcomstock thanks for taking a look!

sp1ff avatar Jan 16 '24 18:01 sp1ff

@sp1ff can you try out the changes I made in https://github.com/cfengine/core/pull/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.

craigcomstock avatar Jan 17 '24 15:01 craigcomstock

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 avatar Jan 17 '24 18:01 sp1ff

@craigcomstock wfm! Thank you very much.

sp1ff avatar Jan 19 '24 15:01 sp1ff

@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.

craigcomstock avatar Jan 22 '24 13:01 craigcomstock