If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Artifact builds don't work with anything else than fx-team

RESOLVED FIXED in Firefox 47

Status

()

Core
Build Config
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: glandium, Assigned: chmanchester)

Tracking

(Blocks: 1 bug)

unspecified
mozilla47
Points:
---

Firefox Tracking Flags

(firefox47 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

2 years ago
While the code makes it possible to specify the tree to mach artifact install, the code in config_status.py doesn't allow to set it, so even if you do, say, mach artifact install --tree=mozilla-inbound, your build subsequently fails because config_status.py re-runs mach artifact install without --tree=mozilla-inbound, which then tries to find the build for your changeset on fx-team.
(Reporter)

Comment 1

2 years ago
And same with mach_commands.py (which is why I wanted _run_mach_artifact_install to be used from config_status.py).
Pretty much.  I think the correct way to address this is to grow a ~/.machrc or similar to configure artifacts out of band.  As we move towards caching objects and libraries in the regular build system, it won't be feasible to specify the tree with every build invocation.

Comment 3

2 years ago
I've wanted a global mach configuration file for the longest time. There is even crude support for mach config files in mach itself.
(Reporter)

Comment 4

2 years ago
I think we can reasonably try to find what the tree is, instead of leaving that to the user config, which would have to change if they switch trees and some such.
(In reply to Mike Hommey [:glandium] from comment #4)
> I think we can reasonably try to find what the tree is, instead of leaving
> that to the user config, which would have to change if they switch trees and
> some such.

I didn't know how to do this when I thought about it.  On merge commits, central, fx-team, and mozilla-inbound may refer to the same commit, and I can't really think of a way to disambiguate fx-team and mozilla-inbound.  Granted, such commits are relatively rare, and in this situation, it doesn't really matter which tree we fetch from.

Comment 6

2 years ago
mozext installs some template keywords to identify which tree(s) a changeset is known to be in.

$ hg log -T '{node|short} {join(trees, ", ")}\n'
66bce0b7c669 inbound
c12b2d098ae0 inbound
9c48dbddeeb6 inbound
67c94e29bcac inbound
b67316254602 central, inbound
20fdd2a2344e inbound, central
2007ddbc08c9 inbound, central
77ce3012d481 inbound, central
5b8303030d9c inbound, central
ffac58123eb0 inbound, central
c96da719d455 inbound, central
154d22a2a34c inbound, central
f89869a36b16 inbound, central

There is also {firstpushtree} that identifies the repo a changeset was first pushed to chronologically. You could use this to break ties to maximize your chances for finding artifacts.

See also `hg help -e mozext` for the full list of template keywords. There are a lot.

I think this will help solve your problem.
See Also: → bug 1241712
The thing that tripped me here is that autoland only works with inbound right now so I started using that as my development tree not realising I was hosing myself with artifact.
(Assignee)

Updated

2 years ago
Assignee: nobody → cmanchester
(Assignee)

Comment 8

2 years ago
Created attachment 8711497 [details]
MozReview Request: Bug 1240667 - Detect a tree to use for artifact builds based on recent changesets. r=nalexander

Currenlty --enable-artifact builds will take artifacts from fx-team regardless of the
state of the current working directory. This can lead to broken builds if someone
updates to a tree other than fx-team.

This commit changes the default behavior from tracking fx-team to tracking a tree
associated with a recent ancestor of the working parent. The first tree a commit
appeared in for the first commit that appeared in a tree is taken, but "try" is
ignored as a candidate tree. This will generally select the tree last updated to.

This also fixes a mis-match between tree names according to mozext and branch names in
the taskcluster index preventing artifact download from common integration branches.

Review commit: https://reviewboard.mozilla.org/r/32213/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/32213/
Attachment #8711497 - Flags: review?(nalexander)
Comment on attachment 8711497 [details]
MozReview Request: Bug 1240667 - Detect a tree to use for artifact builds based on recent changesets. r=nalexander

https://reviewboard.mozilla.org/r/32213/#review28831

I'd like to think deeper about what we want to have happen here.  Can we do this in a single `hg log`?  There's an oddity here in that a tree that gets infrequent pushes, like `cedar`, might "steal" the install chain.  That is, if we have

sha1 "Pushed to cedar"
sha2 "Pushed to fx-team"
...
sha1000 "Pushed to fx-team"

and the cedar sha1 doesn't have the right artifacts built, then we'll ignore all the candidate fx-team pushes that *do* have the right artifacts built.

If we instead folded the trees calculation into the existing log, we might just take the newest artifacts, regardless of what tree they were built on.  I thought about doing this when I first wrote this code, and I think it makes more sense.  So, in the case where a tree isn't specified, look for anything with a push to "fx-team, inbound, central" and use that.  What say you?

::: python/mozbuild/mozbuild/artifacts.py:565
(Diff revision 1)
> +        tree = self._tree_replacements.get(tree, tree)

nit: `.get(tree, tree)` took my a second to parse -- consider `.get(tree) or tree`.

::: python/mozbuild/mozbuild/artifacts.py:675
(Diff revision 1)
> +                 'Retrieving artifacts based on tree: {tree}')

"based on"?  Why not "from tree"?  Is there some subtlety we're trying to convey?

::: python/mozbuild/mozbuild/mach_commands.py:1471
(Diff revision 1)
> -    def _compute_defaults(self, tree=None, job=None):
> +    def _find_tree(self, hg):

I'd like to move this functionality into `artifacts.py`, so that potential non-`mach` consumers can access it.

Also, consider unifying some of the error handling with what's already there, perhaps by adding exception types for the two situations, and then catching those exceptions and providing the fine-grained feedback we want in each case.

::: python/mozbuild/mozbuild/mach_commands.py:1484
(Diff revision 1)
> +                [hg, 'log', '-f', '--template', '{join(trees, ",")}\n',

This dramatically changes the execution profile of `mach artifact install`: we've doubled the number of `hg` invocations, and this one isn't even cached!  This needs more discussion.
Attachment #8711497 - Flags: review?(nalexander)
(Assignee)

Comment 10

2 years ago
https://reviewboard.mozilla.org/r/32213/#review28831

I see the oddity, and I think this could impact people working from central as well. To do this right we'll need build in an idea of how to check for artifacts across multiple trees, which I think will require re-structuring much of the current interaction with version control, although that might not be too bad.

> This dramatically changes the execution profile of `mach artifact install`: we've doubled the number of `hg` invocations, and this one isn't even cached!  This needs more discussion.

We can't necessarily cache the trees a revision might be associated with. Maybe that isn't going to end up being a problem, but let's revisit this after the modifications above.
(Assignee)

Updated

2 years ago
Attachment #8711497 - Flags: review?(nalexander)
(Assignee)

Comment 11

2 years ago
Comment on attachment 8711497 [details]
MozReview Request: Bug 1240667 - Detect a tree to use for artifact builds based on recent changesets. r=nalexander

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32213/diff/1-2/
Comment on attachment 8711497 [details]
MozReview Request: Bug 1240667 - Detect a tree to use for artifact builds based on recent changesets. r=nalexander

https://reviewboard.mozilla.org/r/32213/#review29623

This breaks --tree.  I'd like to either repair it, if it's easy, or remove that functionality (parameter and `_tree` member field, as appropriate) entirely.  (We can add it back in follow-up if it's desireable.)

Otherwise, rock on!

::: python/mozbuild/mozbuild/artifacts.py:510
(Diff revision 2)
> -            pushheads = subprocess.check_output([self._hg, 'log',
> +            result = self._index.listNamespaces(rev_ns, {"limit": 10})

Is this safe?  I feel there are more than 10 namespaces in an "average" push.  Is there a reason to not be flexible here, and take maybe 100?

::: python/mozbuild/mozbuild/artifacts.py:658
(Diff revision 2)
> +        # an exact match.

Can you be more precise, like:
```
# an exact match.  For example, |mach artifact install --tree=central| should map to |hg log -r 'pushhead(central)|', which corresponds to the Task Cluster namespaces "mozilla-central".
```
Or whatever is appropriate.

::: python/mozbuild/mozbuild/artifacts.py:693
(Diff revision 2)
> +            rev_info = line.split(',')

Explain why you can't have "sha," here, which results in `rev_trees['sha'] = ('', )`, which seems wrong.

Is it because `trees` is non-empty if `pushhead()` is true?  That seems reasonable, but I'd still like to be defensive here.

::: python/mozbuild/mozbuild/artifacts.py:704
(Diff revision 2)
> +    def find_pushead_artifacts(self, task_cache, tree_cache, job, pushhead, trees):

nit: "pushhead", with two 'h' characters.
Attachment #8711497 - Flags: review?(nalexander) → review+
(Assignee)

Comment 13

2 years ago
https://reviewboard.mozilla.org/r/32213/#review29623

Will repair. Thank you for noticing that.

> Is this safe?  I feel there are more than 10 namespaces in an "average" push.  Is there a reason to not be flexible here, and take maybe 100?

I think this is safe. The namespaces under buildbot.revision.{rev} are trees -- finding multiple entries there equates to a revision being a pushhead on multiple trees. It seems like more than one would be quite rare.

> Explain why you can't have "sha," here, which results in `rev_trees['sha'] = ('', )`, which seems wrong.
> 
> Is it because `trees` is non-empty if `pushhead()` is true?  That seems reasonable, but I'd still like to be defensive here.

Yes, it looks like trees will be non-empty if pushhead() is true, but I see no reason not to be defensive.
(Assignee)

Comment 14

2 years ago
Comment on attachment 8711497 [details]
MozReview Request: Bug 1240667 - Detect a tree to use for artifact builds based on recent changesets. r=nalexander

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32213/diff/2-3/
(Assignee)

Comment 15

2 years ago
Comment on attachment 8711497 [details]
MozReview Request: Bug 1240667 - Detect a tree to use for artifact builds based on recent changesets. r=nalexander

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32213/diff/3-4/

Comment 16

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/4761d06fd0a5

Comment 17

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/4761d06fd0a5
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox47: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
You need to log in before you can comment on or make changes to this bug.