LDAP should use module specific include dir

RESOLVED FIXED in mozilla1.2

Status

Directory
LDAP C SDK
RESOLVED FIXED
17 years ago
15 years ago

People

(Reporter: cls, Assigned: dmose)

Tracking

other
mozilla1.2

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 1 obsolete attachment)

(Reporter)

Description

17 years ago
LDAP should follow NSPR's example and use a module specific directory for its
public headers; private headers, of course, should not be exported.  This will
put LDAP in sync with the other efforts to modularize headers.  Since LDAP uses
a separate build system, I don't think REQUIRES should be used to find those
headers.
(Reporter)

Comment 1

17 years ago
Created attachment 48746 [details] [diff] [review]
Install ldap headers in dist/include/ldap
(Assignee)

Comment 2

17 years ago
r=dmose
(Reporter)

Comment 3

17 years ago
Patch has been checked in.  dmose says that a similar change needs to be made to
new ldap branch so reassigning to leif.
Assignee: cls → leif
(Reporter)

Comment 4

17 years ago
Changing product.
Component: Build Config → LDAP C SDK
Product: Browser → Directory
(Reporter)

Comment 5

17 years ago
need this for win32 too

OS: Linux → All
Hardware: PC → All
(Reporter)

Comment 6

17 years ago
Created attachment 49076 [details] [diff] [review]
Install win32 ldap headers in DIST_XP\include\ldap
(Assignee)

Comment 7

17 years ago
r=dmose in that cls' win32 changes look reasonable; I don't know the windows
build system well enough to be able to review them for completeness.

Comment 8

17 years ago
why dont we just use REQUIRES in the directories that use ldap instead of adding
it to config.mak? I kind of thought nspr was done that way because we don't
really need to track nspr dependencies because it's so fundamental
(Reporter)

Comment 9

17 years ago
That is probably why NSPR is done that way.  Someday when we have more time to
worry about correctness, I'd like to change that.  IMO, REQUIRES only applies to
the modules that are built as part of the Mozilla core build system.  This is
why ZLIB/JPEG/PNG/MNG_REQUIRES are only set when building the mozilla version of
the packages.  Otherwise, we rely upon setting <mod>_CFLAGS to pull the proper
headers.  LDAP should be treated the same way since it is a completely separate
entity (as opposed to zlib which has been adapted to our build system).

Adding the ldap include path to config.mak was just for convenience.  We can not
add it there and just add it to the handful of makefiles that will use it
(directory/xpcom/* & mailnews/addrbook/* iirc).

(Assignee)

Comment 10

17 years ago
Only adding it to the dirs where it's needed would avoid adding unnecessary IO
cycles to the build.  The directory/xpcom/base subtree should be the ONLY place
that needs to include those files directly.
(Reporter)

Comment 11

17 years ago
Include directly, yes.  But since nsLDAP.h & friends pull in ldap.h, the modules
that require mozldap will also require ldap.  It's still only a handful of
makefiles though.


(Assignee)

Comment 12

17 years ago
nsLDAP.h is going to go away; see bug 92650.
(Reporter)

Comment 13

17 years ago
Sorry, that should have been nsLDAPURL.h & friends (the ones exported from
directory/xpcom/base/src/) not nsLDAP.h
(Reporter)

Updated

17 years ago
Attachment #49076 - Attachment is obsolete: true
(Reporter)

Comment 14

17 years ago
Created attachment 49186 [details] [diff] [review]
Add LDAP_CFLAGS to win32 & use as necessary
(Reporter)

Comment 15

17 years ago
Created attachment 49187 [details] [diff] [review]
Only use LDAP_CFLAGS as necessary on unix

Comment 16

17 years ago
I like that better. I still like the idea of using REQUIRES somehow, because I'm
developing tools that make use of the value of REQUIRES.
(Assignee)

Comment 17

17 years ago
cls: nothing from the src directory should really have to be exported, I don't
think.  It's most likely a bug I introduced by copying other win32 makefiles,
although conceivably there's some weird win32 build system lossage that I was
hacking around.
(Reporter)

Comment 18

17 years ago
You're right. I went digging some more and it wasn't that dir, lxr shows that
addrbook directly includes ldap.h

/mailnews/addrbook/src/nsAbLDAPProperties.h, line 35 -- #include "ldap.h"

(Assignee)

Comment 19

17 years ago
cls: that's definitely a bug; that code should not be including ldap.h.
(Reporter)

Comment 20

17 years ago
Created attachment 50059 [details] [diff] [review]
Remove unneeded ldap.h include from addrbook
(Assignee)

Comment 21

17 years ago
r=dmose on all three, minus the hunks pertaining to
mailnews/addrbook/src/{makefile.win,Makefile.in}

Comment 22

17 years ago
sr=alecf on all three.
(Reporter)

Comment 23

17 years ago
Those 3 patches have been checked in.

Updated

16 years ago
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla0.9.7

Comment 24

16 years ago
Reassign to dmose.
Assignee: leif → dmose
Status: ASSIGNED → NEW

Comment 25

16 years ago
Moving out of 0.9.7.  Feel free to move them past 0.9.8.
Target Milestone: mozilla0.9.7 → mozilla0.9.8
(Assignee)

Comment 26

16 years ago
Mass change: pushing out 0.9.8 bugs, as I'm concentrating feature work now.
Target Milestone: mozilla0.9.8 → mozilla0.9.9

Updated

16 years ago
Target Milestone: mozilla0.9.9 → mozilla1.0

Comment 27

16 years ago
Moving Netscape owned 0.9.9 and 1.0 bugs that don't have an nsbeta1, nsbeta1+,
topembed, topembed+, Mozilla0.9.9+ or Mozilla1.0+ keyword.  Please send any
questions or feedback about this to adt@netscape.com.  You can search for
"Moving bugs not scheduled for a project" to quickly delete this bugmail.
Target Milestone: mozilla1.0 → mozilla1.2
(Assignee)

Comment 28

16 years ago
This bug was fixed with the landing of the LDAP C SDK v5.
Status: NEW → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.