223987 - Fix LDAP library makefile to play nice with "make -jN" builds with MSVC

Status: RESOLVED FIXED

(Directory :: LDAP C SDK, defect)

Description

[Kevin Teuscher]

I am building thunderbird on a W2K box and I am using 'make -j4' in my
.mozconfig to speed things up during the build.  However, when building the
ldap\libraries directories, link fails because there are targets that make the
.lib files and a separate target to make the .dll files.  The link command will
by default make both the .lib and the .dll file if no .lib file exists.  In a
parallel make mode, the target to make the .lib file and the target to make the
.dll will get run at the same time.  link then fails because it can't create the
.lib file (the other target is already trying to make it).

Solution: There are two options; 1. rewrite the makefiles so that if the .dll
target is to be run, the .lib target isn't -- more elegant, but beyond my
makefile hacking skills.  2. make the build of the ldap\libraries directories
not be in parallel even if the rest of the build is -- easily accomplished by a
single makefile addition.

Patch to implement #2 to follow momentarily.

This allows me to speed up my thunderbird builds a bit.  Don't have any
quantitative numbers to give however.

CC'ing mscott in case he is interested in speeding up his build time.

Kevin

Comment 1

[Kevin Teuscher]

Attachment: diff to disable parallel make in ldap libraries directories

Fix for option #2

Comment 2

[Mark Smith [:mcs]]

This issue came up for the "Mozilla Classic" builds also, but I can't remember
how it was addressed. If I remember correctly, something similar to what you
propose as a short term solution was done -- disabling parallel makes for the
LDAP code.

Comment 3

[Kevin Teuscher]

Mark:  Here are the bugs that I found that refer to parallel builds of LDAP stuff.

bug 132848 : This one was an order problem for multiple directories including LDAP
bug 133521 : This one is specifically about LDAP (it was for -j2; I haven't
tried with -j2)
bug 136297 : Problem with a missing directory

None of those are exactly the same problems; although I haven't tried my make
with -j2 instead of -j4 to see if that makes a difference.

After further twiddling with the makefile, I think I have been able to implement
a third option:  Make sure the .lib file is build before we try to make the .dll
file.  I did this by adding $(LIBXXX) {where XXX is the corresponding suffix} to
the dependencies at these locations:
http://lxr.mozilla.org/mozilla/source/directory/c-sdk/ldap/libraries/libldap/Makefile.in#303
http://lxr.mozilla.org/mozilla/source/directory/c-sdk/ldap/libraries/libprldap/Makefile.in#222
http://lxr.mozilla.org/mozilla/source/directory/c-sdk/ldap/libraries/libssldap/Makefile.in#229

This forces the .lib file to be built before the .dll and will allow my build to
run without problem.  I have only tested this on Win2K with MSVC, so it needs to
be checked on some other systems.  I will attach a patch tomorrow (away from my
build machine today)

Kevin

Comment 4

[Mark Smith [:mcs]]

Does Thunderbird use the LDAP code from ldapcsdk_50_client_branch or from the
trunk?  Ths fix made for bug 133521 looks like it might be relevant. But maybe
that fixed things only well enough for -j2 to work, not -j4.

Comment 5

[Kevin Teuscher]

Mark:
In the client.mk file at the top cvs tree /mozilla, the checkout tag defined is
ldapcsdk_50_client_branch (see
http://lxr.mozilla.org/mozilla/source/client.mk#59).  So I believe that it is
using the branch.  The changes made in bug 133521 (the v2 attachement) are what
is on the ldapcsdk_50_client_branch (at least what I get from CVS when I do the
TB checkout).

I am thinking out loud here, but I am wondering if with -j2, you wouldn't get
two make processes running in the same library directory, so you wouldn't have
the problem of two targets trying to make the same .lib file at the same time. 
But with a 'make -j4', you end up with multiple make processes running in the
directory and have one doing the LIBXXXX target and another doing the DLLXXXX
target at the same time.

cls would be a better person to ask, but if you replace the lines I indicated in
c3 with the following stuff, you can do a 'make -j4' build with MSVC.  Just to
be clear to anyone reading or for future reference, I have not tried this (or
the other patch I submitted) on any other platforms, so I don't know how they
will be affected by the changes.

Sorry about not having a patch for this yet; cvs and firewall issues at the
moment :(

in libraries/libldap/Makefile.in#303 ---> $(DLLLDAP): $(LIBLDAP) $(OBJS)
$(LIBDIR) $(LDAP_EXPORT_DEFS)

in libraries/libprldap/Makefile.in#222 ---> $(DLLPRLDAP): $(LIBPRLDAP) $(OBJS)
$(LIBDIR) $(PRLDAP_EXPORT_DEFS)

in libraries/libssldap/Makefile.in#229 ---> $(DLLSSLDAP): $(LIBSSLDAP) $(OBJS)
$(LIBDIR) $(SSLDAP_EXPORT_DEFS)

Kevin

Comment 6

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 134346 [details] [diff] [review]
diff to disable parallel make in ldap libraries directories

>Index: mozilla/directory/c-sdk/ldap/libraries/Makefile.in
>===================================================================
>RCS file: /cvsroot/mozilla/directory/c-sdk/ldap/libraries/Makefile.in,v
>retrieving revision 5.0.2.2
>diff -u -r5.0.2.2 Makefile.in
>--- mozilla/directory/c-sdk/ldap/libraries/Makefile.in	5 Apr 2002 03:51:39 -0000	5.0.2.2
>+++ mozilla/directory/c-sdk/ldap/libraries/Makefile.in	28 Oct 2003 21:00:56 -0000
>@@ -26,6 +26,13 @@
> 
> include $(MOD_DEPTH)/config/autoconf.mk
> 
>+ifeq ($(LD),link)
>+# link under MSVC does not like to be run in parallel when making
>+# .lib and .dll files with the same base name, but using different
>+# rules
>+MAKE := $(patsubst -j%,,$(MAKE)) -j1
>+endif
>+
> DIRS		= liblber libldif libiutil libldap libprldap
> 
> ifdef USE_NSS

I think this needs some other qualifier to test and see if we're running with
MSVC, since other compilers (possibly on other platforms) could have a linker
named "link".

Comment 7

[Kevin Teuscher]

Attachment: Better version

This is a better patch that allows me to do a 'make -j4' build on my PC with no
problems.  I changed the logic so that I only deviate from the current
procedure if running on a WIN32 box and my linker is named 'link'.  This should
only occur in an MSVC build (I think).

Kevin

Comment 8

[Kevin Teuscher]

Comment on attachment 136952 [details] [diff] [review]
Better version

Just asking for a review again

Comment 9

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 136952 [details] [diff] [review]
Better version

r=dmose.  As usual, this needs to land on both the branch and the CVS tip.

Comment 10

[Dan Mosedale (:dmosedale, :dmose)]

mcs: if you want to reassign to me for landing purposes, feel free.

Comment 11

[Mark Smith [:mcs]]

-> dmose so this will land soon (I am swamped at the moment with other work).

Comment 12

[Kevin Teuscher]

dmose:  Who needs to sr this?  There is no rush, since I assume that this will
not go in until the tree opens for 1.7a.  Just wanted to keep it moving.

Kevin

Comment 13

[Kevin Teuscher]

Comment on attachment 136952 [details] [diff] [review]
Better version

Setting flag to get on a radar somewhere

Comment 14

[Simon Paquet [:sipaq]]

Comment on attachment 136952 [details] [diff] [review]
Better version

also asking dmose for sr. Don't know who else could superreview this.

Comment 15

[Kevin Teuscher]

Comment on attachment 136952 [details] [diff] [review]
Better version

Asking for sr from someone else.  I assume that dmose is busy and can't get to
it.

Comment 16

[David :Bienvenu]

Comment on attachment 136952 [details] [diff] [review]
Better version

sr=bienvenu

Comment 17

[Kevin Teuscher]

David or Mark or dmose

I don't have cvs rights, so one of you will need to checkin.  As dmose pointed
out in c9 http://bugzilla.mozilla.org/show_bug.cgi?id=223987#c9 , this needs to
go on both the branch and the tip.

Comment 18

[Kevin Teuscher]

Maybe if I ask really, really, really nicely, someone with cvs access will check
this in so we don't have to wait until 1.8alpha.

Kevin

Comment 19

[David :Bienvenu]

I'll try to get approval for this

Comment 20

[Asa Dotzler [:asa]]

Comment on attachment 136952 [details] [diff] [review]
Better version

a=asa (on behalf of drivers) for checkin to 1.7 Beta

Comment 21

[David :Bienvenu]

fix checked in to 5.0.2 branch - Dan, who would check this into the ldap trunk?

Comment 22

[Mark Smith [:mcs]]

I will commit this to the LDAP trunk within a day or two.

Comment 23

[Mark Smith [:mcs]]

Finally committed to the trunk:

Commit to the trunk the fix for bug # 223987 - Fix LDAP library
    makefile to play nice with "make -jN" builds with MSVC.
mozilla/directory/c-sdk/ldap/libraries/libldap/Makefile.in
  new revision: 5.15; previous revision: 5.14
mozilla/directory/c-sdk/ldap/libraries/libprldap/Makefile.in
  new revision: 5.10; previous revision: 5.9