1356524 - Add a `mach artifact toolchain` option to get artifacts from taskcluster builds

Status: RESOLVED FIXED

(Firefox Build System :: General, enhancement)

Comment 1

[Mike Hommey [:glandium]]

Attachment: Bug 1356524 - Only add Authorization header when sending requests to the tooltool url.

We're going to potentially use the same download manager for tooltool
and taskcluster artifacts, and we don't want to send the tooltool
authentication header to the taskcluster requests.

Review commit: https://reviewboard.mozilla.org/r/130234/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/130234/

Comment 2

[Mike Hommey [:glandium]]

Attachment: Bug 1356524 - Add a `mach artifact toolchain` option to get artifacts from taskcluster builds.

Now that we have automated build jobs that produce toolchains, we want
to avoid the burden of uploading them to tooltool and then update the
tooltool manifests. But we don't have build jobs for all the possible
toolchains, so we allow `mach artifact toolchain` to get a mix of
tooltool and taskcluster artifacts.

For taskcluster artifacts, we can give a list of job names (conveniently
automatically normalized to begin with 'toolchain-' and end with '/opt')
for which the artifacts will be downloaded, in place of any tooltool
package with the same name (if a tooltool manifest is given).

The taskcluster artifacts that we download are the ones matching the
contents of the tree the command is run with, per the resources declared
for the corresponding toolchain build job (in
taskcluster/ci/toolchain*.yml)

So for example, a linux64 build could call the following command:
  mach artifact toolchain --tooltool-manifest \
    browser/config/tooltool-manifests/linux64/releng.manifest \
    --from-build linux64-gcc

and get the right gcc corresponding to the build-gcc script in tree,
along with the other non-gcc files from the tooltool manifest.

Things are however planned to be even more convenient, but some commands
can already benefit from this form (even without a tooltool manifest).
See e.g. bug 1328454.

Review commit: https://reviewboard.mozilla.org/r/130236/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/130236/

Comment 3

[Gregory Szorc [:gps]]

Comment on attachment 8858254 [details]
Bug 1356524 - Only add Authorization header when sending requests to the tooltool url.

https://reviewboard.mozilla.org/r/130234/#review133030

Good catch.

Comment 4

[Gregory Szorc [:gps]]

Comment on attachment 8858255 [details]
Bug 1356524 - Add a `mach artifact toolchain` option to get artifacts from taskcluster builds.

https://reviewboard.mozilla.org/r/130236/#review133038

This is mostly good. But the bareword `except`s are landing blockers.

::: python/mozbuild/mozbuild/mach_commands.py:1561
(Diff revision 1)
>      @CommandArgument('--retry', type=int, default=0,
>          help='Number of times to retry failed downloads')
>      @CommandArgument('files', nargs='*',
>          help='Only download the given file names (you may use file name stems)')
>      def artifact_toolchain(self, verbose=False, cache_dir=None,
> -                          skip_cache=False, tooltool_manifest=None,
> +                          skip_cache=False, from_build=(),

FWIW I'm not a fan of an empty tuples as default arguments. However, since tuples aren't mutable, it doesn't have the "mutable default argument values" problem that lists and dicts to, so it is technically OK. The similarity is enough to make me do a double take when reading code, which is why I'm not a fan.

::: python/mozbuild/mozbuild/mach_commands.py:1665
(Diff revision 1)
> +            def tasks(kind):
> +                kind_path = mozpath.join('taskcluster', 'ci', kind)
> +                with open(mozpath.join(self.topsrcdir, kind_path, 'kind.yml')) as f:
> +                    config = yaml.load(f)
> +                    tasks = Kind(kind, kind_path, config).load_tasks(params, {})
> +                    return {
> +                        task.task['metadata']['name']: task
> +                        for task in tasks
> +                    }

This is a layering violation.

Please add and use a function in the taskgraph package. You can do this as a follow-up if you want. But at least add an inline TODO if this lands.

::: python/mozbuild/mozbuild/mach_commands.py:1703
(Diff revision 1)
> +
> +                for artifact in list_artifacts(task_id):
> +                    name = artifact['name']
> +                    if not name.startswith('public/'):
> +                        continue
> +                    name = name[7:]

Use `len('public/')` instead of `7`.

::: python/mozbuild/mozbuild/mach_commands.py:1743
(Diff revision 1)
> -                # (https://github.com/mozilla/build-tooltool/issues/38)
> -                curdir = os.getcwd()
> -                os.chdir(os.path.dirname(downloaded))
>                  try:
> -                    valid = validate_record.validate()
> -                finally:
> +                    valid = record.validate()
> +                except:

Never use bareword `except` because it catches `KeyboardInterrupt` and `SystemExit` and can prevent program termination. Use `except Except` instead.

::: python/mozbuild/mozbuild/mach_commands.py:1770
(Diff revision 1)
>              # (https://github.com/mozilla/build-tooltool/issues/38), so we
>              # need to copy it, even though we remove it later. Use hard links
>              # when possible.
>              try:
> -                os.link(downloaded, local)
> +                os.link(record.filename, local)
>              except:

Don't use bareword `except`.

Comment 5

[Mike Hommey [:glandium]]

Comment on attachment 8858254 [details]
Bug 1356524 - Only add Authorization header when sending requests to the tooltool url.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/130234/diff/1-2/

Comment 6

[Mike Hommey [:glandium]]

Comment on attachment 8858255 [details]
Bug 1356524 - Add a `mach artifact toolchain` option to get artifacts from taskcluster builds.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/130236/diff/1-2/

Comment 7

[Gregory Szorc [:gps]]

Comment on attachment 8858255 [details]
Bug 1356524 - Add a `mach artifact toolchain` option to get artifacts from taskcluster builds.

https://reviewboard.mozilla.org/r/130236/#review133084

Thank you for the follow-ups.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/be3413a130d8
Only add Authorization header when sending requests to the tooltool url. r=gps
https://hg.mozilla.org/integration/autoland/rev/98c08360e61b
Add a `mach artifact toolchain` option to get artifacts from taskcluster builds. r=gps

Comment 9

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/be3413a130d8
https://hg.mozilla.org/mozilla-central/rev/98c08360e61b