Closed
Bug 1356524
Opened 8 years ago
Closed 8 years ago
Add a `mach artifact toolchain` option to get artifacts from taskcluster builds
Categories
(Firefox Build System :: General, enhancement)
Firefox Build System
General
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.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 3•8 years ago
|
||
mozreview-review |
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 4•8 years ago
|
||
mozreview-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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 7•8 years ago
|
||
mozreview-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
Comment 9•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/be3413a130d8
https://hg.mozilla.org/mozilla-central/rev/98c08360e61b
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Updated•7 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•