1112904 - Dependencies generated by recursivemake.py use build-time, rather than run-time directories

Status: RESOLVED FIXED

(Directory :: LDAP C SDK, defect)

Description

[Mikhail T.]

Attachment: patch-recursivemake.py

When compiling with the LDAP option enabled, the libraries (libxul.so and others) end up with the following dependencies (as listed by ldd):

....
        ../../ldap/sdks/c-sdk/ldap/libraries/libldap/libldap60.so (0)
        ../../ldap/sdks/c-sdk/ldap/libraries/libldif/libldif60.so (0)
        ../../ldap/sdks/c-sdk/ldap/libraries/libprldap/libprldap60.so (0)
....

The path refers to the build-time locations and the libraries -- which are installed right next to libxul.so itself -- are not found. The problem stems from comm-release/obj-FOO/toolkit/library/backend.mk containing the following lines:
SHARED_LIBS += $(DEPTH)/ldap/sdks/c-sdk/ldap/libraries/libldap/libldap60.so
SHARED_LIBS += $(DEPTH)/ldap/sdks/c-sdk/ldap/libraries/libldif/libldif60.so
SHARED_LIBS += $(DEPTH)/ldap/sdks/c-sdk/ldap/libraries/libprldap/libprldap60.so

the file is generated by mozilla/python/mozbuild/mozbuild/backend/recursivemake.py, which the proposed patch modifies to produce the proper lines instead:

SHARED_LIBS += -L$(DEPTH)/ldap/sdks/c-sdk/ldap/libraries/libldap -lldap60
SHARED_LIBS += -L$(DEPTH)/ldap/sdks/c-sdk/ldap/libraries/libldif -lldif60
SHARED_LIBS += -L$(DEPTH)/ldap/sdks/c-sdk/ldap/libraries/libprldap -lprldap60

I'm fairly certain, this affects not just FreeBSD, but most other Unix-builds -- when LDAP is requested. For this reason, FreeBSD port www/seamonkey currently refuses to even build, if the user tries to enable LDAP support.

Comment 1

[Mikhail T.]

Come to think of it, the original (unpatched) recursivemake.py, probably, works fine on Windows. The new code is Unix-only. I'm sure, this controversy is properly addressed elsewhere, but the LDAP-functionality is not enabled by default and thus is not getting as much testing...

Comment 2

[Philip Chee]

Comment on attachment 8538157 [details] [diff] [review]
patch-recursivemake.py

r? jcranmer as well as this patch affects LDAP

Comment 3

[Mike Hommey [:glandium]]

Comment on attachment 8538157 [details] [diff] [review]
patch-recursivemake.py

Review of attachment 8538157 [details] [diff] [review]:
-----------------------------------------------------------------

There is no reason to change this. The actual problem is probably that the ldap libraries are built without a soname.

Comment 4

[Mike Hommey [:glandium]]

This is probably a sibling of bug 1107063.

Comment 5

[Jan Beich]

Attachment: backport bug 125857

Would backporting the following fix also help bug 925217?

https://hg.mozilla.org/projects/nspr/rev/5040c01ccb8a

Comment 6

[Jan Beich]

https://treeherder.mozilla.org/ui/#/jobs?repo=try-comm-central&revision=16fd61cbacf9
https://trillian.chruetertee.ch/freebsd-gecko/changeset/1796/

Comment 7

[Jan Beich]

Comment on attachment 8540516 [details] [diff] [review]
backport bug 125857

Review of attachment 8540516 [details] [diff] [review]:
-----------------------------------------------------------------

::: c-sdk/configure.in
@@ +2376,5 @@
> +	    AC_DEFINE(_REENTRANT)
> +	    AC_DEFINE(_THREAD_SAFE)
> +	    dnl -pthread links in -lc_r, so don't specify it explicitly.
> +	    if test "$ac_cv_have_dash_pthread" = "yes"; then
> +	        _PTHREAD_LDFLAGS="-pthread"

DragonFly needs -pthread handling per
https://github.com/DragonFlyBSD/DeltaPorts/blob/12b0960/ports/devel/nspr/dragonfly/patch-nsprpub_configure.in

Comment 8

[Mark Banner (:standard8)]

Comment on attachment 8540516 [details] [diff] [review]
backport bug 125857

I'm happy for Joshua to review this.

Comment 9

[Mikhail T.]

Comment on attachment 8540516 [details] [diff] [review]
backport bug 125857

Review of attachment 8540516 [details] [diff] [review]:
-----------------------------------------------------------------

This seems like it would work, yes. However...

Only the first hunk of the patch -- to c-sdk/config/FreeBSD.mk -- seems relevant to this bug. Patching configure may be addressing some other problem...

I'm not sure, how the *.mk are laid out -- it would seem, recording of the soname should be common among (almost) all Unixes. I also don't understand, why the c-sdk has its own makefile-collection -- which seems to bit-rot every once in a while because LDAP is disabled by default. Would it not be better to build these optional pieces using the same rules as the rest of seamonkey's shared libraries?

Comment 10

[Jan Beich]

(In reply to Mikhail T. from comment #9)
> Only the first hunk of the patch -- to c-sdk/config/FreeBSD.mk -- seems
> relevant to this bug. Patching configure may be addressing some other
> problem...

suite/configure invokes ldap/sdks/c-sdk/configure which generates
.../config/autoconf.mk and is later included by .../libraries/*.
It's similar to NSPR build except unused <OS>.mk are long gone there.

https://github.com/mozilla/gecko-dev/commit/bef48de
https://github.com/mozilla/gecko-dev/commit/8fae92a

> Would it not be better to build these optional pieces using the same
> rules as the rest of seamonkey's shared libraries?

Do you mean to ignore standalone build and convert to moz.build?
Otherwise, LDAP needs more modern build system and NSPR-inspired
wouldn't be one of such.

Comment 11

[Mikhail T.]

(In reply to Jan Beich from comment #10)
> (In reply to Mikhail T. from comment #9)
> > Would it not be better to build these optional pieces using the same
> > rules as the rest of seamonkey's shared libraries?
> 
> Do you mean to ignore standalone build and convert to moz.build?
> Otherwise, LDAP needs more modern build system and NSPR-inspired
> wouldn't be one of such.

I, actually, do not know, how the rest of the project is built. But I do know, that only the LDAP pieces manifest bitrot every once in a while -- the rest of seamonkey's shared libraries must be built differently. Can the LDAP pieces not be built the same way -- whatever that way is?

If, on the other hand, the LDAP pieces can be built separately, perhaps we should make them shareable by all Mozilla applications -- the way NSPR and NSS can already be (and, indeed, ARE -- on FreeBSD and friends) shared.

Comment 12

[Joshua Cranmer [:jcranmer]]

(In reply to Mikhail T. from comment #11)
> (In reply to Jan Beich from comment #10)
> > (In reply to Mikhail T. from comment #9)
> > > Would it not be better to build these optional pieces using the same
> > > rules as the rest of seamonkey's shared libraries?
> > 
> > Do you mean to ignore standalone build and convert to moz.build?
> > Otherwise, LDAP needs more modern build system and NSPR-inspired
> > wouldn't be one of such.
> 
> I, actually, do not know, how the rest of the project is built. But I do
> know, that only the LDAP pieces manifest bitrot every once in a while -- the
> rest of seamonkey's shared libraries must be built differently. Can the LDAP
> pieces not be built the same way -- whatever that way is?
> 
> If, on the other hand, the LDAP pieces can be built separately, perhaps we
> should make them shareable by all Mozilla applications -- the way NSPR and
> NSS can already be (and, indeed, ARE -- on FreeBSD and friends) shared.

NSPR, NSS, and LDAP are notionally independently-developed projects from the Gecko/comm-central core. As a result, they all have their own build systems independent from the shared moz.build infrastructure for mozilla-central and comm-central. However, whereas NSPR and NSS are used by various other projects, to my knowledge, LDAP has now become used solely by Thunderbird and SeaMonkey, and its development appears to have basically halted except for the occasional build system fixes.

This notional independence would make a conversion to moz.build perhaps difficult, but forking/importing the source into comm-central and converting the rest to moz.build is certainly possible at first glance (I don't think there's anything complex in its files, not that I've looked all that hard). One reason that this hasn't been done (beyond the fact that the LDAP build system has mostly been a mild annoyance) is that I've contemplated instead replacing the LDAP code with OpenLDAP or system LDAP, a decision which has not met much opposition from anyone else in the community (nor much more support--mostly votes of apathy).

Comment 13

[Mikhail T.]

Well, then, let's have something committed to put out the current fire, and move towards building these pieces independently (like NSS and NSPR). The consumer-projects (Thunderbird and Seamonkey) can then have configure-switches added for --system-ldap (or --system-c-sdk).

Switch to OpenLDAP would, probably, constitute a larger undertaking still. I'd certainly welcome it, but -- like those apathetic others -- would not promise any actual coding help.

Comment 14

[Joshua Cranmer [:jcranmer]]

(In reply to Mikhail T. from comment #13)
> Well, then, let's have something committed to put out the current fire, and
> move towards building these pieces independently (like NSS and NSPR). The
> consumer-projects (Thunderbird and Seamonkey) can then have
> configure-switches added for --system-ldap (or --system-c-sdk).

I don't think you understood me fully. LDAP *is* a fully independent build system (like NSS and NSPR), which is the crux of the problem: it's an independent project with basically no desire to maintain it. There's no --system-ldap piece because, well, since LDAP is unused outside of comm-central, no one has bothered to build a system package for it and subsequently desired to use that system package for TB or SM.

Comment 15

[Joshua Cranmer [:jcranmer]]

Comment on attachment 8540516 [details] [diff] [review]
backport bug 125857

Review of attachment 8540516 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry for taking so long to get to this to patch.

As I'm not really knowledgeable to any significant degree about LDAP's build system or FreeBSD (or Dragonfly), I can only give a cursory examination of the patch.

Comment 16

[Joshua Cranmer [:jcranmer]]

Comment on attachment 8540516 [details] [diff] [review]
backport bug 125857

Oops, didn't mean to clear this.

Comment 17

[Joshua Cranmer [:jcranmer]]

(In reply to Joshua Cranmer [:jcranmer] from comment #12)
> This notional independence would make a conversion to moz.build perhaps
> difficult, but forking/importing the source into comm-central and converting
> the rest to moz.build is certainly possible at first glance (I don't think
> there's anything complex in its files, not that I've looked all that hard).

cf. Bug 1126607

Comment 18

[Mike Hommey [:glandium]]

Comment on attachment 8540516 [details] [diff] [review]
backport bug 125857

Review of attachment 8540516 [details] [diff] [review]:
-----------------------------------------------------------------

::: c-sdk/configure.in
@@ +2332,5 @@
>     ${CC-cc} -pthread -o conftest conftest.c > conftest.out 2>&1
>     if test $? -eq 0; then
>  	if test -z "`egrep -i '(unrecognize|unknown)' conftest.out | grep pthread`" && test -z "`egrep -i '(error|incorrect)' conftest.out`" ; then
>  	    ac_cv_have_dash_pthread=yes
> +		case "$target_os" in

splinter may or may not be off, but there might be an indentation problem here and below.

Comment 19

[Jan Beich]

Attachment: backport bug 125857, untabified

(In reply to Mike Hommey [:glandium] from comment #18)
> >  	    ac_cv_have_dash_pthread=yes
> > +		case "$target_os" in
> 
> splinter may or may not be off, but there might be an indentation problem
> here and below.

I've just tried to be inconsistently consistent with NSPR. The tabs
come from bug 125857. Here's the version with them removed in + lines.

Converted via |untabify| after applying Emacs modeline in -*-.
However, the preprocessed file implicitly uses 8 spaces per tab which
breaks whitespace alignment in some places but is an unrelated issue,
not to mention indenting dnl lines ends up increasing indentation of
the following line.

Comment 20

[Jan Beich]

How to nominate the patch for inclusion in Thunderbird 38 or maybe even SeaMonkey 2.34 ? Its NPOTB-ness was tested in comment 6.

Comment 21

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

https://hg.mozilla.org/projects/ldap-sdks/rev/0f7c3698a961

Comment 22

[Philip Chee]

(In reply to Jan Beich from comment #20)
> How to nominate the patch for inclusion in Thunderbird 38 or maybe even
> SeaMonkey 2.34 ? Its NPOTB-ness was tested in comment 6.

First you probably need to tag 0f7c3698a961 as LDAPCSDK_6_0_7J_RTM
https://hg.mozilla.org/projects/ldap-sdks/graph/0f7c3698a961
See example:
https://hg.mozilla.org/projects/ldap-sdks/rev/ff2dacee5458

Then update:
http://mxr.mozilla.org/comm-central/source/client.py?rev=840242ab89dd&mark=20-20#18
to LDAPCSDK_6_0_7J_RTM
Also update comm-beta, comm-aurora as well if necessary.