325625 - Upgrade Mozilla code base to use the latest appropriate LDAP C-SDK.

Status: RESOLVED FIXED

(MailNews Core :: LDAP Integration, defect)

Description

[Mark Banner (:standard8)]

SeaMonkey & Thunderbird currently both build with version 5.0 of the LDAP C-SDK. This was branched from the trunk LDAP C-SDK back in 2002.

Dan & I think now would be a good time to upgrade to the latest version of the LDAP C-SDK, to pull in the latest fixes etc.

The next versions of SeaMonkey & Thunderbird will be taken from the 1.8 branch, and so we have a nice long time to check for any regressions on the trunk. If it turns out that mozilla trunk seems to be working fine for most users then we can copy it across to the 1.8 branch as well.

I'm currently experimenting with the LDAPCSDK_5_1_6_RTM version and will report back once its compiled & I've used it a bit.

We would like comments from Mark & Rich as to which LDAPCSDK version would be best to use, or if it would be better to wait for the next release.

Comment 1

[Rich Megginson]

I'm currently working on what will become LDAPCSDK_5_1_7_RTM.  This version has a more autoconf friendly build, will produce RPM packages suitable for use with Fedora Core 5 and other linuxes, and will contain a number of bug and security fixes.  If you want to build on unix/linux, you can pull from HEAD.  I'm sitting on some Windows build improvement changes, waiting for review.  If you can, please try out the HEAD and let me know.

Comment 2

[Mark Smith [:mcs]]

I think it would be good to cut a new branch from the LDAP C SDK trunk once Rich is done with his currently flurry of activity.  The biggest risk is that some change that landed on the branch used by Seamonkey and TBird did not land on the trunk.  But those would mainly be portability fixes so if there is any serious "fallout" it would be fixed quickly.

Comment 3

[Rich Megginson]

What is the current SeaMonkey & Thunderbird LDAP C-SDK branch?

Comment 4

[Mark Smith [:mcs]]

(In reply to comment #3)
> What is the current SeaMonkey & Thunderbird LDAP C-SDK branch?

ldapcsdk_50_client_branch

I did some digging in my email folders and in bugzilla looking for bugs that may have landed on the branch but not on the trunk.  The one candidate I found is bug 188247.  Of course I may have missed something.

Comment 5

[Mark Banner (:standard8)]

(In reply to comment #2)
> I think it would be good to cut a new branch from the LDAP C SDK trunk once
> Rich is done with his currently flurry of activity.

Agreed, LDAPCSDK_5_1_6_RTM builds fine on Linux with trunk SeaMonkey, however the latest doesn't due to the rpm changes (I think). I'm currently recompiling my trees to see if I can come up with something that will work (when I do I'll post it of course).

(In reply to comment #4)
> I did some digging in my email folders and in bugzilla looking for bugs that
> may have landed on the branch but not on the trunk.  The one candidate I found
> is bug 188247.  Of course I may have missed something.

I'm going to do a short comparison between bug numbers using bonsai just to help verify. Still might miss something, but its probably worth the risk.

Comment 6

[Mark Banner (:standard8)]

(In reply to comment #5)
> I'm going to do a short comparison between bug numbers using bonsai just to
> help verify. Still might miss something, but its probably worth the risk.
> 
I've just done this. The bugs listed below are ones I found in bonsai as in the check-in comments for branch but not for trunk c-sdk.

bug 136756 - The makefiles that this changed have been replaced by Makefile.in. Would these need modifying?

bug 231934 comment 5 - says it was checked in on trunk.
bug 232742 comment 10 - says it was checked in on trunk.
bug 54828 comment 12 - says it was checked in on trunk.
bug 294122 - I verified that this has been checked in on trunk by comparison of the diffs and the current trunk files.

(In reply to comment #5)
> The one candidate I found is bug 188247.

Bonsai reports that as being checked in to trunk on 2004-03-14 22:25

Comment 7

[Rich Megginson]

> bug 136756 - The makefiles that this changed have been replaced by Makefile.in.
> Would these need modifying?

It's hard to tell.  Did you look at the diffs in bonsai?  The files are completely different.  I would say that, if it builds, then those changes can be ignored.

> Agreed, LDAPCSDK_5_1_6_RTM builds fine on Linux with trunk SeaMonkey, however
> the latest doesn't due to the rpm changes (I think). I'm currently recompiling
> my trees to see if I can come up with something that will work (when I do I'll
> post it of course).

What problems are you having?  Perhaps it's something I can help with.

Comment 8

[Mark Banner (:standard8)]

Attachment: Fixes finding nspr in trunk on linux.

I was going to leave this for a while longer, but as you've asked... I have now been able to compile SeaMonkey trunk with the c-sdk trunk, with a couple of modifications. I haven't tried out running it or Thunderbird yet, but I expect the results with Thunderbird will be pretty much the same.

The patch I'm attaching is necessary for the c-sdk to find nspr in the SeaMonkey/Thunderbird builds on linux at least. I'm not sure if its valid for other platforms or not.

Additionally, I have had to comment out the entire svrcore section in configure - its not included by default on SeaMonkey/Thunderbird builds and its not included in my distribution, and there is no way to disable it.

Comment 9

[Rich Megginson]

Attachment: patch for nspr.m4

This would should work with the mozilla client and standalone builds.

Comment 10

[Rich Megginson]

Attachment: patch for svrcore.m4

I'm not sure what the problem is with svrcore, but try this patch to see if that helps.

Comment 11

[Mark Banner (:standard8)]

(In reply to comment #10)
> Created an attachment (id=210627) [edit]
> patch for svrcore.m4
> 
> I'm not sure what the problem is with svrcore, but try this patch to see if
> that helps.
> 
The problem with svrcore is that we're not building it for SeaMonkey/Thunderbird. I'm not sure that we really need to enable it either at the moment (especially as we can build without it, but correct me if I am wrong).

I have tried --without-svrcore in a similar way to --without-nss but I still get the following when I configure:

checking for --with-svrcore-lib... no
/opt/mozmaster/mozilla/directory/c-sdk/configure: line 2825: cd: ../../dist/*.OBJ/lib: No such file or directory
/opt/mozmaster/mozilla/directory/c-sdk/configure: line 2830: cd: ../../dist/public/svrcore: No such file or directory
checking could not find in-tree SVRCORE in ../../dist... no
checking Trying pkg-config svrcore... checking for pkg-config... (cached) /usr/bin/pkg-config
configure: error: system SVRCORE not found
configure: error: /opt/mozmaster/mozilla/directory/c-sdk/configure failed for directory/c-sdk
*** Fix above errors and then restart with "make -f client.mk build"

Comment 12

[Mark Banner (:standard8)]

Comment on attachment 210625 [details] [diff] [review]
patch for nspr.m4

This line:

+    for nsprpath in "$1" "$1"/*.OBJ ; do    

needs to be:

+    for nsprpath in "$1/" "$1"/*.OBJ ; do    

and the nspr section will work fine.

Comment 13

[Rich Megginson]

Attachment: incorporate comments from bz@standard8

add '/' to end of $1

Comment 14

[Rich Megginson]

Attachment: new patch includes configure as well

I apologize for my bad autoconf fu.  I think this cleans up the build considerably.  If you don't care at all about svrcore, it will just skip everything having to do with svrcore.

Comment 15

[Mark Banner (:standard8)]

(In reply to comment #14)
> Created an attachment (id=210664) [edit]
> new patch includes configure as well
> 
> I apologize for my bad autoconf fu.  I think this cleans up the build
> considerably.  If you don't care at all about svrcore, it will just skip
> everything having to do with svrcore.
> 

Ok, my turn to apologise as I didn't realise this before - the nspr change still doesn't work with a clean build of SeaMonkey/Thunderbird.

The problem is that the dist/ directory hasn't been created before configure is run, hence it can't find the in-tree directory.

This gives us with some problems. As the dist/ directory isn't created, we can't use in-tree detection, my next thought was to use the main SeaMonkey/Thunderbird configure script to pass the details of nspr (e.g. --with-nspr-inc={whatever}/dist/include/nspr --with-nspr-lib={whatever}/dist/lib) to the c-sdk script, but that won't work either because again the dist/ directories don't exist and we check that they do before proceeding.

Looking at what the previous version of the configure scripts, it seems like it fell back to mozilla nspr by default. I don't know if that would still be acceptable/possible here. My autoconf/build system knowledge isn't too good. Perhaps Benjamin could advise.

btw the configure script attached, and possibly the m4 file need a slight mod:

# Check whether --with-svrcore or --without-svrcore was given.

in this section, I get "error: no not found" as we haven't done a test to see if $withval is "no".

Comment 16

[Rich Megginson]

> Looking at what the previous version of the configure scripts, it seems like it
> fell back to mozilla nspr by default. I don't know if that would still be
> acceptable/possible here. My autoconf/build system knowledge isn't too good.
> Perhaps Benjamin could advise.

What do you mean by "mozilla nspr" above?  Do you mean it would just default to looking for nspr under mozilla/dist/include/nspr?

I suppose I could just get rid of the code that looks for nspr.h etc. and just assume that if it is not found, just set the include path to mozilla/dist/include/nspr etc.  But in order to do this, I need to know that this is part of a client build - there must be some environment variable or configure flag or the presence of some directory or something to let configure know that it is ok to just assume that path.  Otherwise, after finding no in-tree NSPR, it will look for the "system" NSPR.

On Fedora Core 5 and later Red Hat OSes, NSPR will be part of the operating system as the nspr package.  Likewise with NSS.  This is what the ldap c sdk calls "system" nspr (e.g. configure --with-system-nspr).  It uses "pkg-config nspr", "pkg-config mozilla-nspr", and "nspr-config" to find out where the include files and libraries are.  This may be undesired when building on linux - you may want to force the ldap c sdk build to use the in-tree nspr rather than any system NSPR it may find.  One solution would be to use configure --with-nspr --without-system-nspr - then it would tell it that you wanted to use nspr but not any system nspr that it found.

I'd like to know how difficult it would be to change the client build, and what the existing environment variables and configure options passed to the ldap c sdk configure are.

Comment 17

[Mark Banner (:standard8)]

(In reply to comment #16)
> What do you mean by "mozilla nspr" above?  Do you mean it would just default to
> looking for nspr under mozilla/dist/include/nspr?

I meant the in-(mozilla)-tree nspr code under mozilla/nsprpub which does create the header under mozilla/dist/include/nspr.

If you look at the current SeaMonkey/Thunderbird branched version of mozilla/directory/c-sdk/configure.in:

http://lxr.mozilla.org/seamonkey/source/directory/c-sdk/configure.in#239

you can see that it will fall back to use "default" settings for the in-tree nspr code (which are actually out of date with the main configure.in http://lxr.mozilla.org/seamonkey/source/configure.in#3635) 

> 
> I'd like to know how difficult it would be to change the client build, and what
> the existing environment variables and configure options passed to the ldap c
> sdk configure are.

It wouldn't be very difficult to change, assuming we get agreement from the build config people. The lines in configure.in start around here:

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/configure.in&rev=1.1606#7601

I think we could change that, but we have to remember to respect options passed in to mozilla/configure which are very similar to the ones the c-sdk presents as well.

Comment 18

[Rich Megginson]

Excellent!  Thanks.  So if either --with-mozilla is used, or the environment variable MOZILLA_CLIENT is set, the ldap c sdk build can assume it is being built as part of the mozilla client build.

One more question: the --with-nspr flag is not passed to the ldap c sdk configure from the client configure script AFAICT.  Does the client always use NSPR?  Should the ldap c sdk configure just assume that if --with-mozilla is set, it should implicitly set --with-nspr as well?

Comment 19

[Rich Megginson]

Anyone know the answer to https://bugzilla.mozilla.org/show_bug.cgi?id=325625#c18
?

Comment 20

[Benjamin Smedberg]

> Excellent!  Thanks.  So if either --with-mozilla is used, or the environment
> variable MOZILLA_CLIENT is set, the ldap c sdk build can assume it is being
> built as part of the mozilla client build.

I missed why we need to make this distinction (as long as we solve the NSPR issue below)... please explain. We can certainly pass --with-mozilla when we call ldapcsdk configure from the main configure script.

> One more question: the --with-nspr flag is not passed to the ldap c sdk
> configure from the client configure script AFAICT.  Does the client always use
> NSPR?  Should the ldap c sdk configure just assume that if --with-mozilla is
> set, it should implicitly set --with-nspr as well?

No. The mozilla build supports system-NSPR build configurations. We need to pass --with-nspr when we're using either the in-tree NSPR, and use the system NSPR when --with-system-nspr is passed to the root configure. Note that we may also support --with-system-nspr=<path> in the future, according to bug 324283.

Comment 21

[Rich Megginson]

> I missed why we need to make this distinction (as long as we solve the NSPR
> issue below)... please explain. We can certainly pass --with-mozilla when we
> call ldapcsdk configure from the main configure script.

According to http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/configure.in&rev=1.1606#7601 the ldap c sdk is already being called with --with-mozilla, so it would be easy to change the ldap c sdk configure to assume --with-mozilla implies --with-nspr (if --with-nspr or --with-system-nspr is not also specified - that's not clear from the configure script - I assume that if you run the main client configure with --with-nspr or --with-system-nspr, it will set ac_configure_args to include that flag).
The reason for the distinction is that you can build the mozilla ldap c sdk _without_ nspr.  AFAICT by looking at the client branch of the ldap c sdk configure script at http://lxr.mozilla.org/seamonkey/source/directory/c-sdk/configure.in#239 it always uses nspr.

So my proposal is to change configure in the trunk ldap c sdk so that --with-mozilla implies --with-nspr if neither --with-nspr nor --with-system-nspr is explicitly provided.  Is this ok?

Comment 22

[Rich Megginson]

Attachment: new patch - assumes nspr if --with-mozilla or MOZILLA_CLIENT

Please try this patch.

Comment 23

[Rich Megginson]

With the latest diff, I was able to successfully build and run thunderbird.  This is what I did:
cvs co -r MOZILLA_1_8_BRANCH mozilla/client.mk
cd mozilla
make -f client.mk checkout MOZ_CO_PROJECT=mail
cd ..
cvs co -A -P DirectorySDKSourceC
cd mozilla
patch -p0 < latest.patch
edit ~/.mozconfig to be this:
 . $topsrcdir/mail/config/mozconfig
 mk_add_options MOZ_OBJDIR=@TOPSRCDIR@/built
 mk_add_options MOZ_CO_PROJECT=mail
 ac_add_options --disable-tests
make -f client.mk  build
cd built/dist/bin
./thunderbird

Everything seems to work fine - addressbook lookups, even typedown addressing.

Comment 24

[Rich Megginson]

Checkin to HEAD.  Once I get a couple more bug fixes in, I will tag the tree as LDAPCSDK_5_1_7_RTM.

Checking in directory/c-sdk/build.mk;
/cvsroot/mozilla/directory/c-sdk/build.mk,v  <--  build.mk
new revision: 5.29; previous revision: 5.28
done
Checking in directory/c-sdk/configure;
/cvsroot/mozilla/directory/c-sdk/configure,v  <--  configure
new revision: 5.45; previous revision: 5.44
done
Checking in directory/c-sdk/configure.in;
/cvsroot/mozilla/directory/c-sdk/configure.in,v  <--  configure.in
new revision: 5.46; previous revision: 5.45
done
Checking in directory/c-sdk/config/autoconf.mk.in;
/cvsroot/mozilla/directory/c-sdk/config/autoconf.mk.in,v  <--  autoconf.mk.in
new revision: 5.9; previous revision: 5.8
done
Checking in directory/c-sdk/config/autoconf/nspr.m4;
/cvsroot/mozilla/directory/c-sdk/config/autoconf/nspr.m4,v  <--  nspr.m4
new revision: 5.2; previous revision: 5.1
done
Checking in directory/c-sdk/config/autoconf/nss.m4;
/cvsroot/mozilla/directory/c-sdk/config/autoconf/nss.m4,v  <--  nss.m4
new revision: 5.3; previous revision: 5.2
done
Checking in directory/c-sdk/config/autoconf/svrcore.m4;
/cvsroot/mozilla/directory/c-sdk/config/autoconf/svrcore.m4,v  <--  svrcore.m4
new revision: 5.3; previous revision: 5.2
done

Comment 25

[Benjamin Smedberg]

Rich, I need to understand your use of -rpath (or -Wl,-R which appears to have the same effect) in build.mk more. Because the mozilla products are relocatable apps, we shun use of -rpath in favor of various other techniques like using -rpath-link (yes I know it's not all that portable), explicitly setting LD_LIBRARY_PATH, etc.

But perhaps I'm just misreading the build.mk file and don't understand what the RPATH instructions there are actually for.

Comment 26

[Rich Megginson]

The rpath stuff is only there for the command line tools like ldapsearch, which aren't needed by the client and are not built.  You must use make ... BUILDCLU=1 to build the command line utilities and invoke the rpath related code.

Comment 27

[Mark Banner (:standard8)]

(In reply to comment #23)
> With the latest diff, I was able to successfully build and run thunderbird. 
> ...snip...
> Everything seems to work fine - addressbook lookups, even typedown addressing.
> 

Ditto, except with using SeaMonkey as well.

(In reply to comment #24)
> Checkin to HEAD.  Once I get a couple more bug fixes in, I will tag the tree as
> LDAPCSDK_5_1_7_RTM.

Sounds good, but I'd like to know that benjamin is happy with the changes before we update the mozilla branch of the c-sdk.

Comment 28

[Mark Banner (:standard8)]

Attachment: Update client.mk to point to the new branch.

This patch updates client.mk to point to the new ldap c-sdk branch created for Mozilla from the tag LDAPCSDK_5_1_7_RTM.

I'll request some reviews/approvals once I've done some final checking on my local build.

The main plan is to get this in on trunk and let it roast for a couple of weeks and if everything seems to be ok, then we'll take it to the 1.8.1 branch.

Comment 29

[Mark Banner (:standard8)]

Comment on attachment 214696 [details] [diff] [review]
Update client.mk to point to the new branch.

Requesting acceptance reviews for upgrading SeaMonkey & Thunderbird to the ldap c-sdk 5.1.7.

We have a client branch from the 5.1.7 tag (see my comment 28).

It'll be left on the trunk for at least a week or two before I look for agreement for transferring it to branch.

Comment 30

[David :Bienvenu]

when I repulled and built with this change, I got cvs conflicts in the ldap cdk Makefile's - have you seen that? If this is going to break tinderbox builds, we need to have someone on hand who can clean it up...

Comment 31

[Rich Megginson]

Which makefiles gave a CVS error?

Comment 32

[Mark Banner (:standard8)]

(In reply to comment #30)
> when I repulled and built with this change, I got cvs conflicts in the ldap cdk
> Makefile's - have you seen that? If this is going to break tinderbox builds, we
> need to have someone on hand who can clean it up...
> 

Strange, it works fine for me going from one to the other. You haven't got any existing mods hanging around have you?

I was going to announce it in a few places before I did it, but I'll see if I can co-ordinate with someone who would be able to fix up tinderboxen if they need it.

Comment 33

[David :Bienvenu]

I didn't have any local mods (this was a very new tree that I pulled from scratch recently)

There were four of them, e.g., 

mozilla\directory\c-sdk\ldap\libraries\libssldap\Makefile

and ldap/libraries/libldif/Makefile
ldap/libraries/libiutil/Makefile
ldap/clients/tools/Makefile

this is on Windows. The changes look like they were made by autoconfig, where it puts my tree path in the Makefile. Perhaps there's something wrong with my build environment, but it's something to look out for.

Index: ldap/libraries/libiutil/Makefile
===================================================================
RCS file: /cvsroot/mozilla/directory/c-sdk/ldap/libraries/libiutil/Attic/Makefil
e,v
retrieving revision 5.0.2.7
diff -u -p -8 -r5.0.2.7 Makefile
--- ldap/libraries/libiutil/Makefile    11 Feb 2004 04:01:16 -0000      5.0.2.7
+++ ldap/libraries/libiutil/Makefile    16 Mar 2006 19:06:34 -0000
@@ -18,18 +18,18 @@
 # Copyright (C) 1998-1999 Netscape Communications Corporation. All
 # Rights Reserved.
 #
 # Contributor(s):
 #


 MOD_DEPTH      = ../../..
-srcdir         = .
-topsrcdir      = ../../..
+srcdir         = /cygdrive/c/tbirdtags/mozilla/directory/c-sdk/ldap/libraries/l
ibiutil
+topsrcdir      = /cygdrive/c/tbirdtags/mozilla/directory/c-sdk

 include $(MOD_DEPTH)/config/autoconf.mk
 include $(topsrcdir)/build.mk

 ifeq ($(HAVE_CCONF), 1)
 SRCS           = iutil-lock.c
 else
 SRCS           = iutil-lock.c

Comment 34

[Mark Banner (:standard8)]

(In reply to comment #33)
> I didn't have any local mods (this was a very new tree that I pulled from
> scratch recently)
> 
> There were four of them, e.g., 
> 
> mozilla\directory\c-sdk\ldap\libraries\libssldap\Makefile
> 
> and ldap/libraries/libldif/Makefile
> ldap/libraries/libiutil/Makefile
> ldap/clients/tools/Makefile
> 

Well these no longer exist I believe. Looking at the build logs for the linux boxes in SeaMonkey the following files are modified:

M mozilla/directory/c-sdk/config/Makefile
M mozilla/directory/c-sdk/ldap/libraries/libssldap/Makefile

No such modification exists for mac or windows. So I'll make sure I've got someone with access to them when I check the patch in.

Comment 35

[Mark Banner (:standard8)]

Comment on attachment 214696 [details] [diff] [review]
Update client.mk to point to the new branch.

Changing review request to benjamin to get approval from the build-config side as well.

Comment 36

[Benjamin Smedberg]

Comment on attachment 214696 [details] [diff] [review]
Update client.mk to point to the new branch.

This change in particular is fine: I'd like to review a changeset between the build-config of the two tags, or at least a pointer to significant differences.

Comment 37

[Mark Banner (:standard8)]

Attachment: Build Config diffs from 5.0 to 5.1.7

This diff is the build-config changes between the current 5.0 branch head and 5.1.7. I've removed a lot of the license header changes to make it a bit clearer. I've also removed the configure script differences - its ~ 369k and I'll attach it if you want, but as its generated from configure.in, I'm guessing that will be enough.

I generated these by pulling each branch, and then filtering out files that aren't relevant.

Comment 38

[Benjamin Smedberg]

Comment on attachment 215788 [details] [diff] [review]
Build Config diffs from 5.0 to 5.1.7

Cursory inspection looks good.

Comment 39

[Mark Banner (:standard8)]

This was checked in earlier today, and thanks to a little tinderbox administration by Coop, everything seems to now be ok -> Fixed.

btw. bug 327864 had to be checked into the new branch to fix universal mac builds.

Comment 40

[Leon Sha]

This check in cause bug 333406.