Closed Bug 1246992 Opened 9 years ago Closed 9 years ago

Allow building with artifacts from try builds

Categories

(Firefox Build System :: General, defect)

defect
Not set
normal

Tracking

(firefox47 fixed)

RESOLVED FIXED
mozilla47
Tracking Status
firefox47 --- fixed

People

(Reporter: mossop, Assigned: chmanchester)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

Here is a reasonably common scenario that I find myself in: 1. Write some patch including something that amends compiled code. 2. Get everything working locally 3. Push to try, something fails on a different platform. 4. Fire up my VM or desktop into the appropriate environment and wait an age while it builds my changes so I can debug. If artifact builds could sanely get artifacts from try runs then step 4 would end up being a lot quicker.
This used to be well supported. You could use --disable-compile-environment and then specify the package URL to download, and it worked fine. This conflicts with --enable-artifact-builds, which doesn't give you time to specify the package URL; and with the code from Bug 1238320. I posit that we want a configuration file (.artifactrc?) that lets you specify stuff like this to --enable-artifact-builds, but that's a ways out.
Depends on: 1238320
I expected this to work just by pulling the changesets from try, and it would, except that mozext doesn't acknowledge try as a tree. It would be nice to maintain the idea that for a local tree there is almost always a best set of artifacts from automation that can be determined automatically. From a quick look at the TaskCluster Index documentation this could possibly be done, but only by calling the API once per revision, which will definitely be too slow. As a workaround, I remember testing the ability to |./mach artifact install <url>| when working on the patch for test artifacts, and it worked fine if I ran the command once for the package url, and again for the test url. I think it would be reasonable to have an option that is a revision that means artifacts will only be taken from that revision, even if we can't determine it's a pushhead, and independent of the state of the local repo. I'd be partial to using an environment variable or something in a mozconfig for this rather than inventing a new way to configure the build environment.
I'll post a quick strawman based on comment 2.
Assignee: nobody → cmanchester
Comment on attachment 8717713 [details] MozReview Request: Bug 1246992 - Only accept artifacts from a specific revision when one is provided to mach artifact. r=nalexander https://reviewboard.mozilla.org/r/34277/#review30989 ::: python/mozbuild/mozbuild/artifacts.py:808 (Diff revision 1) > + if revset and re.match('[a-z0-9]{40}', revset): This prevents `mach artifact install BOOKMARK`, and all sorts of other revset goodness. Why not do the thing you are doing more generally? That is, if `revset` is given, only accept it specifically. That suggests moving the "revset='.'" logic out into `install_from`. I expect that conforms to our existing usage internally. ::: python/mozbuild/mozbuild/mach_commands.py:1483 (Diff revision 1) > + 'If the revision provided is a 40 character hg hash, artifacts ' I really prefer to allow the full hg revset spec. We should fight hard to enable it.
https://reviewboard.mozilla.org/r/34277/#review30989 > This prevents `mach artifact install BOOKMARK`, and all sorts of other revset goodness. > > Why not do the thing you are doing more generally? That is, if `revset` is given, only accept it specifically. That suggests moving the "revset='.'" logic out into `install_from`. > > I expect that conforms to our existing usage internally. I was a bit worried about changing the meaning of this for people who already use it, but that I guess that's reasonable.
The real problem here is automation treats each repository as its own "namespace" instead of what it really is: all part of the same DAG. Data is indexed separately, which means that despite your VCS knowing the shape of the DAG and candidate ancestors whose data can be used to bootstrap a build, we have to look in N locations/indexes in automation to find data. Of course, we can't simply throw everything into the same bucket easily because there are fundamental differences between a "build" job in central from a "build" job on beta. We should be indexing results by their VCS revision and distinguishing "repos" by the job/artifact name. e.g. revision X would have a "central build" and a "beta build" job/artifact. This also has advantages that we would be able to trigger beta builds of things when they land on central as a means of testing packaging before waiting for the trains to run months later.
Attachment #8717713 - Attachment description: MozReview Request: Bug 1246992 - Only accept artifacts from a specific revision when a 40 character hash is provided to mach artifact. r=nalexander → MozReview Request: Bug 1246992 - Only accept artifacts from a specific revision when one is provided to mach artifact. r=nalexander
Attachment #8717713 - Flags: review?(nalexander)
Comment on attachment 8717713 [details] MozReview Request: Bug 1246992 - Only accept artifacts from a specific revision when one is provided to mach artifact. r=nalexander Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34277/diff/1-2/
(In reply to Gregory Szorc [:gps] from comment #7) > The real problem here is automation treats each repository as its own > "namespace" instead of what it really is: all part of the same DAG. Data is > indexed separately, which means that despite your VCS knowing the shape of > the DAG and candidate ancestors whose data can be used to bootstrap a build, > we have to look in N locations/indexes in automation to find data. Of > course, we can't simply throw everything into the same bucket easily because > there are fundamental differences between a "build" job in central from a > "build" job on beta. We should be indexing results by their VCS revision and > distinguishing "repos" by the job/artifact name. e.g. revision X would have > a "central build" and a "beta build" job/artifact. This also has advantages > that we would be able to trigger beta builds of things when they land on > central as a means of testing packaging before waiting for the trains to run > months later. I agree, but I'm not sure I see how to improve the situation right now. I guess we could skate to where the puck *may* be by trying to associate a "flavour" of build job ("inbound", "central", "beta") with each artifact install? I think I saw you (gps) and glandium talking about including more of the mozconfig (for example, branding) to distinguish trees -- that leads more directly into the "real cache" solution.
Comment on attachment 8717713 [details] MozReview Request: Bug 1246992 - Only accept artifacts from a specific revision when one is provided to mach artifact. r=nalexander https://reviewboard.mozilla.org/r/34277/#review31113 Just nits. Looks good! ::: python/mozbuild/mozbuild/artifacts.py:692 (Diff revision 2) > # Return an ordered dict associating revisions that are pushheads with nit: update comment. ::: python/mozbuild/mozbuild/artifacts.py:700 (Diff revision 2) > - '-r', 'last(pushhead({tree}) and ::{parent}, {num})'.format( > + '-r', 'last(pushhead({tree}) and ::".", {num})'.format( Are the double quotes needed? We might save Windows pain if we don't quote. ::: python/mozbuild/mozbuild/artifacts.py:737 (Diff revision 2) > + trees = list(known_trees) Let's order this in some way so we're deterministic. Perhaps just sort by tree name? ::: python/mozbuild/mozbuild/artifacts.py:823 (Diff revision 2) > - self.log(logging.ERROR, 'artifact', > + self.log(logging.ERROR, 'artifact', {}, 'No built artifacts found.') Consider adding "Tried to find artifacts for 3 pushheads." ::: python/mozbuild/mozbuild/artifacts.py:832 (Diff revision 2) > + if len(revision) != 40: This is a tiny thing, but you could have a 40 character bookmark name. I think it's okay to always take the `hg log` process overhead for simplicity. ::: python/mozbuild/mozbuild/artifacts.py:837 (Diff revision 2) > + rev_pushheads = collections.OrderedDict({revision: None}) The `OrderedDict` appears unnecessary. Replace with a regular dictionary if it is. ::: python/mozbuild/mozbuild/artifacts.py:842 (Diff revision 2) > + '(revset "{revset}")') nit: perhaps say "matched revset ..." or "given revset ...".
Attachment #8717713 - Flags: review?(nalexander) → review+
https://reviewboard.mozilla.org/r/34277/#review31113 > nit: update comment. This comment is still up to date (but perhaps it could be better). The only change here is that the working parent is always used as the parent revision. > The `OrderedDict` appears unnecessary. Replace with a regular dictionary if it is. It's not used for this path, but it still is for install_from_recent. I'll stop using popitem in install_from_pushheads and use a regular dict here.
Comment on attachment 8717713 [details] MozReview Request: Bug 1246992 - Only accept artifacts from a specific revision when one is provided to mach artifact. r=nalexander Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34277/diff/2-3/
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Should we document this on the MDN page in some way? https://developer.mozilla.org/en-US/docs/Artifact_builds
Flags: needinfo?(cmanchester)
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #15) > Should we document this on the MDN page in some way? > > https://developer.mozilla.org/en-US/docs/Artifact_builds Sure, and I bet there are a few updates we should make to that page. I'll make a note to go over it with nalexander next week.
Flags: needinfo?(cmanchester)
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: