Closed Bug 1112904 Opened 10 years ago Closed 10 years ago

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

Categories

(Directory :: LDAP C SDK, defect)

All
FreeBSD
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: mi+mozilla, Assigned: jbeich)

References

Details

Attachments

(1 file, 2 obsolete files)

Attached patch patch-recursivemake.py (obsolete) (deleted) — Splinter Review
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.
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...
Attachment #8538157 - Attachment is patch: true
Attachment #8538157 - Attachment mime type: text/x-python → text/plain
Assignee: nobody → mi+mozilla
Status: NEW → ASSIGNED
Product: SeaMonkey → Core
Comment on attachment 8538157 [details] [diff] [review] patch-recursivemake.py r? jcranmer as well as this patch affects LDAP
Attachment #8538157 - Flags: review?(nfroyd)
Attachment #8538157 - Flags: review?(Pidgeot18)
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.
Attachment #8538157 - Flags: review?(nfroyd)
Attachment #8538157 - Flags: review?(Pidgeot18)
Attachment #8538157 - Flags: review-
This is probably a sibling of bug 1107063.
Assignee: mi+mozilla → nobody
Status: ASSIGNED → NEW
Depends on: 1107063
Attached patch backport bug 125857 (obsolete) (deleted) — Splinter Review
Would backporting the following fix also help bug 925217? https://hg.mozilla.org/projects/nspr/rev/5040c01ccb8a
Attachment #8540516 - Flags: review?(standard8)
Attachment #8540516 - Flags: feedback?(mi+mozilla)
Blocks: 1036894
Component: Build Config → LDAP C SDK
Depends on: 125857
No longer depends on: 1107063
Product: Core → Directory
Hardware: x86 → All
Version: unspecified → other
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 on attachment 8540516 [details] [diff] [review] backport bug 125857 I'm happy for Joshua to review this.
Attachment #8540516 - Flags: review?(standard8) → review?(Pidgeot18)
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?
(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.
(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.
(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).
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.
(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 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.
Attachment #8540516 - Flags: review?(mh+mozilla)
Attachment #8540516 - Flags: review?(Pidgeot18)
Attachment #8540516 - Flags: feedback?(mi+mozilla)
Attachment #8540516 - Flags: feedback+
Comment on attachment 8540516 [details] [diff] [review] backport bug 125857 Oops, didn't mean to clear this.
Attachment #8540516 - Flags: feedback?(mi+mozilla)
(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 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.
Attachment #8540516 - Flags: review?(mh+mozilla) → review+
Attached patch backport bug 125857, untabified (deleted) — Splinter Review
(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.
Assignee: nobody → jbeich
Attachment #8540516 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8540516 - Flags: feedback?(mi+mozilla)
Attachment #8570425 - Flags: review?(mh+mozilla)
Attachment #8570425 - Flags: review?(mh+mozilla) → review+
Keywords: checkin-needed
Attachment #8538157 - Attachment is obsolete: true
How to nominate the patch for inclusion in Thunderbird 38 or maybe even SeaMonkey 2.34 ? Its NPOTB-ness was tested in comment 6.
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
(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.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: