1465836 - Artifact builds should be able to pull and upload host/bin/{mar,mbsdiff}{,.exe}

Status: RESOLVED FIXED

(Firefox Build System :: General, enhancement)

Description

[Justin Wood (:Callek)]

I recently created tasks that can run on try (L10n) that are able to test the L10n repack process without requiring nightlies, in doing so those tasks depend on the 'B' task's host/bin/mar and mbsdiff.

Unfortunately we have this hybrid task on try when --artifact is specified that converts the 'B' to an artifact build, and thus does not download (or upload) mar/mbsdiff host binaries.

I'd like us to download and re-upload as task artifacts those files when run in automation (locally doesn't make sense, since at least OSX doesn't have useable host/bin/ from our automation).

:nalexander said he'd take an initial look at this

Comment 1

[Nick Alexander :nalexander [he/him]]

Attachment: Bug 1465836 - Make MOZ_AUTOMATION artifact builds pull host binaries (mar,mbsdiff).

This populates $OBJDIR/dist/host/bin as part of |mach artifact install|.

It's clear that the artifacts code is showing its age, 'cuz the effort
to group the mar and mbsdiff files into one processed JAR isn't worth
it.

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

Comment 2

[Justin Wood (:Callek)]

Comment on attachment 8982298 [details]
Bug 1465836 - Make MOZ_AUTOMATION artifact builds pull host binaries (mar,mbsdiff).

I did a push to try with the same try syntax message that spurred this bug and all looks well with this patch.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e20081cc8eef87f12f2aaea6234c9fecccb64d03&selectedJob=181268547

Comment 3

[Kim Moir [:kmoir] ET]

triaging, assigning to nalexander since he attached patches

Comment 4

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8982298 [details]
Bug 1465836 - Make MOZ_AUTOMATION artifact builds pull host binaries (mar,mbsdiff).

https://reviewboard.mozilla.org/r/248248/#review255174

::: commit-message-763f3:5
(Diff revision 1)
> +It's clear that the artifacts code is showing its age, 'cuz the effort
> +to group the mar and mbsdiff files into one processed JAR isn't worth
> +it.

I don't understand this part of the commit message, I'm not sure it's adding much.

::: python/mozbuild/mozbuild/artifacts.py:198
(Diff revision 1)
> +            orig_basename = os.path.basename(filename).split('-', 1)[1]
> +            if self._host_bins_re.match('public/build/host/bin/{}'.format(orig_basename)):

`dirname(filename)` should end with "public/build/host/bin" here, right? If so we could avoid the duplication here. As is, it's clumsy to match a regex we build that starts with `public/build/host/bin` with a string we build to start with `public/build/host/bin`, although I expect the result to be correct because we're still matching `(mar|mbsdiff)` against the filename.

::: python/mozbuild/mozbuild/artifacts.py:890
(Diff revision 1)
>                   cache_dir='.', hg=None, git=None, skip_cache=False,
>                   topsrcdir=None):
>          if (hg and git) or (not hg and not git):
>              raise ValueError("Must provide path to exactly one of hg and git")
>  
> +        self._defines = defines

Moving this isn't necessary, it'll just confuse blame down the road.

::: python/mozbuild/mozbuild/artifacts.py:910
(Diff revision 1)
>              self._artifact_job = get_job_details(self._job, log=self._log,
>                                                   download_symbols=self._download_symbols,
> +                                                 download_host_bins=self._download_host_bins,
>                                                   substs=self._substs)

This argument order is inconsistent with the new definition of `get_job_details`.

Comment 5

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8982298 [details]
Bug 1465836 - Make MOZ_AUTOMATION artifact builds pull host binaries (mar,mbsdiff).

https://reviewboard.mozilla.org/r/248248/#review255174

> I don't understand this part of the commit message, I'm not sure it's adding much.

So right now it's a pain to take `artifact1` and `artifact2` and produce a single `artifact1and2.preprocessed.jar`.  So I don't do that, and we make `artifact{1,2}.processed.jar`, even though they're related and tiny.  I'll reword the commit message to say this.

> `dirname(filename)` should end with "public/build/host/bin" here, right? If so we could avoid the duplication here. As is, it's clumsy to match a regex we build that starts with `public/build/host/bin` with a string we build to start with `public/build/host/bin`, although I expect the result to be correct because we're still matching `(mar|mbsdiff)` against the filename.

No, `filename` is something like `$CACHE/$HASH-mar.exe`.  We can either split the regex into "path bits" and "filename bits" so that we can use them independently, or we can reconstruct the input data so that it matches the single regex.

An obvious refactor/rewrite of the artifacts code is to maintain more information about the artifact data from TC, which would preserve the artifact path (with "public/build/..."), so I elected to generate the bit that will be available sometime in the future.  I agree that it's clumsy, but it's also pretty clearly doing what it's doing, without the reader needing to know the details about the filename.  (Perhaps splitting the regex into path bits and filename bits would also be clear in that respect.)

Since you phrased this as an "if-then", and the "if" is not true, I'm going to drop it.

> Moving this isn't necessary, it'll just confuse blame down the road.

Sure, I'll roll this back.  I was trying to keep a parallel structure where `download_symbols, download_host_bins` would always appear in that order in the file, but I think I found that I couldn't make it consistent throughout anyway.

Comment 6

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8982298 [details]
Bug 1465836 - Make MOZ_AUTOMATION artifact builds pull host binaries (mar,mbsdiff).

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

Comment 7

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8982298 [details]
Bug 1465836 - Make MOZ_AUTOMATION artifact builds pull host binaries (mar,mbsdiff).

https://reviewboard.mozilla.org/r/248248/#review255258

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/188e8b4ba822
Make MOZ_AUTOMATION artifact builds pull host binaries (mar,mbsdiff). r=chmanchester

Comment 9

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/188e8b4ba822

Comment 10

[Nick Alexander :nalexander [he/him]]

Clearing NI: this has landed.