486540 - Abstracted build factories need to take comm-central's mozilla subdir into account

Status: RESOLVED FIXED

(Release Engineering :: General, defect)

Description

[Robert Kaiser]

I started to look into getting the abstracted buildbotcustom factories to work with SeaMonkey, based on the work done in bug 478229, and now I noted that there are multiple cases where we need to take the mozilla subdir of comm-central into account.
We can't just add this to the objdir as the usual build system calls need/should happen from the top level, but where we refer to dist/, _tests/, etc. or to scripts in testing/ or tools/ on the srcdir side, we need to prefix those with "mozilla/" (or "mozilla\" in win32 paths, yay!) when doing a comm-central product.
We should be able to abstract that away through some config.py var, but I wonder about the best way to do so.

Comment 1

[Aki Sasaki (not active)]

How about a variable in init that defaults to '.' but can be overridden in the master.cfg?

Comment 2

[Aki Sasaki (not active)]

We have two approaches that might work.

One would be to inherit the base factories and override the functions that need overriding. This may work but may require more abstraction work underneath.

The second is comment #1; could you go through the factory steps and determine which ones need this additional subdirectory, and how they need to be specified?

Gozer: is this something you need as well?

Comment 3

[Philippe M. Chiasson (:gozer)]

(In reply to comment #2)
> We have two approaches that might work.
> 
> One would be to inherit the base factories and override the functions that need
> overriding. This may work but may require more abstraction work underneath.

Yes, the problem with this one becomes how do you configure which factory class to use in a given instance (with my try server work fresh in my mind)

> The second is comment #1; could you go through the factory steps and determine
> which ones need this additional subdirectory, and how they need to be
> specified?

Yes, the idea would be to somehow separate between comm-central's objdir and mozilla's objdir.

> Gozer: is this something you need as well?

Yes, just letting KaiRo do all the hard work for now, but, yes, that's something we'd like to be able to use in Thunderbird land as well.

Comment 4

[Robert Kaiser]

(In reply to comment #2)
> One would be to inherit the base factories and override the functions that need
> overriding. This may work but may require more abstraction work underneath.

And it would require a lot more overhead on our side. I'm not even sure yet how to avoid some overhead with what we have, but I'd really like to avoid adding even more needed overhead.

> The second is comment #1; could you go through the factory steps and determine
> which ones need this additional subdirectory, and how they need to be
> specified?

I think that or something like that is the best way to do it, all comm-central applications need those things the same way anyhow.

Comment 5

[Robert Kaiser]

Attachment: WIP 1: untested try to get mozillaDir in

This is an untested WIP version of a patch for this for MercurialBuildFactory and the classes based on it. Ben, please review just the general direction this is going, I still need to set up a test master and slave somewhere and make a comm-central factory subclass that can be used to check if I caught all places where we need this (and did the correct thing in all of them).
For all configs existing so far, this should collapse to no change at all.

Comment 6

[[e-mail] (:bhearsum)]

Comment on attachment 375235 [details] [diff] [review]
WIP 1: untested try to get mozillaDir in

>diff --git a/process/factory.py b/process/factory.py
>+        # Mozilla subdi string in unix and native dir notation
>+        if mozillaDir:
>+            self.unixMozillaDir = '/%s' % mozillaDir
>+            if self.platform.startswith("win32"):
>+                self.nativeMozillaDir = '\\%s' % mozillaDir
>+            else:
>+                self.nativeMozillaDir = self.unixMozillaDir
>+        else:
>+            self.unixMozillaDir = ''
>+            self.nativeMozillaDir = self.unixMozillaDir
>+

I don't see nativeMozillaDir being used - what's the story there? It looks to me as its existence is necessitated by the difference in path separators. If that's the case, you probably don't need it. Because we start Buildbot from within MSYS (and IIRC you do too) we can use the same single forward slash separators as unix.

Also, there is a ton of '%s%s....' % (self.objdir, self.unixMozillaDir) usage below. Is there any reason not to just append unixMozillaDir to objdir? Doing that would make sure we reduce the chance of new code being incompatible, too.

Comment 7

[Robert Kaiser]

(In reply to comment #6)
> I don't see nativeMozillaDir being used - what's the story there?

We probably don't need it in those classes right now, but we are using a native path in the unit test classes (with the different separators), so if we ever want to inherit from MercurialBuildFactory for those, we need this probably.

> Also, there is a ton of '%s%s....' % (self.objdir, self.unixMozillaDir) usage
> below. Is there any reason not to just append unixMozillaDir to objdir? Doing
> that would make sure we reduce the chance of new code being incompatible, too.

The problem is that we don't need this in all cases where we use objdir, we sometimes actually need to top level objdir and sometimes need the Mozilla objdir.

Comment 8

[[e-mail] (:bhearsum)]

(In reply to comment #7)
> (In reply to comment #6)
> > I don't see nativeMozillaDir being used - what's the story there?
> 
> We probably don't need it in those classes right now, but we are using a native
> path in the unit test classes (with the different separators), so if we ever
> want to inherit from MercurialBuildFactory for those, we need this probably.
>

Can we leave it out, for now? I suspect all of the unittest things would work with unix style paths, too, and we can cross that bridge when we come to it. (Or maybe we'll just end up using the UnittestPackagedBuildFactory before this becomes an issue).

> > Also, there is a ton of '%s%s....' % (self.objdir, self.unixMozillaDir) usage
> > below. Is there any reason not to just append unixMozillaDir to objdir? Doing
> > that would make sure we reduce the chance of new code being incompatible, too.
> 
> The problem is that we don't need this in all cases where we use objdir, we
> sometimes actually need to top level objdir and sometimes need the Mozilla
> objdir.

Hmm...that makes things more difficult! I wonder if it'd be better to have self.objdir and self.mozillaObjdir, rather than dealing with string construction all over the place.

Anyways, proviso deferring the nativeMozillaDir part I'm fine with this approach.

Comment 9

[Robert Kaiser]

(In reply to comment #8)
> (In reply to comment #7)
> > (In reply to comment #6)
> > > I don't see nativeMozillaDir being used - what's the story there?
> > 
> > We probably don't need it in those classes right now, but we are using a native
> > path in the unit test classes (with the different separators), so if we ever
> > want to inherit from MercurialBuildFactory for those, we need this probably.
> >
> 
> Can we leave it out, for now? I suspect all of the unittest things would work
> with unix style paths, too, and we can cross that bridge when we come to it.
> (Or maybe we'll just end up using the UnittestPackagedBuildFactory before this
> becomes an issue).

Sure. When I started work on this I thought we might have more places using native paths, but it looks like http://hg.mozilla.org/build/buildbotcustom/file/c2d328b2bb48/process/factory.py#l2083 (and the respective place in CCUnittestBuildFactory) is the only place we have so far where we're using that.

> Hmm...that makes things more difficult! I wonder if it'd be better to have
> self.objdir and self.mozillaObjdir, rather than dealing with string
> construction all over the place.

We could do that, but it wouldn't save us from still using self.unixMozillaDir (or self.mozillaDir if we can ignore unix vs. native) in a number of places, as we're also referring to a few places in the Mozilla source directory. I'll leave it up to you if you want to have self.mozillaObjdir introduced in addition to simplify the places where it can be used.

Comment 10

[[e-mail] (:bhearsum)]

(In reply to comment #9)
> We could do that, but it wouldn't save us from still using self.unixMozillaDir
> (or self.mozillaDir if we can ignore unix vs. native) in a number of places, as
> we're also referring to a few places in the Mozilla source directory. I'll
> leave it up to you if you want to have self.mozillaObjdir introduced in
> addition to simplify the places where it can be used.

In that case, let's do it. It needs to be documented well to avoid confusion - but feel free to ignore that requirement and let me do it once the code is in.

Comment 11

[Robert Kaiser]

Attachment: pre version of WIP 2

Here's a second WIP that adresses the comments Ben had. Not requesting review right now as this is still untested but I now have a configuration that should be able to test the major parts of this.

Comment 12

[Robert Kaiser]

Attachment: WIP 2: address Ben's comments, still untested

oops, saw that I hadn't saved one of the last changes, but I want this up here so I can easily pull it into the test master

Comment 13

[Robert Kaiser]

Attachment: address Ben's comments, fix a few more things

OK, here's a version that has actually been tested and works fine for nightly, unit test and leak test configs together with two small subclasses and the mozilla2-derived configs I made for the testing buildbots I have running on the new SeaMonkey machines.

Most things are pretty straight-forward now, only CompareBloatLogs was slightly tricky.

Comment 14

[Robert Kaiser]

Attachment: v1.1 - fix even more steps

Sorry to attach even one more patch here today, but I found things I had missed when I finally turned on codesighs on my shiny new buildbots. The Codesighs step itself is another one that is somewhat tricky, the rest of the changes isn't that much of a deal.

With this patch, my buildbots are completing everything where CCNightlyBuildFactory is needed.

I will care about L10n and release factories in followup bugs, the stuff here is a large enough step by itself :)

Comment 15

[[e-mail] (:bhearsum)]

Comment on attachment 376654 [details] [diff] [review]
v1.1 - fix even more steps


> class CompareBloatLogs(ShellCommand):
>     warnOnWarnings = True
>     warnOnFailure = True
>     
>     def __init__(self, bloatLog, testname="", testnameprefix="",
>-                       bloatDiffPath="tools/rb/bloatdiff.pl", **kwargs):
>+                       bloatDiffPath="tools/rb/bloatdiff.pl",
>+                       mozillaDir="", **kwargs):
>         ShellCommand.__init__(self, **kwargs)
>         self.addFactoryArguments(bloatLog=bloatLog, testname=testname,
>                                  testnameprefix=testnameprefix,
>                                  bloatDiffPath=bloatDiffPath)

You need to add mozillaDir to this, otherwise this patch looks good.

I ran it through staging for quite a few hours and had absolutely no problems. I'll land this tomorrow morning.

Comment 16

[[e-mail] (:bhearsum)]

Comment on attachment 376654 [details] [diff] [review]
v1.1 - fix even more steps

changeset:   276:f84415211997

Comment 17

[Robert Kaiser]

This is in buildbotcustom and use by the new SeaMonkey buildbots, marking fixed. I will file separate bugs depending on this for the fixes to L10n repack and release classes.