Closed Bug 1355731 Opened 7 years ago Closed 7 years ago

Add a mach command wrapping tooltool

Categories

(Firefox Build System :: General, enhancement)

Product:
Firefox Build System
Component:
General
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(firefox55 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: glandium, Assigned: glandium)

References

Details

Attachments

(3 files, 9 obsolete files)

Bug 1355731 - Move tooltool to mozbuild.action.
59 bytes, text/x-review-board-request
gps
: review+
Details
Bug 1355731 - For artifacts cache, use the file name from the url if it looks like a hash digest.
59 bytes, text/x-review-board-request
gps
: review+
Details
Bug 1355731 - Add a mach command wrapping tooltool.
59 bytes, text/x-review-board-request
gps
: review+
Details 
      No description provided.
Comment on attachment 8857379 [details]
Bug 1355731 - Move tooltool to mozbuild.action.

https://reviewboard.mozilla.org/r/129366/#review132118
Attachment #8857379 - Flags: review+
Comment on attachment 8857380 [details]
Bug 1355731 - For artifacts cache, use the file name from the url if it looks like a hash digest.

https://reviewboard.mozilla.org/r/129368/#review132120

Nice usability win.
Attachment #8857380 - Flags: review+
Comment on attachment 8857381 [details]
Bug 1355731 - Add a mach command wrapping tooltool.

https://reviewboard.mozilla.org/r/129370/#review132122

I like where you're going. Lots of minor issues and questions on this patch though. I think most of them can be answers with "because tooltool :("

::: python/mozbuild/mozbuild/mach_commands.py:1579
(Diff revision 1)
> +    def artifact_toolchain(self, verbose=False, cache_dir=None,
> +                          skip_cache=False, tooltool_manifest=None,
> +                          authentication_file=None, tooltool_url=None,
> +                          no_unpack=False, retry=None, toolchains=()):

Can you please add a docstring here to describe the sub-command? Please also include example values for "toolchains" since their values aren't obvious.

::: python/mozbuild/mozbuild/mach_commands.py:1608
(Diff revision 1)
> +        try:
> +            os.makedirs(cache_dir)
> +        except OSError as e:
> +            if e.errno != errno.EEXIST:
> +                raise

Use `mozbuild.util.mkdir()`.

::: python/mozbuild/mozbuild/mach_commands.py:1630
(Diff revision 1)
> +
> +        manifest = open_manifest(os.path.join(self.topsrcdir,
> +                                              tooltool_manifest))
> +        downloaded_files = {}
> +
> +        for record in manifest.file_records:

This is scope bloat for this bug, but it would be nice if we could do parallel download here, since I imagine any single thread will be tied up in network or filesystem I/O a lot of the time.

::: python/mozbuild/mozbuild/mach_commands.py:1641
(Diff revision 1)
> +            self.log(logging.INFO, 'artifact', {'name': record.filename},
> +                     'Downloading {name}')
> +            url = '{}/{}/{}'.format(tooltool_url, record.algorithm,
> +                                    record.digest)
> +            valid = False
> +            for _ in redo.retrier(attempts=retry+1, sleeptime=60):

60s seems a bit high. How did you arrive at this number?

::: python/mozbuild/mozbuild/mach_commands.py:1644
(Diff revision 1)
> +                except requests.exceptions.HTTPError:
> +                    continue

Does this log anywhere?

::: python/mozbuild/mozbuild/mach_commands.py:1649
(Diff revision 1)
> +                except requests.exceptions.HTTPError:
> +                    continue
> +                validate_record = FileRecord(
> +                    os.path.basename(downloaded), record.size, record.digest,
> +                    record.algorithm)
> +                # FileRecord.validate needs the file in the current directory

I can haz sad.

Could we get an issue on file upstream so we have a TODO to clean up this mess?

::: python/mozbuild/mozbuild/mach_commands.py:1673
(Diff revision 1)
> +            shutil.copy(downloaded, local)
> +            if record.unpack and not no_unpack:
> +                unpack_file(local, record.setup)
> +                os.unlink(local)

Why do we make a file copy only to delete it afterwards? Can't we operate on the original file?

Since some of the toolchain files can be dozens of megabytes, I worry about perf implications here.

Also, why do we have to touch disk at all? It could be nice to buffer the file in memory and only write the unpacked contents to disk. But I suppose that would require an invasive refactor of tooltool's API.
Attachment #8857381 - Flags: review-
(In reply to Gregory Szorc [:gps] from comment #6)
> ::: python/mozbuild/mozbuild/mach_commands.py:1630
> (Diff revision 1)
> > +
> > +        manifest = open_manifest(os.path.join(self.topsrcdir,
> > +                                              tooltool_manifest))
> > +        downloaded_files = {}
> > +
> > +        for record in manifest.file_records:
> 
> This is scope bloat for this bug, but it would be nice if we could do
> parallel download here, since I imagine any single thread will be tied up in
> network or filesystem I/O a lot of the time.

That's true of artifact downloads too, and that uses the same code. 2 birds with 1 stone, but yeah, scope bloat.

> ::: python/mozbuild/mozbuild/mach_commands.py:1641
> (Diff revision 1)
> > +            self.log(logging.INFO, 'artifact', {'name': record.filename},
> > +                     'Downloading {name}')
> > +            url = '{}/{}/{}'.format(tooltool_url, record.algorithm,
> > +                                    record.digest)
> > +            valid = False
> > +            for _ in redo.retrier(attempts=retry+1, sleeptime=60):
> 
> 60s seems a bit high. How did you arrive at this number?

That's what retry.py (used by tooltool_wrapper.sh) does.

> ::: python/mozbuild/mozbuild/mach_commands.py:1644
> (Diff revision 1)
> > +                except requests.exceptions.HTTPError:
> > +                    continue
> 
> Does this log anywhere?

Mmmm probably not.
 
> ::: python/mozbuild/mozbuild/mach_commands.py:1649
> (Diff revision 1)
> > +                except requests.exceptions.HTTPError:
> > +                    continue
> > +                validate_record = FileRecord(
> > +                    os.path.basename(downloaded), record.size, record.digest,
> > +                    record.algorithm)
> > +                # FileRecord.validate needs the file in the current directory
> 
> I can haz sad.
> 
> Could we get an issue on file upstream so we have a TODO to clean up this
> mess?
> 
> ::: python/mozbuild/mozbuild/mach_commands.py:1673
> (Diff revision 1)
> > +            shutil.copy(downloaded, local)
> > +            if record.unpack and not no_unpack:
> > +                unpack_file(local, record.setup)
> > +                os.unlink(local)
> 
> Why do we make a file copy only to delete it afterwards? Can't we operate on
> the original file?

Not without changing unpack_file.
 
> Since some of the toolchain files can be dozens of megabytes, I worry about
> perf implications here.

Nothing worse than what tooltool already does (hint: it does the same)

> Also, why do we have to touch disk at all? It could be nice to buffer the
> file in memory and only write the unpacked contents to disk. But I suppose
> that would require an invasive refactor of tooltool's API.

unpack_file runs commands to unpack. Which need actual files.

At this point, I'd rather focus on something that gets the job done rather than something that's perfect.
Also note I'm not expecting this to be run by people directly.
I agree that perfect is the enemy here. It is pretty obvious from reading this code that tooltool needs some performance and API love upstream. Given the implications for end-to-end times, it might be worth our time to just do it for them.
Well, the end game is less tooltool and more taskcluster artifacts, so... let's see later.
Comment on attachment 8857381 [details]
Bug 1355731 - Add a mach command wrapping tooltool.

https://reviewboard.mozilla.org/r/129370/#review132122

> Use `mozbuild.util.mkdir()`.

I'm not going to address this here, but rather in a followup, to address it in _make_artifacts as well.
Comment on attachment 8857381 [details]
Bug 1355731 - Add a mach command wrapping tooltool.

https://reviewboard.mozilla.org/r/129370/#review132122

> Can you please add a docstring here to describe the sub-command? Please also include example values for "toolchains" since their values aren't obvious.

AIUI, fixing bug 1306501 would make such a docstring appear in mach command subcommand --help output, with the individual help for arguments. I'm not sure there's much more to put than what's already in there (except I changed the toolchains option and rephrased its help)
Depends on: 1356123
Blocks: 1356140
Blocks: 1356142
Comment on attachment 8857381 [details]
Bug 1355731 - Add a mach command wrapping tooltool.

Forwarding review for this patch to gps since he already has eyes on it. Attempting to do the same in mozreview just cleared the flag, unfortunately.
Attachment #8857381 - Flags: review?(gps)
Comment on attachment 8857838 [details]
Bug 1355731 - Use new tooltool wrapper for valgrind builds.

https://reviewboard.mozilla.org/r/129858/#review132734
Attachment #8857838 - Flags: review?(cmanchester) → review+
Comment on attachment 8857839 [details]
Bug 1355731 - Use new tooltool wrapper for Windows Spidermonkey builds.

https://reviewboard.mozilla.org/r/129860/#review132738
Attachment #8857839 - Flags: review?(cmanchester) → review+
Comment on attachment 8857840 [details]
Bug 1355731 - Use new tooltool wrapper in mozharness.

https://reviewboard.mozilla.org/r/129862/#review132740

::: testing/mozharness/mozharness/mozilla/tooltool.py:85
(Diff revision 1)
>          auth_file = self._get_auth_file()
>          if auth_file and os.path.exists(auth_file):
>              cmd.extend(['--authentication-file', auth_file])
>  
> +        if self.topsrcdir:
> +            cmd.extend(['--tooltool-manifest'])

Should be cmd.extend(['--tooltool-manifest', manifest]) ; fixed locally.
Depends on: 1356476
Depends on: 1356520
Blocks: 1356524
Depends on: 1356541
Comment on attachment 8857381 [details]
Bug 1355731 - Add a mach command wrapping tooltool.

https://reviewboard.mozilla.org/r/129370/#review133046

I kinda wish this logic lived in a reusable Python module and not a mach command. But I don't want to you lots of rebase problems. We can always do that in a follow-up if we need to.
Attachment #8857381 - Flags: review?(gps) → review+
Blocks: 1356683
Attachment #8857838 - Attachment is obsolete: true
Attachment #8857839 - Attachment is obsolete: true
Attachment #8857840 - Attachment is obsolete: true
Attachment #8857840 - Flags: review?(cmanchester)
Attachment #8857841 - Attachment is obsolete: true
Attachment #8857841 - Flags: review?(cmanchester)
Attachment #8857842 - Attachment is obsolete: true
Attachment #8857842 - Flags: review?(cmanchester)
Attachment #8857843 - Attachment is obsolete: true
Attachment #8857843 - Flags: review?(cmanchester)
Attachment #8857844 - Attachment is obsolete: true
Attachment #8857844 - Flags: review?(cmanchester)
Attachment #8857845 - Attachment is obsolete: true
Attachment #8857845 - Flags: review?(cmanchester)
Attachment #8857846 - Attachment is obsolete: true
Attachment #8857846 - Flags: review?(cmanchester)
Pushed by mh@glandium.org:
https://hg.mozilla.org/integration/autoland/rev/1d029c427c1a
Move tooltool to mozbuild.action. r=gps
https://hg.mozilla.org/integration/autoland/rev/8c376c767054
For artifacts cache, use the file name from the url if it looks like a hash digest. r=gps
https://hg.mozilla.org/integration/autoland/rev/c61332f30eaa
Add a mach command wrapping tooltool. r=gps
Blocks: 1356976
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: