Closed Bug 1356524 Opened 5 years ago Closed 5 years ago

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

Categories

(Firefox Build System :: General, enhancement)

enhancement
Not set
normal

Tracking

(firefox55 fixed)

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: glandium, Assigned: glandium)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

No description provided.
Blocks: 1328454
Blocks: 1356529
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.
Attachment #8858254 - Flags: review?(gps) → review+
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`.
Attachment #8858255 - Flags: review?(gps) → review-
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.
Attachment #8858255 - Flags: review?(gps) → review+
Pushed by mh@glandium.org:
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
https://hg.mozilla.org/mozilla-central/rev/be3413a130d8
https://hg.mozilla.org/mozilla-central/rev/98c08360e61b
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Depends on: 1356934
Depends on: 1356937
Blocks: 1369630
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.