1246992 - Allow building with artifacts from try builds

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Dave Townsend [:mossop]]

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.

Comment 1

[Nick Alexander :nalexander [he/him]]

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.

Comment 2

[Chris Manchester (limited bugmail, email directly)]

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.

Comment 3

[Chris Manchester (limited bugmail, email directly)]

I'll post a quick strawman based on comment 2.

Comment 4

[Chris Manchester (limited bugmail, email directly)]

Attachment: MozReview Request: Bug 1246992 - Only accept artifacts from a specific revision when one is provided to mach artifact. r=nalexander

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

Comment 5

[Nick Alexander :nalexander [he/him]]

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.

Comment 6

[Chris Manchester (limited bugmail, email directly)]

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.

Comment 7

[Gregory Szorc [:gps]]

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.

Comment 8

[Chris Manchester (limited bugmail, email directly)]

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/

Comment 9

[Nick Alexander :nalexander [he/him]]

(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 10

[Nick Alexander :nalexander [he/him]]

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 ...".

Comment 11

[Chris Manchester (limited bugmail, email directly)]

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 12

[Chris Manchester (limited bugmail, email directly)]

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/

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ff750b001940

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ff750b001940

Comment 15

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Should we document this on the MDN page in some way?

https://developer.mozilla.org/en-US/docs/Artifact_builds

Comment 16

[Chris Manchester (limited bugmail, email directly)]

(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.