506989 - l10n repackages for Maemo need to happen within the scratchbox

Status: RESOLVED FIXED

(Release Engineering :: General, defect, P1)

Description

[Axel Hecht [:Pike]]

When we add deb packaging, we probably need to have the repackaging run inside the sandbox so that the deb pkg stuff picks up the right platform.

The deb packages will be created by the installers-% foo target transparently.

If there's no performance penalty involved, we should do this asap, probably for both the configure and the make installers-% foo step.

Then we only need to figure out the upload stuff afterwards.

Comment 1

[Aki Sasaki (not active)]

Attachment: use /scratchbox/moz_scratchbox commands for maemo repack autoconf/make

- moved the BaseRepackFactory autoconf/make steps into a method so I could override it in MaemoNightlyRepackFactory
- fixed some extraneous whitespace
- overrode various methods from both BaseRepackFactory and MobileNightlyRepackFactory in MaemoNightlyRepackFactory
- tested on s-m2

It's creating and uploading tar.bz2 files to staging-stage, so I think it's good.  It seems like they're all dying in compare locales, but that might be expected.

The tricks:

- during make wget-en-US, specify -k option to moz_scratchbox to preserve env
- hg commands need to happen in the same directory, but not using scratchbox since we don't appear to have an updated hg there?

Comment 2

[Aki Sasaki (not active)]

Comment on attachment 391283 [details] [diff] [review]
use /scratchbox/moz_scratchbox commands for maemo repack autoconf/make

Also, when this lands, we need to clobber /scratchbox/users/cltbld/home/cltbld/l10n, and/or the config.cache, since we were previously running configure without calling scratchbox.

Comment 3

[Aki Sasaki (not active)]

sigh, i tried nuking dist/ to fix bug 502759 and now i'm getting

eval eval /builds/scratchbox/sbin/chroot-uid /builds/scratchbox/users/cltbld /scratchbox/tools/bin/env - $SBENV /scratchbox/tools/bin/bash -l /scratchbox/tools/bin/exec.sh l10n/mozilla-central/mobile/locales make ident
gecko_revision Key SourceStamp not found.
make: *** [ident] Error 1

Comment 4

[Axel Hecht [:Pike]]

After you nuked dist, you need to

make -C config
  to get nsinstall
make wget-en-US
make unpack

Regarding the actual patch, I'm not that fond of it, tbh. It's hard forking code for fennec which I think is really just some rewriting of where the path goes. I'd prefer to have a method

  def sandbox(self, **kwargs):
      '''Mangle addStep kwargs to work in a scratchbox.
      '''

and make the workdir param here part of the command stuff. In the BaseRepackFactory that would just return kwargs, so the factory would call it like

         self.addStep(SetProperty,
                      **self.sandbox(
                          command=['hg', 'ident', '-i'],
                          haltOnFailure=True,
                          property='l10n_revision',
                          workdir=WithProperties('build/' + self.l10nRepoPath +
                                                 '/%(locale)s')
                          )
                      )

using a non-sandboxed command here, probably. I hope the concept is clear?

In my local testing, I didn't need the extra configure argument anymore, as we're now running it in the right environment, btw.

Comment 5

[Aki Sasaki (not active)]

Brad -- is this still necessary, or should I be able to just run |make package| (or whatever command) in scratchbox?

Comment 6

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to comment #5)
> Brad -- is this still necessary, or should I be able to just run |make package|
> (or whatever command) in scratchbox?

Sorry, I don't understand what the original problem is

Comment 7

[Axel Hecht [:Pike]]

... and I recall that we had an irc chat about using a function to scratchbox some commands, but I don't recall which argument you had why that wouldn't work.

Comment 8

[Aki Sasaki (not active)]

Unless I'm missing something, looks like I'm able to repackage the debs without this.  We can reopen if there's something else/new that needs this.

Comment 10

[Armen [:armenzg]]

Attachment: run commands within scratchbox for Maemo builds

Aki and Axel could you please review this almost as a formal review?
If you can't almost review can you have a quick look at it? I need to know if this approach is way off.

It works start to end but I need to test it against release runs, winmo runs and desktop builds.

There is also a buildbot-configs patch but it is minor. I will post it when formal reviews are requested.

Comment 11

[Armen [:armenzg]]

Find the French locale in:
http://staging-stage.build.mozilla.org/pub/mozilla.org/mobile/nightly/latest-mobile-1.9.2-l10n/?C=M;O=D

Comment 12

[Axel Hecht [:Pike]]

Comment on attachment 428835 [details] [diff] [review]
run commands within scratchbox for Maemo builds

r- on a few things.

I'm not that fond of the interdependency between processCommand and self.scratchboxMode.

I'd rather do a decorator-style redefinition of the function if scratchboxPath is set in __init__ below.

The processCommand method has two positional params, but you call it with keyword params, that's wrong.

I'd prefer '/'.join(('build', self.baseBuildDir, dir)) to the string placement stuff.

+            if self.scratchboxMode:
+                mergeOptions = 'LOCALE_MERGEDIR=~/build/%s/%s/merged' % \
+                               (self.baseBuildDir, self.appName)

has a ~ in it, is that any more reliable than the $PWD variable? Seems like this is just tilde vs env expanding. Or is ~ just not expanded within make? The whole line looks rather brittle and dependent on have the build slaves at one particular spot.

Comment 13

[Aki Sasaki (not active)]

I'll do further review and may have more comments later, but:

(In reply to comment #12)
> I'm not that fond of the interdependency between processCommand and
> self.scratchboxMode.

Do you mean that BaseRepackFactory.processCommand() should be

    def processCommand(self, command, dir, verbose=True):
        return command

and Armen should override that in MaemoBuildFactory?  Or are you changing your mind about comment 4 ?

Comment 14

[Aki Sasaki (not active)]

n/m, re-read and searched:
http://www.python.org/dev/peps/pep-0318/#on-the-name-decorator

Will add my comments in a bit.

Comment 15

[Aki Sasaki (not active)]

From the link in comment 14:

"For people who... have used at least one other programming language, much of Python is intuitive. Very few people will have had any experience with the decorator concept before encountering it in Python. There's just no strong preexisting meme that captures the concept."

I would argue that keeping our configs more intuitive is more important, especially since there is an acceptable way of doing things without decorators.  You can, however, specify a placeholder processCommand (like in comment 13) and override it in MaemoBuildFactory, which would let us remove the self.scratchboxMode variable entirely.

Setting the Maemo- and scratchbox- level configs and logic in MaemoBuildFactory also makes sense since it's Maemo-specific... that could help solve the mergeOptions nit as well.

I'm not terribly concerned with the other two nits.  We use %s a lot of other places in our configs, so that would be consistent.  If setting command and dir=None would appease the keyword/position argument, that's fine with me.

Overall, great + thanks for doing this, Armen!

Comment 16

[Axel Hecht [:Pike]]

Here's a code snippet that works for me locally to make my "decorator-style" more clear:

class Foo:
    def __init__(self, do=False):
        if not do:
            return
        _foo = self.foo
        def __foo():
            return _foo() + "_tweaked"
        self.foo = __foo
    def foo(self):
        return "hi"

f = Foo()
print f.foo()
g = Foo(do=True)
print g.foo()

Comment 17

[Axel Hecht [:Pike]]

... or just overload processCommand with something like

def processCommand(self, cmd):
  if self.scratchboxPath is None:
    return cmd
  ....

Comment 18

[Armen [:armenzg]]

Attachment: run commands within scratchbox for Maemo builds (v3)

I pushed def processCommand to MaemoNightlyRepackFactory and left "return command" on BaseRepackFactory.

Comment 19

[Armen [:armenzg]]

Attachment: add baseBuildDir to the factories plus few staging fixes

Comment 20

[Aki Sasaki (not active)]

Comment on attachment 429588 [details] [diff] [review]
add baseBuildDir to the factories plus few staging fixes

Hm, thanks for cleaning this up.

It would be better to clean this up by adding base_l10n_builddir in mobile_config.py to avoid duplication, but we're ok for now so I'm not going to block on that.

However, could you update the download_base_url for production mobile_config.py as well? r=me with that change.

Comment 21

[Aki Sasaki (not active)]

Comment on attachment 429586 [details] [diff] [review]
run commands within scratchbox for Maemo builds (v3)

Looks good to me. I bet Axel still wants the command and dir variables fixed in processCommand.

Axel -- what do you recommend, setting them to command='', dir='' as defaults?  Or None and assert that they're set?

Comment 22

[Axel Hecht [:Pike]]

Comment on attachment 429586 [details] [diff] [review]
run commands within scratchbox for Maemo builds (v3)

Found more as I'm looking more closely, sorry. I don't like the double-slightly-different coding of the workdirs. Reading the patch, it seems that you just need to wrap the command to be run in the scratchbox in the workdir, if that's true, we should code it up like that. Doing the processCommand on the kwargs dict to ShellCommand instead of just the command list might expose that in the right way.

Now that we're running in the scratchbox, does repack_debs still need the env vars for DEB_BUILD_ARCH and DEB_PKG_NAME?

Comment 23

[Armen [:armenzg]]

Attachment: run commands within scratchbox for Maemo builds (v4)

I am using the structure of comment 4 and I am liking since it removes the oddness of having "dir" and "workdir" plus it is a cleaner patch.

Comment 24

[Aki Sasaki (not active)]

Comment on attachment 430050 [details] [diff] [review]
run commands within scratchbox for Maemo builds (v4)

Hm, we could put command and workdir in as keyword arguments in MaemoNightlyRepackFactory.processCommand(), as you did with verbose, and remove the kwargs['workdir'] type awkwardness.

However, I don't think I care enough about that, while I care a great deal about getting Maemo repacks working again, so for me that's a non-blocking nit. r=me.

Comment 25

[Axel Hecht [:Pike]]

Comment on attachment 430050 [details] [diff] [review]
run commands within scratchbox for Maemo builds (v4)

I actually don't find kwargs['workdir'] awkward :-)

Comment 26

[Armen [:armenzg]]

Comment on attachment 429588 [details] [diff] [review]
add baseBuildDir to the factories plus few staging fixes

http://hg.mozilla.org/build/buildbot-configs/rev/7ab788cc7594

Comment 27

[Armen [:armenzg]]

Comment on attachment 430050 [details] [diff] [review]
run commands within scratchbox for Maemo builds (v4)

http://hg.mozilla.org/build/buildbotcustom/rev/4b558260afeb

Comment 28

[Armen [:armenzg]]

Attachment: missing line - 'sh -c' is not liked inside of scratchbox

http://hg.mozilla.org/build/buildbotcustom/rev/34b81b332531

Comment 29

[Armen [:armenzg]]

The jobs can go green but are currently disabled in bug 548178.

The builds are happening within scratchbox. Resolving FIXED.