1162191 - Add |mach artifact| command for fetching and installing downloaded Fennec artifacts

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Nick Alexander :nalexander [he/him]]

Bug 1147384 is rather easier for Fennec for a few reasons:

* Fennec has few binaries to deal with;
* our front-end doesn't interact with the regular C++ binary building layer at all;
* Post Bug 1159371, Fennec builds with --disable-compile-environment;
* and I've built a lot of the functionality for building and packaging just the Java and JavaScript code while supporting Gradle and IntelliJ -- so we have a perfect binaries package waiting for us after remote builds already.

In Bug 1147384, catlee wanted |mach build --binaries=...|.  For mobile/android, |mach package --binaries=...| is appropriate, but I want to keep this outside of the main flow for the time being.

This ticket tracks implementing |mach package-frontend|, for use with --disable-compile-environment.

Comment 1

[Nick Alexander :nalexander [he/him]]

Attachment: MozReview Request: bz://1162191/nalexander

/r/8251 - Bug 1162191 - Add |mach package-frontend| command for packaging mobile/android with downloaded binaries. r=gps

Pull down this commit:

hg pull -r 3c0fb13b77b8460e56a31054a5f005bb3e4cdda1 https://reviewboard-hg.mozilla.org/gecko/

Comment 2

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8602254 [details]
MozReview Request: bz://1162191/nalexander

/r/8251 - Bug 1162191 - Add |mach package-frontend| command for packaging mobile/android with downloaded binaries. r=gps

Pull down this commit:

hg pull -r 3c0fb13b77b8460e56a31054a5f005bb3e4cdda1 https://reviewboard-hg.mozilla.org/gecko/

Comment 3

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/8251/#review7177

Nice.

I'm not crazy about all the subprocess calls. But it gets the job done. But I do want you to kill some of the more aggregious uses.

::: mobile/android/mach_commands.py:177
(Diff revision 1)
> +        default='http://ftp.mozilla.org/pub/mozilla.org/mobile/nightly/latest-mozilla-central-android-api-11/en-US')

Is there not a secure endpoint we can use?

::: mobile/android/mach_commands.py:240
(Diff revision 1)
> +        version = subprocess.check_output(['sh', '-c', 'head -n 1 ' + pattern]).strip()

Can't you do this with os.listdir?

::: mobile/android/mach_commands.py:208
(Diff revision 1)
> +    def _fetch_fennec_binaries(self, root, force=False):

I'd feel much better if this code were part of a proper Python module and not in a mach_commands.py file. That way, it can be reused more easily.

::: mobile/android/mach_commands.py:264
(Diff revision 1)
> +        self.run_process([
> +                'unzip',
> +                '-u', # Update files.
> +                os.path.abspath('geckolibs-{version}.aar'.format(version=version)), # Printed to the console.
> +                '-d', # Destination directory (relative to $CWD).
> +                os.path.abspath('stage'), # Printed to the console.
> +            ],
> +            ensure_exit_code=True, # Raise an exception if this command fails.
> +        )

You could implement this in Python. It's not as much work as you think. See zipfile module.

::: mobile/android/mach_commands.py:290
(Diff revision 1)
> +        self.virtualenv_manager.ensure()

You shouldn't need this line. If mach runs, the paths with manifest processing support should be in sys.path.

::: mobile/android/mach_commands.py:291
(Diff revision 1)
> +        self.run_process([
> +                self.virtualenv_manager.python_path,
> +                os.path.join(self.topsrcdir, 'python/mozbuild/mozbuild/action/process_install_manifest.py'),
> +                '--no-remove',
> +                '--no-remove-all-directory-symlinks',
> +                '--no-remove-empty-directories',
> +                os.path.join(self.distdir, 'bin'),
> +                os.path.abspath(manifest_path), # Printed to the console.
> +            ],
> +            pass_thru=True, # Not strictly necessary, but doesn't hurt.
> +            ensure_exit_code=True, # Throw on non-zero exit code.
> +        )

copier = FileCopier()
manifest.populate_registry(copier)
copier.copy(os.path.join(self.distdir, 'bin'),
            remove_unaccounted=False,
            remove_all_directory_symlinks=False,
            remove_empty_directories=False)

::: mobile/android/mach_commands.py:218
(Diff revision 1)
> +        pattern = 'fennec-*.en-US.android-arm.txt'
> +        args = [
> +            'wget',
> +            '--quiet',
> +            '--recursive',
> +            '--level=1',
> +            '--no-directories',
> +            '--no-parent',
> +            '--accept',
> +            pattern,
> +            root + '/',
> +        ]

Hmmm.

There are tools for downloading Firefox binaries. mozdownload or something.

This code should ideally be an API on that package. But I'll let it slide for now.

I'll also let the `wget` slide, as it's certainly easier to re-use their existing spidering logic than to reinvent the wheel.

Comment 4

[Nick Alexander :nalexander [he/him]]

giles: Further to our discussion, this is the ticket that tracks my package-frontend work, including downloading artifacts.

Comment 5

[(not currently active) Ted Mielczarek]

https://reviewboard.mozilla.org/r/8251/#review7427

::: mobile/android/mach_commands.py:218
(Diff revision 1)
> +        pattern = 'fennec-*.en-US.android-arm.txt'
> +        args = [
> +            'wget',
> +            '--quiet',
> +            '--recursive',
> +            '--level=1',
> +            '--no-directories',
> +            '--no-parent',
> +            '--accept',
> +            pattern,
> +            root + '/',
> +        ]

You actually might want to look into this sooner rather than later, as RelEng is actively moving uploads to S3, which means that directory indexing on ftp.mo is likely to stop working.

Comment 6

[Nick Alexander :nalexander [he/him]]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #5)
> https://reviewboard.mozilla.org/r/8251/#review7427
> 
> ::: mobile/android/mach_commands.py:218
> (Diff revision 1)
> > +        pattern = 'fennec-*.en-US.android-arm.txt'
> > +        args = [
> > +            'wget',
> > +            '--quiet',
> > +            '--recursive',
> > +            '--level=1',
> > +            '--no-directories',
> > +            '--no-parent',
> > +            '--accept',
> > +            pattern,
> > +            root + '/',
> > +        ]
> 
> You actually might want to look into this sooner rather than later, as
> RelEng is actively moving uploads to S3, which means that directory indexing
> on ftp.mo is likely to stop working.

I am aware, and am CCed on the relevant tickets, but there has been nothing implemented (or even proposed, other than "wait for TaskCluster") that addresses the existing needs of mozdownload, mozregression, or other tools like this one :(

Comment 8

[Nick Alexander :nalexander [he/him]]

Attachment: MozReview Request: Bug 1162191 - Add |mach artifact| for installing downloaded Fennec binaries. r=gps

Comment 9

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8620243 [details]
MozReview Request: Bug 1162191 - Add |mach artifact| for installing downloaded Fennec binaries. r=gps

Bug 1162191 - Add |mach artifact| for installing downloaded Fennec binaries. r=gps

Very much a first cut, but I'd like to get some Fennec early adopters testing.

This adds:

* |mach artifact install| to fetch and install Fennec binaries;
* |mach artifact last| to print details about what was last installed;
* |mach artifact {print,clear}-caches|, for debugging.

This code is exposed as a new mozbuild.artifacts Python but it's not
particularly general.  My intention was to get things out of the mach command
more than produce a general artifact fetching API.  We can leave that bike
shed to Bug 1124378.

I've been testing this with --disable-compile-environment and it works well
locally, although there's no reason a knowledgeable developer couldn't use
this in conjunction with a fully-built tree.  (I don't know when such a
situation would arise, but I know of no technical impediment.)

Others have agitated for a cross-product (browser, mobile/android, other)
solution to this problem but until now only mobile/android has invested in
making this possible.  (Of course, we've done things in Android-specific ways,
because we're interested in Gradle and IntelliJ.)  We'll happily unify when
browser and the core build system implement similar functionality.

Comment 10

[Gregory Szorc [:gps]]

Comment on attachment 8620243 [details]
MozReview Request: Bug 1162191 - Add |mach artifact| for installing downloaded Fennec binaries. r=gps

https://reviewboard.mozilla.org/r/8251/#review9875

Awesome work! There's a few minor issues. But nothing that should block this becoming a thing.

::: mobile/android/mach_commands.py:176
(Diff revision 2)
> +        pass

As of last week, we now display docstrings for mach command functions in `mach help`. You should consider writing some detailed docstrings for how this command should be used!

::: mobile/android/mach_commands.py:182
(Diff revision 2)
> +        # install_pip_package is slow, even when the package is already
> +        # installed, so we try to avoid it.
> +        try:
> +            import pylru
> +        except ImportError:
> +            self.virtualenv_manager.install_pip_package('pylru')
> +            import pylru

Bleh. I'd rather see this inlined in `install_pip_package`. But I'll let you get away with it.

::: python/mozbuild/mozbuild/artifacts.py:48
(Diff revision 2)
> +# From pip in the ambient virtualenv.

No need for this comment.

::: python/mozbuild/mozbuild/artifacts.py:67
(Diff revision 2)
> +def cachedmethod(cachefunc):
> +    '''Decorator to wrap a class or instance method with a memoizing callable that
> +    saves results in a (possibly shared) cache.
> +    '''
> +    def decorator(method):
> +        def wrapper(self, *args, **kwargs):
> +            mapping = cachefunc(self)
> +            if mapping is None:
> +                return method(self, *args, **kwargs)
> +            key = (method.__name__, args, tuple(sorted(kwargs.items())))
> +            try:
> +                value = mapping[key]
> +                return value
> +            except KeyError:
> +                pass
> +            result = method(self, *args, **kwargs)
> +            mapping[key] = result
> +            return result
> +        return functools.update_wrapper(wrapper, method)
> +    return decorator

Can you use mozbuild.util.memoize?

::: python/mozbuild/mozbuild/artifacts.py:89
(Diff revision 2)
> +class CacheManager(object):
> +    '''Maintain an LRU cache.  Provide simple persistence, including support for
> +    loading and saving the state using a "with" block.  Allow clearing the cache
> +    and printing the cache for debugging.
> +
> +    Provide simple logging.
> +    '''

This class belongs in mozbuild.util. But that likely makes pylru a required dependency unless you trap the import. I have no objection to adding pylru to the source tree under python/pylru.

::: python/mozbuild/mozbuild/artifacts.py:182
(Diff revision 2)
> +        # It appears that the Task Cluster index only takes 12-char hex hashes.

Wat wat?! This is a massive bug. I filed bug 1175655.

::: python/mozbuild/mozbuild/artifacts.py:237
(Diff revision 2)
> +        import subprocess

This should be at file level since. We only care about deferred imports to break import cycles and in mach commands.

::: python/mozbuild/mozbuild/artifacts.py:240
(Diff revision 2)
> +        # Leave it to the subprocess to handle Ctrl+C. If it terminates as
> +        # a result of Ctrl+C, proc.wait() will return a status code, and,
> +        # we get out of the loop. If it doesn't, like e.g. gdb, we continue
> +        # waiting.
> +        while status is None:
> +            try:
> +                status = proc.wait()
> +            except KeyboardInterrupt:
> +                pass

I don't believe Python will pass the signal down to child processes. I think you want to use proc.send_signal() to propogate the signal.

::: python/mozbuild/mozbuild/artifacts.py:293
(Diff revision 2)
> +                if not info.filename.endswith('.so'):

So I guess this doesn't work on Windows. Then again, my recollection is you can't do Fennec development on Windows. So I guess it isn't a problem.

::: python/mozbuild/mozbuild/artifacts.py:296
(Diff revision 2)
> +                fh = FileAvoidWrite(n, mode='r')

I thought we could use FileAvoidWrite as a context manager.

::: python/mozbuild/mozbuild/artifacts.py:315
(Diff revision 2)
> +            revset = subprocess.check_output([self._hg, 'parent', '--template', '{node}\n']).strip()

`hg parents` is deprecated. You should use `hg log -r .` instead.

::: python/mozbuild/mozbuild/artifacts.py:345
(Diff revision 2)
> +            return self.install_from_hg(source, distdir)

The Git users will complain. You should verify a .hg directory exists somewhere.

::: python/mozbuild/mozbuild/util.py:127
(Diff revision 2)
> +        self.mode = mode

Niiice. I've been wanting to make FileAvoidWrite binary safe for a while.

::: mobile/android/mach_commands.py:207
(Diff revision 2)
> +    @SubCommand('artifact', 'install',
> +        'Install a good pre-built artifact.')

I'm not crazy about the user workflow: I think utilizing artifacts should be automatic and part of the build configuration. But you have to start somewhere. This is acceptable for now.

Awe

Comment 11

[Nick Alexander :nalexander [he/him]]

https://reviewboard.mozilla.org/r/8251/#review10027

> Bleh. I'd rather see this inlined in `install_pip_package`. But I'll let you get away with it.

I agree, but this would need to handle stripping package specs (versions, etc); and it would need to do the right thing with exporting symbols outside of the function scope, which I'd need to look up.

Long term, the difference between virtualenv/non-virtualenv needs to shrink.

> I'm not crazy about the user workflow: I think utilizing artifacts should be automatic and part of the build configuration. But you have to start somewhere. This is acceptable for now.

I agree: originally, I added |mach package-frontend|.  But it's quite useful to be able to manage artifacts independently of packaging.  I would like to see |mach package| invoke |mach artifact install| automatically; but I couldn't see how to dispatch a sub-command and decided not to block.

> I don't believe Python will pass the signal down to child processes. I think you want to use proc.send_signal() to propogate the signal.

This is copy-paste from existing code: https://dxr.mozilla.org/mozilla-central/source/python/mach/mach/mixin/process.py#120.  I would use the mixin but I'm not certain that the mach modules are available.

> So I guess this doesn't work on Windows. Then again, my recollection is you can't do Fennec development on Windows. So I guess it isn't a problem.

Correct.  In future we will branch on "job type" (android-api-11, android-api-9, linux64, etc).  I expect each job type to care about different artifact types, and to do different things to install from those artifact types.

> I thought we could use FileAvoidWrite as a context manager.

You can, but you do not get the return values from close(), which I use to print status information.

> Can you use mozbuild.util.memoize?

I looked at this, but memoize works rather differently.  My situation is trickier 'cuz the cache needs to be available on the instance object, for serialization.

I'm confident we could generalize this approach, but I don't care to do it for the initial landing.

Comment 12

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8620243 [details]
MozReview Request: Bug 1162191 - Add |mach artifact| for installing downloaded Fennec binaries. r=gps

Bug 1162191 - Add |mach artifact| for installing downloaded Fennec binaries. r=gps

Very much a first cut, but I'd like to get some Fennec early adopters testing.

This adds:

* |mach artifact install| to fetch and install Fennec binaries;
* |mach artifact last| to print details about what was last installed;
* |mach artifact {print,clear}-caches|, for debugging.

This code is exposed as a new mozbuild.artifacts Python but it's not
particularly general.  My intention was to get things out of the mach command
more than produce a general artifact fetching API.  We can leave that bike
shed to Bug 1124378.

I've been testing this with --disable-compile-environment and it works well
locally, although there's no reason a knowledgeable developer couldn't use
this in conjunction with a fully-built tree.  (I don't know when such a
situation would arise, but I know of no technical impediment.)

Others have agitated for a cross-product (browser, mobile/android, other)
solution to this problem but until now only mobile/android has invested in
making this possible.  (Of course, we've done things in Android-specific ways,
because we're interested in Gradle and IntelliJ.)  We'll happily unify when
browser and the core build system implement similar functionality.

Comment 13

[Nick Alexander :nalexander [he/him]]

Attachment: MozReview Request: Bug 1162191 - Review comment: Restrict to hg source checkouts. r?gps

Bug 1162191 - Review comment: Restrict to hg source checkouts. r?gps

An expedient way to do this is add a restriction on the mach command
itself.  This required making each subcommand inherit the conditions
of its parent command.  I chose this approach because the assumption
that hg is present runs deep in the code: the main model object
accepts the path to hg as a parameter, for example.  It is not
particularly hard to extract an hg revision from a git repository, but
that will require some re-factoring that I don't intend to do for this
initial landing.

Comment 14

[Nick Alexander :nalexander [he/him]]

Attachment: MozReview Request: Bug 1162191 - Address small review comments. r?gps

Bug 1162191 - Address small review comments. r?gps

Comment 15

[Nick Alexander :nalexander [he/him]]

gps: I no longer see my responses to some of your comments on RB, but perhaps I've just misplaced them.  I addressed the large point (about requiring .hg) with a sledge-hammer.  I addressed some small points in a further commit.  But some of the bigger things aren't easy to arrange, and I don't want to spend much time honing a thing that I expect will change /a lot/ when we see it in the wild.

Comment 16

[Mike Hommey [:glandium]]

> I looked at this, but memoize works rather differently.  My situation is trickier 'cuz the cache needs to be available on the instance object, for serialization.

memoize's cache *is* in the instance object.

Comment 17

[Mike Hommey [:glandium]]

Some nitpicking:
- artifacts.py doesn't belong in mozbuild
- ideally it should share code with whatever mozregression is using.

Comment 18

[Mike Hommey [:glandium]]

> I agree: originally, I added |mach package-frontend|.  But it's quite useful to be able to manage artifacts independently of packaging.  I would like to see |mach package| invoke |mach artifact install| automatically; but I couldn't see how to dispatch a sub-command and decided not to block.

mach package is way to late for that. If this is meant to be automatic, it should happen during the build or during configure.

Comment 19

[Nick Alexander :nalexander [he/him]]

(In reply to Mike Hommey [:glandium] from comment #17)
> Some nitpicking:
> - artifacts.py doesn't belong in mozbuild

I don't care where it goes -- name your place :)

> - ideally it should share code with whatever mozregression is using.

I agree, but mozregression is doing about a million things that we don't need, and hasn't moved to use Task Cluster's index yet.  Perfect is the enemy of landed, etc.

Comment 20

[Nick Alexander :nalexander [he/him]]

(In reply to Mike Hommey [:glandium] from comment #18)
> > I agree: originally, I added |mach package-frontend|.  But it's quite useful to be able to manage artifacts independently of packaging.  I would like to see |mach package| invoke |mach artifact install| automatically; but I couldn't see how to dispatch a sub-command and decided not to block.
> 
> mach package is way to late for that. If this is meant to be automatic, it
> should happen during the build or during configure.

I don't see why.  This caches heavily, so it's cheap to run if nothing needs to be done; and for most packages, nothing needs to be done.  The later in the process this runs, the more likely it is to reflect the state of your tree.  In particular, lots of pulls from upstream won't trigger a configure at all, but will want new binaries.  (And the same is true checking out different revisions locally.)  I've been using variants of this approach since January and I have never once thought this would be better at configure time.

Comment 21

[Mike Hommey [:glandium]]

(In reply to Nick Alexander :nalexander from comment #19)
> (In reply to Mike Hommey [:glandium] from comment #17)
> > Some nitpicking:
> > - artifacts.py doesn't belong in mozbuild
> 
> I don't care where it goes -- name your place :)

If you ask me, it should be somewhere in mozbase. But mozbase people might disagree.

> > - ideally it should share code with whatever mozregression is using.
> 
> I agree, but mozregression is doing about a million things that we don't
> need, and hasn't moved to use Task Cluster's index yet.  Perfect is the
> enemy of landed, etc.

Please change the last paragraph of the corresponding commit message.

Comment 22

[Mike Hommey [:glandium]]

(In reply to Nick Alexander :nalexander from comment #20)
> (In reply to Mike Hommey [:glandium] from comment #18)
> > > I agree: originally, I added |mach package-frontend|.  But it's quite useful to be able to manage artifacts independently of packaging.  I would like to see |mach package| invoke |mach artifact install| automatically; but I couldn't see how to dispatch a sub-command and decided not to block.
> > 
> > mach package is way to late for that. If this is meant to be automatic, it
> > should happen during the build or during configure.
> 
> I don't see why.

Because mach package is something that needs to die, and because my patch queue is going to use artifacts-like things during the build.

Comment 23

[Richard Newman [:rnewman]]

Mike: it sounds like you're opposed to Nick's approach. Can you propose an expedient alternative to this, if so?

The mobile frontend team and our contributors, particularly those outside the western high-speed world, would see *enormous* benefits from Nick's work here, and Nick and others have been using it with great success for months.

We have a working patch that yields immediate 20x speedups for 95% of our developers. Is there a good short-term alternative?

Comment 24

[Mike Hommey [:glandium]]

(In reply to Richard Newman [:rnewman] from comment #23)
> Mike: it sounds like you're opposed to Nick's approach.

I'm "opposing" something that is not even implemented in this patch queue.

Comment 25

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8620243 [details]
MozReview Request: Bug 1162191 - Add |mach artifact| for installing downloaded Fennec binaries. r=gps

Bug 1162191 - Add |mach artifact| for installing downloaded Fennec binaries. r=gps

Very much a first cut, but I'd like to get some Fennec early adopters testing.

This adds:

* |mach artifact install| to fetch and install Fennec binaries;
* |mach artifact last| to print details about what was last installed;
* |mach artifact {print,clear}-caches|, for debugging.

This code is exposed as a new mozbuild.artifacts Python but it's not
particularly general.  My intention was to get things out of the mach command
more than produce a general artifact fetching API.  We can leave that bike
shed to Bug 1124378.

I've been testing this with --disable-compile-environment and it works well
locally, although there's no reason a knowledgeable developer couldn't use
this in conjunction with a fully-built tree.  (I don't know when such a
situation would arise, but I know of no technical impediment.)

Others have agitated for a cross-product (browser, mobile/android, other)
solution to this problem but until now only mobile/android has invested in
making this possible.  (Of course, we've done things in Android-specific ways,
because we're interested in Gradle and IntelliJ.)  We'll happily unify when
browser and the core build system implement similar functionality.

Comment 26

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8624022 [details]
MozReview Request: Bug 1162191 - Review comment: Restrict to hg source checkouts. r?gps

Bug 1162191 - Review comment: Restrict to hg source checkouts. r?gps

An expedient way to do this is add a restriction on the mach command
itself.  This required making each subcommand inherit the conditions
of its parent command.  I chose this approach because the assumption
that hg is present runs deep in the code: the main model object
accepts the path to hg as a parameter, for example.  It is not
particularly hard to extract an hg revision from a git repository, but
that will require some re-factoring that I don't intend to do for this
initial landing.

Comment 27

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8624023 [details]
MozReview Request: Bug 1162191 - Address small review comments. r?gps

Bug 1162191 - Address small review comments. r?gps

Comment 28

[Nick Alexander :nalexander [he/him]]

Attachment: MozReview Request: Bug 1162191 - Add per-job downloads. r?gps

Bug 1162191 - Add per-job downloads. r?gps

This is interesting only because android-api-11 has a valuable
non-package named artifact: geckolibs-*.aar is half the size of the
fennec APK.  I expect additional artifact types to evolve over time.

Comment 29

[Nick Alexander :nalexander [he/him]]

Gijs: Rebased and added per-job artifact regexps.  Try |mach artifact install --job macosx64| or similar.

Comment 30

[Gregory Szorc [:gps]]

Comment on attachment 8624022 [details]
MozReview Request: Bug 1162191 - Review comment: Restrict to hg source checkouts. r?gps

https://reviewboard.mozilla.org/r/11645/#review10439

::: python/mozbuild/mozbuild/base.py:770
(Diff revision 2)
> +        if hasattr(cls, 'substs'):
> +            top_srcdir = cls.substs.get('top_srcdir')
> +            return top_srcdir and os.path.isdir(os.path.join(top_srcdir, '.hg'))
> +        return False

I /think/ cls.topsrcdir exists. But you may want to double check (I can't remember what cls is here). But this code is fine, since this convention is already used in this file.

Comment 31

[Gregory Szorc [:gps]]

Comment on attachment 8624023 [details]
MozReview Request: Bug 1162191 - Address small review comments. r?gps

https://reviewboard.mozilla.org/r/11647/#review10441

::: mobile/android/mach_commands.py:180
(Diff revision 2)
> +        r'''Download, cache, and install pre-built binary artifacts to build Fennec.

You don't need r prefix. Also, please indent subsequent lines: that's how docstrings work.

Comment 32

[Gregory Szorc [:gps]]

Comment on attachment 8620243 [details]
MozReview Request: Bug 1162191 - Add |mach artifact| for installing downloaded Fennec binaries. r=gps

https://reviewboard.mozilla.org/r/8251/#review10443

I can't guarantee this code is perfect. But when it comes to groundbreaking new features like this, perfect is most definitely the enemy of done. Let's land this and get it in the hands of more developers and fix issues as they are reported.

::: mobile/android/mach_commands.py:189
(Diff revision 4)
> +            self.virtualenv_manager.install_pip_package('pylru')

One thing to watch out for when installing pip packages is new, incompatible versions may be installed in the future. It's a best practice to pin versions. e.g. install_pip_package('pylru==1.0')

::: mobile/android/mach_commands.py:213
(Diff revision 4)
> +        default='fx-team')

Why are you using fx-team and not central? A benefit of using central is that it is typically more stable than fx-team. Although, it does lag a little.

We can always change this as a follow-up.

::: mobile/android/mach_commands.py:216
(Diff revision 4)
> +        default='android-api-11')

Please add an in-line TODO to select the job based on the current build configuration, since we may leverage this command for non-Fennec builds some day.

::: python/mozbuild/mozbuild/artifacts.py:37
(Diff revision 4)
> +from __future__ import print_function, unicode_literals

I just mass rewrote all of mozbuild to use absolute_import (or at least the patches got r+d). Please add absolute_import.

::: python/mozbuild/mozbuild/artifacts.py:108
(Diff revision 4)
> +            items = pickle.load(open(self._cache_filename, 'rb'))

Pretty sure this will raise something not an IOError when the cache file becomes corrupted. Might want to catch whatever pickle.load can raise and also treat it as an empty cache.

::: python/mozbuild/mozbuild/artifacts.py:237
(Diff revision 4)
> +        import subprocess

Do this at file level.

Comment 33

[Gregory Szorc [:gps]]

Comment on attachment 8625992 [details]
MozReview Request: Bug 1162191 - Add per-job downloads. r?gps

https://reviewboard.mozilla.org/r/11973/#review10449

::: python/mozbuild/mozbuild/artifacts.py:192
(Diff revision 1)
> +            re = JOB_DETAILS[job]['re']

Please rename this variable so it doesn't shadow the "re" module.

Comment 34

[:Gijs (he/him)]

(In reply to Nick Alexander :nalexander from comment #29)
> Gijs: Rebased and added per-job artifact regexps.  Try |mach artifact
> install --job macosx64| or similar.

I'll look at this tomorrow, sorry for delays, will keep needinfo until then.

Comment 35

[Nick Alexander :nalexander [he/him]]

https://reviewboard.mozilla.org/r/8251/#review10455

> One thing to watch out for when installing pip packages is new, incompatible versions may be installed in the future. It's a best practice to pin versions. e.g. install_pip_package('pylru==1.0')

Done!

> Why are you using fx-team and not central? A benefit of using central is that it is typically more stable than fx-team. Although, it does lag a little.
> 
> We can always change this as a follow-up.

Only because Android front-end team builds against fx-team.  It's true that central is more stable, but I am more comfortable with fx-team for now.  If we had some sort of "binary interface" measure in automation we could do better...

> Pretty sure this will raise something not an IOError when the cache file becomes corrupted. Might want to catch whatever pickle.load can raise and also treat it as an empty cache.

You are correct.  Unfortunately pickle.load raises many exceptions, so we catch-and-ignore everything (and drop the cache).

Comment 36

[Mike Hommey [:glandium]]

> I just mass rewrote all of mozbuild to use absolute_import (or at least the patches got r+d). Please add absolute_import.

We should probably have a make check test that double checks that we don't forget to add one.

Comment 37

[Nick Alexander :nalexander [he/him]]

https://hg.mozilla.org/integration/fx-team/rev/516232663a0b

Comment 38

[Nick Alexander :nalexander [he/him]]

Lots of follow-up bugs to file tomorrow.

Comment 39

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/516232663a0b

Comment 40

[:Gijs (he/him)]

(In reply to Nick Alexander :nalexander from comment #29)
> Gijs: Rebased and added per-job artifact regexps.  Try |mach artifact
> install --job macosx64| or similar.

Gah, sorry for my being so late to respond here. I've been distracted by far too many different things.

Trying this results in:

gkruitbosch-16516:fx-team gkruitbosch$ ./mach clobber && ./mach artifact install --job macosx64
usage: mach [global arguments] artifact install [command arguments]
mach: error: argument --job: invalid choice: 'macosx64' (choose from u'android-api-11')

Should I just file a new bug for this at this point?

Comment 41

[Nick Alexander :nalexander [he/him]]

(In reply to :Gijs Kruitbosch from comment #40)
> (In reply to Nick Alexander :nalexander from comment #29)
> > Gijs: Rebased and added per-job artifact regexps.  Try |mach artifact
> > install --job macosx64| or similar.
> 
> Gah, sorry for my being so late to respond here. I've been distracted by far
> too many different things.

No trouble -- we're all busy.

> Trying this results in:
> 
> gkruitbosch-16516:fx-team gkruitbosch$ ./mach clobber && ./mach artifact
> install --job macosx64
> usage: mach [global arguments] artifact install [command arguments]
> mach: error: argument --job: invalid choice: 'macosx64' (choose from
> u'android-api-11')
> 
> Should I just file a new bug for this at this point?

Yes, please do.  The extension point is there but we don't yet handle artifacts from different builds with the same name smoothly.