Closed Bug 223987 Opened 21 years ago Closed 20 years ago

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

Categories

(Directory :: LDAP C SDK, defect)

Product:
Directory
Component:
LDAP C SDK
Platform:
x86
Windows 2000
Type:
defect
Priority:
Not set
Severity:
minor

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
mozilla1.7

People

(Reporter: klteuscher, Assigned: Bienvenu)

Details

Attachments

(1 file, 1 obsolete file)

Better version
2.83 KB, patch
dmosedale
: review+
Bienvenu
: superreview+
asa
: approval1.7b+
Details | Diff | Splinter Review 
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
Fix for option #2
Attachment #134346 - Flags: review?(dmose)
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.
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

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.
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 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".
Attachment #134346 - Flags: review?(dmose) → review-
Attached patch Better version — — Splinter Review 
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
Attachment #134346 - Attachment is obsolete: true
Comment on attachment 136952 [details] [diff] [review]
Better version

Just asking for a review again
Attachment #136952 - Flags: review?(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.
Attachment #136952 - Flags: review?(dmose) → review+
mcs: if you want to reassign to me for landing purposes, feel free.
-> dmose so this will land soon (I am swamped at the moment with other work).
Assignee: MarkCSmithWork → dmose
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 on attachment 136952 [details] [diff] [review]
Better version

Setting flag to get on a radar somewhere
Attachment #136952 - Flags: superreview?
Comment on attachment 136952 [details] [diff] [review]
Better version

also asking dmose for sr. Don't know who else could superreview this.
Attachment #136952 - Flags: superreview? → superreview?(dmose)
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.
Attachment #136952 - Flags: superreview?(dmose) → superreview?(brendan)
Comment on attachment 136952 [details] [diff] [review]
Better version

sr=bienvenu
Attachment #136952 - Flags: superreview?(brendan) → superreview+
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.
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla1.7
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
I'll try to get approval for this
Assignee: dmose → bienvenu
Status: ASSIGNED → NEW
Comment on attachment 136952 [details] [diff] [review]
Better version

a=asa (on behalf of drivers) for checkin to 1.7 Beta
Attachment #136952 - Flags: approval1.7b+
fix checked in to 5.0.2 branch - Dan, who would check this into the ldap trunk?
Status: NEW → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
I will commit this to the LDAP trunk within a day or two.
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
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: