Closed
Bug 1465836
Opened 6 years ago
Closed 6 years ago
Artifact builds should be able to pull and upload host/bin/{mar,mbsdiff}{,.exe}
Categories
(Firefox Build System :: General, enhancement)
Tracking
(firefox62 fixed)
RESOLVED
FIXED
mozilla62
Tracking | Status | |
---|---|---|
firefox62 | --- | fixed |
People
(Reporter: Callek, Assigned: nalexander)
References
Details
Attachments
(1 file)
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
Flags: needinfo?(nalexander)
Comment hidden (mozreview-request) |
Reporter | ||
Comment 2•6 years ago
|
||
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
Attachment #8982298 -
Flags: feedback+
Comment 3•6 years ago
|
||
triaging, assigning to nalexander since he attached patches
Assignee: nobody → nalexander
Comment 4•6 years ago
|
||
mozreview-review |
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`.
Attachment #8982298 -
Flags: review?(cmanchester)
Assignee | ||
Comment 5•6 years ago
|
||
mozreview-review-reply |
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 hidden (mozreview-request) |
Comment 7•6 years ago
|
||
mozreview-review |
Comment on attachment 8982298 [details] Bug 1465836 - Make MOZ_AUTOMATION artifact builds pull host binaries (mar,mbsdiff). https://reviewboard.mozilla.org/r/248248/#review255258
Attachment #8982298 -
Flags: review?(cmanchester) → review+
Pushed by nalexander@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/188e8b4ba822 Make MOZ_AUTOMATION artifact builds pull host binaries (mar,mbsdiff). r=chmanchester
Comment 9•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/188e8b4ba822
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox62:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
Updated•5 years ago
|
Version: Version 3 → 3 Branch
You need to log in
before you can comment on or make changes to this bug.
Description
•