1126607 - Kill the LDAP build system

Status: RESOLVED FIXED

(MailNews Core :: LDAP Integration, defect)

Description

[Joshua Cranmer [:jcranmer]]

I'm sorely tempted to name this bug "Take the LDAP build system out back and shoot it," which describes my current feelings of the build system.

It ought to be well known at this point by everyone interested in this bug that the LDAP build system is outdated and receives rather little maintenance; given the nature of its quasi-independent repository, it's also annoying to do any updates.

A better solution would be to move to OpenLDAP (which we've wanted to do since, oh, 2010?), but that's not a quick fix and is probably best shelved until after we do the address book rewrite (ensemble).

Comment 1

[Joshua Cranmer [:jcranmer]]

Attachment: Draft approach

Here's a draft approach I want feedback on... using the same source code, but wrapping the entire library stuff within mozilla-central. This patch only really works on Linux for the moment, though.

Comment 2

[Mike Hommey [:glandium]]

Comment on attachment 8555579 [details] [diff] [review]
Draft approach

Review of attachment 8555579 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry this took so long.

Why not just add the moz.build files to the ldap repo directly? The risk of being our-of-sync is the same whether they're in-tree or out-of tree, but the files would be less horrible if they were in the ldap repo.

Comment 3

[Joshua Cranmer [:jcranmer]]

(In reply to Mike Hommey [:glandium] from comment #2)
> Comment on attachment 8555579 [details] [diff] [review]
> Draft approach
> 
> Review of attachment 8555579 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Sorry this took so long.
> 
> Why not just add the moz.build files to the ldap repo directly? The risk of
> being our-of-sync is the same whether they're in-tree or out-of tree, but
> the files would be less horrible if they were in the ldap repo.

Partially, it's to avoid dealing with the headache of finding proper owners for LDAP build system and ensuring whether or not they're okay with it. I'm not sure how active the owners are--and completely replacing/bypassing the build system in ldap-sdks is something that needs their input, as opposed to any build system peer's.

Comment 4

[Joshua Cranmer [:jcranmer]]

Another complication:

The moz.build files are reliant on Makefile.in files. The LDAP C-SDK build system is not trivially portable to moz.build, not even in the same semi-standalone sense that js/src is. It's also impossible to keep these two build systems side-by-side, since they both interpret Makefile.in files but do so very differently. So moz.build can't be added into the LDAP C SDK repo. It is plausible, though, that we could check the C SDK files into the repository directly and ditch the build system that way.

Comment 5

[Joshua Cranmer [:jcranmer]]

Attachment: Alternative LDAP patch

This is an approach that replaces the LDAP makefiles with moz.build files. It's a patch to the ldap/sdks repo since it's easier for me to test locally that way, but the idea would be to import the ldap/sdks code into c-c and then apply this patch (along with other changes, e.g., dropping the ldap/sdks configure). ldap-sdk.mozbuild is basically the same as in attachment 8555579 [details] [diff] [review], except I added this bit at the bottom:
if CONFIG['OS_ARCH'] == 'WINNT':
    get_ldap_name = lambda simple: 'ns%s32v60' % simple
else:
    get_ldap_name = lambda simple: '%s60' % simple

Comment 6

[Mark Smith [:mcs]]

As has been pointed out, there is very little activity under directory/c-sdk.  For that reason, the proposal to import the code into c-c and switch to moz.build files makes a lot of sense to me.  I don't consider it a true forking of the code because I don't expect tbird engineers (or anyone) to suddenly start a lot of new LDAP development with this code.

Comment 7

[Mike Hommey [:glandium]]

Comment on attachment 8568597 [details] [diff] [review]
Alternative LDAP patch

Review of attachment 8568597 [details] [diff] [review]:
-----------------------------------------------------------------

::: c-sdk/ldap/include/Makefile.in
@@ +66,4 @@
>  $(LDAP_STD_HEADER): ldap-standard-tmpl.h
>  	$(PERL) $(srcdir)/../build/replace.pl \
> +			LDAP_VENDOR_NAME="mozilla.org" \
> +			LDAP_VENDOR_VERSION="607" \

You could replace {{ and }} in that file with @, and use our preprocessor instead.

Comment 8

[Mike Hommey [:glandium]]

(In reply to Joshua Cranmer [:jcranmer] from comment #5)
> if CONFIG['OS_ARCH'] == 'WINNT':
>     get_ldap_name = lambda simple: 'ns%s32v60' % simple
> else:
>     get_ldap_name = lambda simple: '%s60' % simple

Is the different lib name really necessary? I doubt it. I can see how it might be useful for libldap as a separate project. Not in c-c.

Comment 9

[Joshua Cranmer [:jcranmer]]

Attachment: MozReview Request: bz://1126607/jcranmer

/r/6757 - Bug 1126607, part 1: Import the LDAP C-SDK into comm-central.
/r/6759 - Bug 1126607, part 2: Integrate the imported code into moz.build, r=glandium.
/r/6761 - Bug 1126607, part 3: Stop downloading the LDAP C-SDK repository, r=glandium.
/r/6763 - Bug 1126607, part 4: Simplify the LDAP build system further, glandium.

Pull down these commits:

hg pull -r 592e450fa366ccf77a780eb96eac6d4d40ba23bc https://reviewboard-hg.mozilla.org/comm-central

Comment 10

[Joshua Cranmer [:jcranmer]]

Comment on attachment 8590031 [details]
MozReview Request: bz://1126607/jcranmer

Asking for feedback to make sure that *BSDs are happy with these changes (hard to test non-tier 1 platforms).

Comment 11

[Landry Breuil (:gaston)]

I'd be happy to test this but how do i import those patches in my local MQ without totally messing up my hg clone ? I guess the hg pull -r thing, but how am i supposed to cleanup afterwards/return back to a clean tree state ?

Comment 12

[Joshua Cranmer [:jcranmer]]

(In reply to Landry Breuil (:gaston) from comment #11)
> I'd be happy to test this but how do i import those patches in my local MQ
> without totally messing up my hg clone ? I guess the hg pull -r thing, but
> how am i supposed to cleanup afterwards/return back to a clean tree state ?

You can use hg strip to remove the imported patches.

Comment 13

[Landry Breuil (:gaston)]

Comment on attachment 8590031 [details]
MozReview Request: bz://1126607/jcranmer

With patches from #1151829 and #1150659 under mozilla & your 4 patches from that review, i can build c-c on openbsd, so yay from me!

Comment 14

[Yonggang Luo]

https://reviewboard.mozilla.org/r/6759/#review6013

Ship It!

Comment 15

[Joshua Cranmer [:jcranmer]]

https://hg.mozilla.org/comm-central/rev/f9f973c875f8
https://hg.mozilla.org/comm-central/rev/2b00be2ded7f
https://hg.mozilla.org/comm-central/rev/557498f58325
https://hg.mozilla.org/comm-central/rev/24203ada3e1e

Comment 16

[Landry Breuil (:gaston)]

All i can say is yay again \o/ Thanks jcranmer

Comment 17

[Yonggang Luo]

Can this be port back to tb-38?

Comment 18

[:aceman]

Comment on attachment 8555579 [details] [diff] [review]
Draft approach

Looks like this approach was not chosen.

Comment 19

[:aceman]

(In reply to Yonggang Luo from comment #17)
> Can this be port back to tb-38?

What would be the benefit?

I assume this causes no user-visible functionality changes.

Comment 20

[Joshua Cranmer [:jcranmer]]

(In reply to Yonggang Luo from comment #17)
> Can this be port back to tb-38?

This change is risky enough (and quite frankly, insufficiently important) to avoid needing to be backported.

Comment 21

[Philip Chee]

Why does part 1 say:
author [e-mail]
?

Comment 23

[Joshua Cranmer [:jcranmer]]

Attachment: MozReview Request: Bug 1126607, part 3: Stop downloading the LDAP C-SDK repository, r=glandium.

Comment 24

[Joshua Cranmer [:jcranmer]]

Attachment: MozReview Request: Bug 1126607, part 4: Simplify the LDAP build system further, glandium.

Comment 25

[Joshua Cranmer [:jcranmer]]

Attachment: MozReview Request: Bug 1126607, part 1: Import the LDAP C-SDK into comm-central.

Comment 26

[Joshua Cranmer [:jcranmer]]

Attachment: MozReview Request: Bug 1126607, part 2: Integrate the imported code into moz.build, r=glandium.