506601 - Transition LDAP SDKs to Mercurial (from cvs)

Status: VERIFIED FIXED

(MailNews Core :: Build Config, defect)

Description

[Serge Gautherie (:sgautherie)]

+++ This bug was initially created as a clone of Bug #437064 +++

Or at least a mirror (at
http://hg.mozilla.org/cvs-trunk-mirror/file/default/ or elsewhere)


Fwiw, from a SeaMonkey p-o-v, DOMi and Venkman are now in hg, CZ stayed in cvs only.

Comment 1

[Mark Banner (:standard8)]

This is something I've been meaning to bring up for discussion for a while - on mozilla.dev.tech.ldap not here (bugzilla isn't a discussion forum, and this should have discussion first). I'll try and post something there later today.

Comment 2

[Mark Banner (:standard8)]

The message thread is here: http://groups.google.com/group/mozilla.dev.tech.ldap/msg/07608796e5e37cc4

Please post any discussion to that thread.

Comment 3

[Mark Banner (:standard8)]

I'm currently working through some of this with the LDAP guys. Should have more to report soon.

Comment 4

[Mark Banner (:standard8)]

Attachment: Draft Patch

I've been discussing this offline with the ldap folks. There is a test repository here that has been set up which has imported the code and a relevant branch from cvs:

http://hg.mozilla.org/users/bugzilla_standard8.plus.com/ldap-sdks/

The main location has been suggested as:

http://hg.mozilla.org/ldap-sdks/

Note that this includes more that just the ldap c-sdk, hence the change in name.

I'm waiting for a couple more days for final confirmation, but once I get that I'll file a bug to get the repository created for us.

The patch attached does the work to get the ldap-sdks repo included in comm-central. I've also moved from using /directory/ to /ldap/ as it is a better description of what is actually in that folder.

I've not removed the old cvs directory, just in case anyone has changes in it.

The url in client.py is currently pointing to the test repository which, of course, will need updating when we get the new one set up.

Comment 5

[Justin Wood (:Callek)]

Comment on attachment 491503 [details] [diff] [review]
Draft Patch


>-#  "COMM", "MOZILLA", "CHATZILLA", "INSPECTOR" or "VENKMAN"
>+#  "COMM", "MOZILLA", "CHATZILLA", "INSPECTOR", "VENKMAN", "LDAPCSDK"

nit: LDAPSDK or LDAPSDKS since the hg repo will have more than CSDK (same for following vars)

Comment 6

[Mark Banner (:standard8)]

(In reply to comment #4)
> The main location has been suggested as:
> 
> http://hg.mozilla.org/ldap-sdks/

Following discussion on bug 613921 the revised suggestion is to use:

http://hg.mozilla.org/projects/ldap-sdks/

as this probably doesn't need to be a top-level directory.

If there's comments against this (or for a better location) please comment within the next 24 hours.

Comment 7

[Mark Banner (:standard8)]

Attachment: The fix

Updated patch to point to the now created and filled-out repository.

Also takes account of the latest changes to client.py.

I'll take whoever reviews it first.

Note that I'm planning on landing this in comm-central first for a couple of days, then porting to comm-1.9.2 and comm-1.9.1 after that. I'm aiming to get this into comm-1.9.* in time for the builds of the security releases.

Comment 8

[Robert Kaiser]

Comment on attachment 492596 [details] [diff] [review]
The fix

>diff --git a/client.py b/client.py
>-def do_cvs_checkout(modules, tag, cvsroot, cvs, checkoutdir):

yay for killing cvs! :)

>+def fixup_ldap_repo_options(options):

Hmm, why exactly do we need fixup_ldap_repo_options here? It sounds superfluous to me...

>diff --git a/configure.in b/configure.in
>--- a/configure.in
>+++ b/configure.in
>@@ -7281,14 +7281,17 @@ MOZ_BUILD_APP="$MOZ_BUILD_APP_CACHED"
> if test -n "$COMPILE_ENVIRONMENT" -a -n "$MOZ_LDAP_XPCOM"; then
>     # these subdirs may not yet have been created in the build tree.
>     # don't use the "-p" switch to mkdir, since not all platforms have it
>-    if test ! -d "directory/c-sdk/ldap"; then
>-        if test ! -d "directory/c-sdk"; then
>-            if test ! -d "directory"; then
>-                mkdir "directory"
>+    if test ! -d "ldap/sdks/c-sdk/ldap"; then
>+        if test ! -d "ldap/sdks/c-sdk"; then
>+            if test ! -d "ldap/sdks"; then
>+                if test ! -d "ldap"; then
>+                    mkdir "ldap"
>+                fi
>+                mkdir "ldap/sdks"
>             fi
>-            mkdir "directory/c-sdk"
>+            mkdir "ldap/sdks/c-sdk"
>         fi
>-        mkdir "directory/c-sdk/ldap"
>+        mkdir "ldap/sdks/c-sdk/ldap"
>     fi


Why exactly do we need this still? It sounds like a lot of hackery to me...

Comment 9

[Mark Banner (:standard8)]

(In reply to comment #8)
> >+def fixup_ldap_repo_options(options):
> 
> Hmm, why exactly do we need fixup_ldap_repo_options here? It sounds superfluous
> to me...

Well, it follows the style of the other functions in there. We could probably make sorting out all those options easier, but I can't see a way at a quick glance, and it'd be a separate bug anyway.

> >diff --git a/configure.in b/configure.in


> >+    if test ! -d "ldap/sdks/c-sdk/ldap"; then
> >+        if test ! -d "ldap/sdks/c-sdk"; then
> >+            if test ! -d "ldap/sdks"; then
> >+                if test ! -d "ldap"; then
> >+                    mkdir "ldap"
> >+                fi
> >+                mkdir "ldap/sdks"
> >             fi
> >-            mkdir "directory/c-sdk"
> >+            mkdir "ldap/sdks/c-sdk"
> >         fi
> >-        mkdir "directory/c-sdk/ldap"
> >+        mkdir "ldap/sdks/c-sdk/ldap"
> >     fi
> 
> 
> Why exactly do we need this still? It sounds like a lot of hackery to me...

I'm pretty sure its for objdir builds - we need to ensure that the target directory is created for the configure to run properly.

Comment 10

[Robert Kaiser]

(In reply to comment #9)
> (In reply to comment #8)
> > >+def fixup_ldap_repo_options(options):
> > 
> > Hmm, why exactly do we need fixup_ldap_repo_options here? It sounds superfluous
> > to me...
> 
> Well, it follows the style of the other functions in there. We could probably
> make sorting out all those options easier, but I can't see a way at a quick
> glance, and it'd be a separate bug anyway.

We only have fixup functions where we need to replace an existing dir with something else and move away old dirs for that. This is not the case here, so I don't think we need it.

> > Why exactly do we need this still? It sounds like a lot of hackery to me...
> 
> I'm pretty sure its for objdir builds - we need to ensure that the target
> directory is created for the configure to run properly.

Hmm, I wonder if we could get that into the comment. IMHO, that sounds like it really should happen inside the c-sdk configure though, ideally. Maybe we should create a bug for moving it there.

Comment 11

[Mark Banner (:standard8)]

(In reply to comment #10)
> (In reply to comment #9)
> > (In reply to comment #8)
> > > >+def fixup_ldap_repo_options(options):
> > > 
> > > Hmm, why exactly do we need fixup_ldap_repo_options here? It sounds superfluous
> > > to me...
> > 
> > Well, it follows the style of the other functions in there. We could probably
> > make sorting out all those options easier, but I can't see a way at a quick
> > glance, and it'd be a separate bug anyway.
> 
> We only have fixup functions where we need to replace an existing dir with
> something else and move away old dirs for that. This is not the case here, so I
> don't think we need it.

Well mozilla and inspector both no longer do anything with their dirs, so they are in the same position. Although the functions are really fixing up the options for the repository they apply to, so I don't think they are that far out in naming.

> > > Why exactly do we need this still? It sounds like a lot of hackery to me...
> > 
> > I'm pretty sure its for objdir builds - we need to ensure that the target
> > directory is created for the configure to run properly.
> 
> Hmm, I wonder if we could get that into the comment. IMHO, that sounds like it
> really should happen inside the c-sdk configure though, ideally. Maybe we
> should create a bug for moving it there.

Not sure, but we do the same thing with mozilla/ if that hasn't been created, its just not as complex.

Comment 12

[Mark Banner (:standard8)]

This is now checked in. I've filed bug 614294 for getting mxr updated to remove the old cvs files.

http://hg.mozilla.org/comm-central/rev/da7dab70b9d2

Comment 13

[Serge Gautherie (:sgautherie)]

V.Fixed, per bug 614294 comment 2.

At last :-)

Comment 14

[Serge Gautherie (:sgautherie)]

(In reply to comment #6)
> http://hg.mozilla.org/projects/ldap-sdks/

Ftr, wrt c-sdk,
*You "merged" .cvsignore files into .hgignore.
*You removed ldap/examples/Makefile.

Comment 15

[Mark Banner (:standard8)]

Attachment: Port to 1.9.2

I've discussed this with the TB build team and whilst we normally wouldn't back-port a change like this, due to the frequent failures that we're seeing from cvs, especially at release time, we have decided in this instance to back-port the change.

This patch is based on the trunk patch but is minus the change of location (i.e. I've not moved directory/ to ldap/).

Comment 16

[Mark Banner (:standard8)]

Attachment: Port to 1.9.2 v2

Correct version of the patch - forgot to hg qrefresh.

Comment 17

[Justin Wood (:Callek)]

(In reply to comment #15)
> Created attachment 504400 [details] [diff] [review]
> Port to 1.9.2
> 
> I've discussed this with the TB build team and whilst we normally wouldn't
> back-port a change like this, due to the frequent failures that we're seeing
> from cvs, especially at release time, we have decided in this instance to
> back-port the change.

Assuming gozer r+'s this, any chance you could whip up a version for us all for 1.9.1 for suite's benefit (and/or review a 1.9.1 port too); I'd like to get a+ from KaiRo on a plan for that [even if over IRC] though.

Comment 18

[Mark Banner (:standard8)]

(In reply to comment #17)
> (In reply to comment #15)
> > Created attachment 504400 [details] [diff] [review]
> > Port to 1.9.2
> > 
> > I've discussed this with the TB build team and whilst we normally wouldn't
> > back-port a change like this, due to the frequent failures that we're seeing
> > from cvs, especially at release time, we have decided in this instance to
> > back-port the change.
> 
> Assuming gozer r+'s this, any chance you could whip up a version for us all for
> 1.9.1 for suite's benefit (and/or review a 1.9.1 port too); I'd like to get a+
> from KaiRo on a plan for that [even if over IRC] though.

Last I spoke to KaiRo he didn't want to transition 1.9.1. I also don't have any spare time this week, but I expect the patch will largely apply straight-off if you want it.

Comment 19

[Mark Banner (:standard8)]

Moving this to mailnews core, as the patch was all mailnews and I need the flags that are there ;-)

Comment 20

[Mark Banner (:standard8)]

Checked into 1.9.2: http://hg.mozilla.org/releases/comm-1.9.2/rev/86e548c2dde3