Bug 14288 - OmniOS build/linking problem
Summary: OmniOS build/linking problem
Status: RESOLVED FIXED
Alias: None
Product: Samba 4.1 and newer
Classification: Unclassified
Component: Build (show other bugs)
Version: 4.14.0rc4
Hardware: All Other
: P5 normal (vote)
Target Milestone: ---
Assignee: Karolin Seeger
QA Contact: Samba QA Contact
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2020-02-21 21:55 UTC by Peter Eriksson
Modified: 2021-04-16 12:24 UTC (History)
3 users (show)

See Also:


Attachments
Patch to wscript to fix OmniOS linking again (622 bytes, text/plain)
2020-02-21 21:55 UTC, Peter Eriksson
no flags Details
backported fix for 4.14, 4.13, 4.12 (1.00 KB, patch)
2021-03-03 13:27 UTC, Björn Jacke
bjacke: review+
Details
Patch for 4.14, 4.13 (2.36 KB, patch)
2021-04-12 10:50 UTC, Martin Schwenke
abartlet: review+
martins: ci-passed+
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Peter Eriksson 2020-02-21 21:55:08 UTC
Created attachment 15812 [details]
Patch to wscript to fix OmniOS linking again

A change in the "wscript" from earlier version breaks OmniOS linking. See attached patch that reverts the change (and fixes the problem).
Comment 1 Peter Eriksson 2021-03-02 15:47:00 UTC
This fix is still needed to build Samba 4.14.0rc4 on latest OmniOS.

Output from bin/config.log without it:

> ['gcc', '-lreadline', '-lncurses', '-lnsl', '-lsocket', '-Wl,--as-needed', 'test.c.1.o', '-o/Users/peter86/Build/samba/samba-4.14.0rc4/bin/.conf_check_33fa872a3523539c06934d3ea23c1f54/testbuild/default/testprog', '-Wl,-Bstatic', '-Wl,-Bdynamic', '-L/usr/local/lib', '-L/usr/local/lib', '-lc']
> err: ld: fatal: unrecognized option '--as-needed'
> ld: fatal: use the -z help option for usage information
Comment 2 Björn Jacke 2021-03-02 22:16:11 UTC
this wscript line is also working correctly for you? :
conf.ADD_LDFLAGS('-Wl,--as-needed', testflags=True)
Comment 3 Peter Eriksson 2021-03-02 23:04:17 UTC
Yes, that seems to work also.

#    conf.add_as_needed()                                                                                               
#    conf.env.asneeded_ldflags = conf.ADD_LDFLAGS('-Wl,--as-needed', testflags=True)                                    
    conf.ADD_LDFLAGS('-Wl,--as-needed', testflags=True)

(Then compilation fails later on in source3/modules/vfs_solarisacls.c - but that’s a complete other issue.)

- Peter


> On 2 Mar 2021, at 23:16, samba-bugs@samba.org wrote:
> 
> https://bugzilla.samba.org/show_bug.cgi?id=14288
> 
> --- Comment #2 from Björn Jacke <bjacke@samba.org> ---
> this wscript line is also working correctly for you? :
> conf.ADD_LDFLAGS('-Wl,--as-needed', testflags=True)
> 
> -- 
> You are receiving this mail because:
> You reported the bug.
Comment 4 Samba QA Contact 2021-03-03 12:37:02 UTC
This bug was referenced in samba master:

996560191ac6bd603901dcd6c0de5d239e019ef4
Comment 5 Björn Jacke 2021-03-03 12:37:46 UTC
I've reported the waf issue upsteam: https://gitlab.com/ita1024/waf/-/issues/2342
Comment 6 Björn Jacke 2021-03-03 13:27:09 UTC
Created attachment 16494 [details]
backported fix for 4.14, 4.13, 4.12
Comment 7 Karolin Seeger 2021-03-05 10:08:29 UTC
(In reply to Björn Jacke from comment #6)
Pushed to autobuild-v4-{14,13,12}-test.
Comment 8 Samba QA Contact 2021-03-05 12:19:11 UTC
This bug was referenced in samba v4-13-test:

eebf510fbd8847077c7bec72a1cda674b5a02714
Comment 9 Samba QA Contact 2021-03-05 13:18:11 UTC
This bug was referenced in samba v4-12-test:

c63f00801cae16a855aa5283fa0cc449e84577ce
Comment 10 Samba QA Contact 2021-03-05 14:26:03 UTC
This bug was referenced in samba v4-14-test:

4d1ed9c319deac5cba1682611dcefdf002cb9d48
Comment 11 Andrew Bartlett 2021-03-08 02:35:23 UTC
Does Samba actually work well with this patch?

I'm concerned that Samba might build but then just fail per bug 11355 instead.
Comment 12 Andrew Bartlett 2021-03-08 02:47:56 UTC
Sadly this has caused a regression as --as-needed is now omitted on Linux.

The issue here is that conf.ADD_LDFLAGS() modified EXTRA_LDFLAGS
while conf.add_as_needed() changes LINKFLAGS.

I forgot to check the build output before after.
Comment 13 Karolin Seeger 2021-03-08 07:24:39 UTC
(In reply to Andrew Bartlett from comment #12)
So this is a showstopper for Samba 4.14.0?
Comment 14 Martin Schwenke 2021-03-08 10:51:59 UTC
(In reply to Andrew Bartlett from comment #12)

--as-needed isn't omitted in the case I'm seeing.

The problem is with CTDB's reqid_test, which uses this build rule in ctdb/wscript:

    bld.SAMBA_BINARY('reqid_test',
                     source='tests/src/reqid_test.c',
                     deps='samba-util',
                     install_path='${CTDB_TEST_LIBEXECDIR}')

The installation link command looks like:

[4694/6851] Linking bin/default/ctdb/reqid_test.inst
20:16:55 runner ['/usr/lib64/ccache/gcc', 'ctdb/tests/src/reqid_test.c.181.o', '-o/root/samba-4.15.0pre1-GIT-b145434/bin/default/ctdb/reqid
_test.inst', '-Wl,-rpath,/usr/local/samba/lib', '-Wl,-Bstatic', '-Wl,-Bdynamic', '-L/root/samba-4.15.0pre1-GIT-b145434/bin/default/lib/repl
ace', '-L/root/samba-4.15.0pre1-GIT-b145434/bin/default/lib/talloc', '-L/root/samba-4.15.0pre1-GIT-b145434/bin/default/lib/tevent', '-L/roo
t/samba-4.15.0pre1-GIT-b145434/bin/default/lib/util', '-L/usr/local/lib', '-L/usr/local/lib', '-lsamba-util', '-lgenrand-samba4', '-lsocket
-blocking-samba4', '-lutil-setid-samba4', '-ltevent', '-lsamba-debug-samba4', '-ltime-basic-samba4', '-lsys-rw-samba4', '-ltalloc', '-lrepl
ace-samba4', '-liov-buf-samba4', '-licui18n', '-lgnutls', '-licudata', '-licuuc', '-lsystemd', '-lunwind-generic', '-lunwind', '-lbsd', '-l
dl', '-lpthread', '-pie', '-Wl,-z,relro,-z,now', '-Wl,-no-undefined', '-Wl,--export-dynamic', '-Wl,--as-needed']

It is missing -Wl,-rpath,/usr/local/samba/lib/private, needed to locate private libraries that are dependencies of samba-util:

[root@testtest1 ~]# ldd /usr/local/samba/libexec/ctdb/tests/reqid_test
        linux-vdso.so.1 (0x00007fff261f6000)
        libsamba-util.so.0 => /usr/local/samba/lib/libsamba-util.so.0 (0x00007fc9d9970000)
        libgenrand-samba4.so => not found
        libsocket-blocking-samba4.so => not found
        libutil-setid-samba4.so => not found
        libtevent.so.0 => /lib64/libtevent.so.0 (0x00007fc9d975c000)
        libsamba-debug-samba4.so => not found
        libtime-basic-samba4.so => not found
        libsys-rw-samba4.so => not found
        libtalloc.so.2 => /lib64/libtalloc.so.2 (0x00007fc9d9547000)
        libreplace-samba4.so => not found
        libiov-buf-samba4.so => not found
        libicui18n.so.60 => /lib64/libicui18n.so.60 (0x00007fc9d9086000)
        libgnutls.so.30 => /lib64/libgnutls.so.30 (0x00007fc9d8c95000)
        ...

If I add a direct dependency on, say, replace:

    bld.SAMBA_BINARY('reqid_test',
                     source='tests/src/reqid_test.c',
                     deps='samba-util replace',
                     install_path='${CTDB_TEST_LIBEXECDIR}')

then this works around the problem.

So, for some reason the changed code still picks up all the dependencies but it stops directories for recursive dependencies being added to rpath.

Looking at third_party/waf/waflib/Tools/c_config.py:add_as_needed(), I can't see why using this function causes anything to happen with rpath.

It is also unclear why the failure happens on CentOS 7 but it doesn't happen on Debian testing.
Comment 15 Martin Schwenke 2021-03-08 11:37:02 UTC
(In reply to Karolin Seeger from comment #13)

Good question.

The short answer is that it causes a test regression when the CTDB tests are installed, on 1 platform that we know about.  We're probably the only ones who do that.

The real question is whether any other binaries suffer from the same problem is the CTDB reqid_test binary.  It appears these binaries are affected:

[root@testtbuild1 samba-4.15.0pre1-GIT-b145434]# for i in $(find bin/default/ -name "*.inst") ; do if ldd "$i" | grep -q "not found" ; then echo "$i"; fi; done | sort
bin/default/ctdb/reqid_test.inst
bin/default/examples/libsmbclient/testacl.inst
bin/default/examples/libsmbclient/testacl2.inst
bin/default/examples/libsmbclient/testacl3.inst
bin/default/examples/libsmbclient/testbrowse.inst
bin/default/examples/libsmbclient/testbrowse2.inst
bin/default/examples/libsmbclient/testchmod.inst
bin/default/examples/libsmbclient/testctx.inst
bin/default/examples/libsmbclient/testfstatvfs.inst
bin/default/examples/libsmbclient/testnotify.inst
bin/default/examples/libsmbclient/testread.inst
bin/default/examples/libsmbclient/testsmbc.inst
bin/default/examples/libsmbclient/teststat.inst
bin/default/examples/libsmbclient/teststat2.inst
bin/default/examples/libsmbclient/teststat3.inst
bin/default/examples/libsmbclient/teststatvfs.inst
bin/default/examples/libsmbclient/testtruncate.inst
bin/default/examples/libsmbclient/testutime.inst
bin/default/examples/libsmbclient/testwrite.inst
bin/default/nsswitch/stress-nss-libwbclient.inst
bin/default/source3/lib/netapi/examples/dsgetdc/dsgetdc.inst
bin/default/source3/lib/netapi/examples/file/file_close.inst
bin/default/source3/lib/netapi/examples/file/file_enum.inst
bin/default/source3/lib/netapi/examples/file/file_getinfo.inst
bin/default/source3/lib/netapi/examples/getdc/getdc.inst
bin/default/source3/lib/netapi/examples/group/group_add.inst
bin/default/source3/lib/netapi/examples/group/group_adduser.inst
bin/default/source3/lib/netapi/examples/group/group_del.inst
bin/default/source3/lib/netapi/examples/group/group_deluser.inst
bin/default/source3/lib/netapi/examples/group/group_enum.inst
bin/default/source3/lib/netapi/examples/group/group_getinfo.inst
bin/default/source3/lib/netapi/examples/group/group_getusers.inst
bin/default/source3/lib/netapi/examples/group/group_setinfo.inst
bin/default/source3/lib/netapi/examples/group/group_setusers.inst
bin/default/source3/lib/netapi/examples/join/getjoinableous.inst
bin/default/source3/lib/netapi/examples/join/getjoininformation.inst
bin/default/source3/lib/netapi/examples/join/netdomjoin.inst
bin/default/source3/lib/netapi/examples/join/rename_machine.inst
bin/default/source3/lib/netapi/examples/localgroup/localgroup_add.inst
bin/default/source3/lib/netapi/examples/localgroup/localgroup_addmembers.inst
bin/default/source3/lib/netapi/examples/localgroup/localgroup_del.inst
bin/default/source3/lib/netapi/examples/localgroup/localgroup_delmembers.inst
bin/default/source3/lib/netapi/examples/localgroup/localgroup_enum.inst
bin/default/source3/lib/netapi/examples/localgroup/localgroup_getinfo.inst
bin/default/source3/lib/netapi/examples/localgroup/localgroup_getmembers.inst
bin/default/source3/lib/netapi/examples/localgroup/localgroup_setinfo.inst
bin/default/source3/lib/netapi/examples/localgroup/localgroup_setmembers.inst
bin/default/source3/lib/netapi/examples/netlogon/netlogon_control.inst
bin/default/source3/lib/netapi/examples/netlogon/netlogon_control2.inst
bin/default/source3/lib/netapi/examples/netlogon/nltest.inst
bin/default/source3/lib/netapi/examples/server/remote_tod.inst
bin/default/source3/lib/netapi/examples/server/server_getinfo.inst
bin/default/source3/lib/netapi/examples/share/share_add.inst
bin/default/source3/lib/netapi/examples/share/share_del.inst
bin/default/source3/lib/netapi/examples/share/share_enum.inst
bin/default/source3/lib/netapi/examples/share/share_getinfo.inst
bin/default/source3/lib/netapi/examples/share/share_setinfo.inst
bin/default/source3/lib/netapi/examples/shutdown/shutdown_abort.inst
bin/default/source3/lib/netapi/examples/shutdown/shutdown_init.inst
bin/default/source3/lib/netapi/examples/user/user_add.inst
bin/default/source3/lib/netapi/examples/user/user_chgpwd.inst
bin/default/source3/lib/netapi/examples/user/user_del.inst
bin/default/source3/lib/netapi/examples/user/user_dispinfo.inst
bin/default/source3/lib/netapi/examples/user/user_enum.inst
bin/default/source3/lib/netapi/examples/user/user_getgroups.inst
bin/default/source3/lib/netapi/examples/user/user_getinfo.inst
bin/default/source3/lib/netapi/examples/user/user_getlocalgroups.inst
bin/default/source3/lib/netapi/examples/user/user_modalsget.inst
bin/default/source3/lib/netapi/examples/user/user_modalsset.inst
bin/default/source3/lib/netapi/examples/user/user_setgroups.inst
bin/default/source3/lib/netapi/examples/user/user_setinfo.inst
bin/default/source3/lib/netapi/tests/netapitest.inst
bin/default/source3/smbd/notifyd/notifyd-tests.inst
bin/default/source3/versiontest.inst

All examples and tests?  Showstoppers?  Not sure...
Comment 16 Karolin Seeger 2021-03-08 12:16:18 UTC
Thanks, Martin!
Still not 100% sure, so delaying the release(s).
4.13.5 is scheduled for tomorrow and also affected, so let's see what will happen until tomorrow...
Comment 17 Andrew Bartlett 2021-03-08 20:13:41 UTC
With all due respect to our normal rules advising against a revert and all due respect to OmniOS users, I think we should *revert the change* so things work as they did previously on Linux until someone has the time and resources to get this working and tested on a multi-platform basis.

Multi-platform build and linking bugs tend to be subtle and complex, I wouldn't suggest trying to come to a full fix on a deadline. 

Sorry,
Comment 18 Samba QA Contact 2021-03-09 11:02:25 UTC
This bug was referenced in samba v4-13-stable (Release samba-4.13.5):

eebf510fbd8847077c7bec72a1cda674b5a02714
Comment 19 Samba QA Contact 2021-03-09 12:38:51 UTC
This bug was referenced in samba v4-14-stable (Release samba-4.14.0):

4d1ed9c319deac5cba1682611dcefdf002cb9d48
Comment 20 Karolin Seeger 2021-03-10 08:12:01 UTC
(In reply to Samba QA Contact from comment #18)
but was reverted
Comment 21 Karolin Seeger 2021-03-10 08:13:08 UTC
Re-assigning to Andrew for further investigations.
Comment 22 Andrew Bartlett 2021-03-10 08:17:52 UTC
(In reply to Karolin Seeger from comment #21)
Lucky me!

But more seriously, just in case it wasn't clear to the reporter and anyone else interested, we have reverted the change because of the releases that were due out that otherwise would have been blocked.

Fixing this is clearly non-trivial, and any further patches are going to need clear proof that the Linux case is unchanged, both in terms of relevant parts of config.log or such, and in terms of the linking of the binaries which are output.

As confirming this under CI is proving a challenge, we will need to be particularly careful.
Comment 23 Peter Eriksson 2021-03-10 09:08:01 UTC
(In reply to Andrew Bartlett from comment #22)

One "quick-and-dirty" fix could be to only do this for the sys.platform == 'sunos5' case, since that's the only(? - as far as I know) platform (still in some use) where Gnu ld isn't used... 

(I know you've been reluctant to add OS-specific hacks before but that contains this fix to just this platform atleast :-)


--- samba-4.14.0/wscript        tors jan. 21 14:20:41 2021
+++ samba-4.14.0-liu/wscript    ons mars 10 09:51:23 2021
@@ -340,7 +340,11 @@
     # allows us to find problems on our development hosts faster.
     # It also results in faster load time.
 
-    conf.add_as_needed()
+    # Solaris based systems doesn't use gnu ld, and so no '--as-needed'
+    if sys.platform == 'sunos5':
+        conf.ADD_LDFLAGS('-Wl,--as-needed', testflags=True)
+    else:
+        conf.add_as_needed()
 
     if not conf.CHECK_NEED_LC("-lc not needed"):
         conf.ADD_LDFLAGS('-lc', testflags=False)
Comment 24 Björn Jacke 2021-03-10 13:49:18 UTC
such a OS specific exceptio hack if definetely not what we want to start.

The build regression on centos7 looks quite obviously like unclean waf build depencencies. A "proof that the Linux case is unchanged" looks like an impossible proof like the proof of the existence of god. We should find the wrong waf dependencies and fix that and reimplement the correct as-needed check again. I tried also adding the -z defs linker flags to our waf buiuld that we used to have before we switched to waf but that also resulted in various undefined symbol errors, that should actually not pop up.
Comment 25 Andrew Bartlett 2021-03-10 18:55:18 UTC
(In reply to Björn Jacke from comment #24)
We have often used this standard (that things that are meant to be unchanged are unchanged) for things like this.  

Often we compare config.h files or CPP output to ensure that what is suggested to be harmless changes to configure checks are indeed as harmless as they appear.
Comment 26 Samba QA Contact 2021-03-11 11:47:30 UTC
This bug was referenced in samba v4-12-stable (Release samba-4.12.12):

c63f00801cae16a855aa5283fa0cc449e84577ce
Comment 27 Martin Schwenke 2021-03-11 23:14:16 UTC
(In reply to Björn Jacke from comment #24)

> The build regression on centos7 looks quite obviously like unclean
> waf build depencencies. [...] We should find the wrong waf dependencies
> and fix that and reimplement the correct as-needed check again.

I don't think this is true.  The breakage involves private libraries that are part of samba-util (or, more precisely, private libraries that samba-util depends on).  These recursive dependencies are all linked.

However, as I previously commented, with the patch, the rpath magic for those recursive dependencies isn't properly handled.
Comment 28 Samba QA Contact 2021-04-07 03:17:03 UTC
This bug was referenced in samba master:

ff1c3af603b47a7e8f9faad8d1c2e4a489559155
Comment 29 Martin Schwenke 2021-04-12 07:32:14 UTC
I've backported https://gitlab.com/samba-team/samba/-/merge_requests/1874 to v4-13-test and v4-14-test.  CI pipelines are running now.  I'll also wait until these branches pass my overnight runs on CentOS 8.
Comment 30 Martin Schwenke 2021-04-12 10:50:52 UTC
Created attachment 16588 [details]
Patch for 4.14, 4.13

Pipelines passed for:

* 4.14 @ https://gitlab.com/samba-team/devel/samba/-/pipelines/284810020
* 4.13 @ https://gitlab.com/samba-team/devel/samba/-/pipelines/284809855

Before I add reviewers, waiting for:

* My overnight tests to confirm that these patches are OK on CentOS 8

* Peter Eriksson (bug submitter) to confirm that this patch fixes the
  build on OmniOS

Thanks...
Comment 31 Martin Schwenke 2021-04-13 04:20:44 UTC
My tests on CentOS 8 pass.

Peter, can you please confirm that this fixes the build on OmniOS?

I'm hoping  this can still make it into the next 4.14 release, which due about a week from now.

Thanks...
Comment 32 Peter Eriksson 2021-04-13 07:30:15 UTC
(In reply to Martin Schwenke from comment #31)

I can confirm that the patch works fine on OmniOS!
Comment 33 Martin Schwenke 2021-04-13 10:01:57 UTC
(In reply to Peter Eriksson from comment #32)

Thanks!
Comment 34 Karolin Seeger 2021-04-13 11:19:08 UTC
Pushed to autobuild-v4-{14,13}-test.
Comment 35 Samba QA Contact 2021-04-13 13:17:03 UTC
This bug was referenced in samba v4-13-test:

56156a8fd5432728b3d0526bb3ac3165ab5ebc90
Comment 36 Samba QA Contact 2021-04-14 07:35:14 UTC
This bug was referenced in samba v4-14-test:

12bfc43006347ad6f775181528b872c0a968f8cd
Comment 37 Karolin Seeger 2021-04-16 12:24:33 UTC
Closing out bug report.

Thanks!