1234913 - Implement |mach artifact install| for git users

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Nick Alexander :nalexander [he/him]]

Right now, artifacts.py requires hg with the `pushlog` extension: https://dxr.mozilla.org/mozilla-central/source/python/mozbuild/mozbuild/artifacts.py#364.

This ticket tracks making this work with git.  The hard parts here are likely to be maintaining the upstream pushlog locally, and converting upstream push SHAs (which are hg) into local git commits SHAs.  I believe there's a mapping service endpoint for this, but I don't know the API or how well it will perform in practice.

Comment 1

[Gregory Szorc [:gps]]

https://api.pub.build.mozilla.org/mapper/gecko-dev/mapfile/full contains the SHA-1 map for the gecko-dev Git repo. See https://wiki.mozilla.org/ReleaseEngineering/Applications/Mapper for the full API, which supports querying individual SHA-1s. FWIW, I've considered exposing these Git SHA-1s (complete with URLs to git.mo and/or GitHub) on hg.mo. If that makes things easier for you, let me know. We could potentially expose them through the pushlog on hg.mo as well.

Of course, git-cinnabar has its own set of SHA-1s and there is a different mechanism to do the SHA-1 lookup there. So when you say "work with git" there are 2 "gits" that need to be implemented.

Comment 2

[Nick Alexander :nalexander [he/him]]

Attachment: MozReview Request: Bug 1234913 - Pre: Allow to --skip-cache when running |mach artifact install|. r?chmanchester

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

Comment 3

[Nick Alexander :nalexander [he/him]]

Attachment: MozReview Request: Bug 1234913 - Pre: Show download progress during |mach artifact install|. r?chmanchester

Hotel WiFi inspires many things.

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

Comment 4

[Nick Alexander :nalexander [he/him]]

Attachment: MozReview Request: Bug 1234913 - Part 1: Clarify things that are hg-specific; make iteration lazy. r?chmanchester

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

Comment 5

[Nick Alexander :nalexander [he/him]]

Attachment: MozReview Request: Bug 1234913 - Part 2: Support git in |mach artifact install|. r?chmanchester

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

Comment 6

[Nick Alexander :nalexander [he/him]]

Attachment: MozReview Request: Bug 1234913 - Post: Stop using local pushlog in |mach artifact install|. r?chmanchester

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

Comment 7

[Nick Alexander :nalexander [he/him]]

Attachment: MozReview Request: Bug 1234913 - Post: Unify trees and known_trees; remove explicit trees. r?chmanchester

Just a little clean-up and simplification.

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

Comment 8

[Nick Alexander :nalexander [he/him]]

ahunt: testing of this feature in a git repo would be appreciated.  It should work more or less exactly as it does in an hg repo.

Comment 9

[Mike Kaply [:mkaply]]

FYI, I just applied these patches in my local tree and whereas I couldn't get the pushlog stuff to work, this worked great.

Fastest build ever. Nice work.

Comment 10

[Mike Kaply [:mkaply]]

Sorry, this was mercurial, not git.

Comment 11

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8723437 [details]
MozReview Request: Bug 1234913 - Pre: Allow to --skip-cache when running |mach artifact install|. r?chmanchester

https://reviewboard.mozilla.org/r/36551/#review33191

I will use this.

Comment 12

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8723438 [details]
MozReview Request: Bug 1234913 - Pre: Show download progress during |mach artifact install|. r?chmanchester

https://reviewboard.mozilla.org/r/36553/#review33193

::: python/mozbuild/mozbuild/artifacts.py:643
(Diff revision 1)
> +            # Python doesn't make it easy to capture a variable outside of the
> +            # function scope.  last[0] is our last printed value.
> +            last = [-1]

This is probably the most objectionable thing about python I've seen so far. Making this a property on self would be a little easier on the eyes later on.

Comment 13

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8723439 [details]
MozReview Request: Bug 1234913 - Part 1: Clarify things that are hg-specific; make iteration lazy. r?chmanchester

https://reviewboard.mozilla.org/r/36555/#review33203

This is an improvement independent of later revisions.

Comment 14

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8723440 [details]
MozReview Request: Bug 1234913 - Part 2: Support git in |mach artifact install|. r?chmanchester

https://reviewboard.mozilla.org/r/36557/#review33205

As discussed irl this exposes a hazard when querying many revisions that don't have artifacts yet, so we're going to re-think this.

Comment 15

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8723441 [details]
MozReview Request: Bug 1234913 - Post: Stop using local pushlog in |mach artifact install|. r?chmanchester

https://reviewboard.mozilla.org/r/36559/#review33207

Comment 16

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8723442 [details]
MozReview Request: Bug 1234913 - Post: Unify trees and known_trees; remove explicit trees. r?chmanchester

https://reviewboard.mozilla.org/r/36561/#review33209

Comment 17

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/36557/#review33225

::: python/mozbuild/mozbuild/artifacts.py:784
(Diff revision 1)
> +    def _find_git_pushheads(self, rev):

There is a mozversioncontrol Python package under python/. I'd like to see the version control interaction code live there to prevent duplication across the tree.

::: python/mozbuild/mozbuild/artifacts.py:799
(Diff revision 1)
> +            self._git, 'rev-list', '--ancestry-path',

You want `--topo-order` here.

::: python/mozbuild/mozbuild/mach_commands.py:1477
(Diff revision 1)
> +        self.virtualenv_manager.install_pip_package('mercurial==3.7.1')

Oy. This could cause pain. git-cinnabar already has hg installed. Why is this needed?

Comment 18

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/36559/#review33229

::: python/mozbuild/mozbuild/artifacts.py:767
(Diff revision 1)
> -            yield rev_info[0], tuple(rev_info[1:])
> +            yield (hg_hash, trees)

Nit: don't need () when returning tuples.

Comment 19

[Nick Alexander :nalexander [he/him]]

https://reviewboard.mozilla.org/r/36557/#review33225

> Oy. This could cause pain. git-cinnabar already has hg installed. Why is this needed?

I'm about to file a bug because `install_pip_package` appears broken on multiple machines, so please explain more.  The only Python that it makes sense to use to run cinnabar is the virtualenv Python; and it definitely does *not* have mercurial installed, so what do you propose?

Comment 20

[Mike Hommey [:glandium]]

(In reply to Nick Alexander :nalexander from comment #19)
> The only Python that it makes sense to use to run cinnabar is the virtualenv Python; and it definitely
> does *not* have mercurial installed, so what do you propose?

Huh? If you run "git cinnabar" it's *very* unlikely to be using the virtualenv python.

Comment 21

[Chris Manchester (limited bugmail, email directly)]

I volunteered to see this through.

Comment 22

[Chris Manchester (limited bugmail, email directly)]

Attachment: MozReview Request: Bug 1234913 - Pre: Allow to --skip-cache when running |mach artifact install|. r=chmanchester

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

Comment 23

[Chris Manchester (limited bugmail, email directly)]

Attachment: MozReview Request: Bug 1234913 - Pre: Show download progress during |mach artifact install|. r=nalexander

Hotel WiFi inspires many things.

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

Comment 24

[Chris Manchester (limited bugmail, email directly)]

Attachment: MozReview Request: Bug 1234913 - Clarify things that are hg-specific; make iteration lazy. r=chmanchester

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

Comment 25

[Chris Manchester (limited bugmail, email directly)]

Attachment: MozReview Request: Bug 1234913 - Stop using local pushlog in |mach artifact install|. r=nalexander

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

Comment 26

[Chris Manchester (limited bugmail, email directly)]

Attachment: MozReview Request: Bug 1234913 - Support git in |mach artifact install|. r=nalexander

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

Comment 27

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8725026 [details]
MozReview Request: Bug 1234913 - Pre: Allow to --skip-cache when running |mach artifact install|. r=chmanchester

https://reviewboard.mozilla.org/r/37259/#review33845

This is Nick's code, already reviewed.

Comment 28

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8725028 [details]
MozReview Request: Bug 1234913 - Clarify things that are hg-specific; make iteration lazy. r=chmanchester

https://reviewboard.mozilla.org/r/37263/#review33847

This is Nick's code, already reviewed.

Comment 29

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8725027 [details]
MozReview Request: Bug 1234913 - Pre: Show download progress during |mach artifact install|. r=nalexander

https://reviewboard.mozilla.org/r/37261/#review33853

Sure, this was your review comment.  Right?

Comment 30

[Chris Manchester (limited bugmail, email directly)]

https://reviewboard.mozilla.org/r/37261/#review33853

Yep. I also noticed we need to guard dl.set_progress in case dl is None.

Comment 31

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8725029 [details]
MozReview Request: Bug 1234913 - Stop using local pushlog in |mach artifact install|. r=nalexander

https://reviewboard.mozilla.org/r/37265/#review33855

Nice work!  I was always trying to do this in one changeset-based query, and couldn't get it to work.  Did you try `from=HASH1&to=HASH2` or whatever the arguments are, and find it didn't work?

I have one larger request: could you log as you're making each request and iterating each result so we can see more of the work?  (Could be DEBUG level logging, if you see fit.)

::: python/mozbuild/mozbuild/artifacts.py:563
(Diff revision 1)
> +        if req.status_code >= 400:

Why not outside of `[200, 299]`?

::: python/mozbuild/mozbuild/artifacts.py:573
(Diff revision 1)
> +        end = pushid

Having the constant baked in here makes the function much less clear.  It doesn't appear to impact the cache, so can you just put `start, end` in the parameters?

::: python/mozbuild/mozbuild/artifacts.py:768
(Diff revision 1)
> -    def _find_hg_pushheads(self):
> +    def _pushheads_from_rev(self, rev, count):

The comment is a little unclear.  The "contains a push introducing `rev`" is per tree?  And then you return that pushhead, and `count - 1` before that?

::: python/mozbuild/mozbuild/artifacts.py:769
(Diff revision 1)
> -        """Return an iterator of (hg_hash, {tree-set}) associating hg revision
> +        """Queries the hg.m.o's json-pushlog for the last `count` pushhead revisions

nit: no sense abbreviating -- might as well help somebody who's searching.  Although I guess they'll see the actual URLs...

Also, drop "the".  (Or "hg.mo.o's".)

::: python/mozbuild/mozbuild/artifacts.py:809
(Diff revision 1)
> +        last_revs = self._get_recent_revisions()

Let's be clear that `_get_recent_revisions` is *public* or *remote* revisions.

Comment 32

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8725030 [details]
MozReview Request: Bug 1234913 - Support git in |mach artifact install|. r=nalexander

https://reviewboard.mozilla.org/r/37267/#review33863

Not much of substance here -- just some nits and tidying up.

Usually I would stamp ship it, but I'm trying to break my habit of doing so to prepare for a more security conscious me; and because I want to test this particular implementation locally.

Great work!

::: python/mozbuild/mozbuild/artifacts.py:809
(Diff revision 1)
> +            cinnabar, 'git2hg'

This approach *definitely* didn't work for me locally; I will test now.

::: python/mozbuild/mozbuild/artifacts.py:968
(Diff revision 1)
>      def install_from_hg_revset(self, revset, distdir):

This name is now misleading, in that it doesn't install from revsets if we're using git.  Can you reconsider the callers and either split the function or weaken the name?

::: python/mozbuild/mozbuild/artifacts.py:975
(Diff revision 1)
> +            if len(revset) != 40:

I feel like we should be able to git-ify the underlying idea, allowing named references, etc.  `git rev-parse` would do it, wouldn't?

Comment 33

[Nick Alexander :nalexander [he/him]]

https://reviewboard.mozilla.org/r/37267/#review33863

Local testing in hg and git went well.  I uncovered a weirdness around importing using cinnabar that exposes a weakness in our approach (namely, cinnabar doesn't accurately reflect hg's published state; we're using all 0s as a proxy) but it's not something we can or should addres.

Comment 34

[Chris Manchester (limited bugmail, email directly)]

https://reviewboard.mozilla.org/r/37265/#review33855

Thanks for the super speedy review!

I didn't try the fromchange/tochange arguments, because of an earlier version of this where I wasn't assuming anything about how many revisions might be in a push. Now that I'm using a fixed range I could, but the result would be about the same.

Comment 35

[Chris Manchester (limited bugmail, email directly)]

https://reviewboard.mozilla.org/r/37267/#review33863

Can you say a bit more about this? In this implementation we're strongly assuming that the first changeset we have locally that's known to hg will also be known to automation. If this is a bad assumtion under some circumstances we need to accomodate that.

Comment 36

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8725026 [details]
MozReview Request: Bug 1234913 - Pre: Allow to --skip-cache when running |mach artifact install|. r=chmanchester

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

Comment 37

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8725027 [details]
MozReview Request: Bug 1234913 - Pre: Show download progress during |mach artifact install|. r=nalexander

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

Comment 38

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8725028 [details]
MozReview Request: Bug 1234913 - Clarify things that are hg-specific; make iteration lazy. r=chmanchester

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

Comment 39

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8725029 [details]
MozReview Request: Bug 1234913 - Stop using local pushlog in |mach artifact install|. r=nalexander

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

Comment 40

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8725030 [details]
MozReview Request: Bug 1234913 - Support git in |mach artifact install|. r=nalexander

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

Comment 41

[Nick Alexander :nalexander [he/him]]

https://reviewboard.mozilla.org/r/37267/#review33863

Sure.  My local hg clone, `gecko`, confuses cinnabar.  Therefore I have a second `gecko-clean` that has no local changes.  To apply from RB, I pull into `gecko-clean` and then use cinnabar to pull into `gecko.git`.  (Convoluted, I know -- but I don't know how to pull a specific RB reference in cinnabar.  It is possible, I am told.)

The commits in `gecko-clean` that came from RB are not public, since RB is not publishing.  But cinnabar seems them as coming from an external repository, hence has an hg hash for them; and that's the bit that we switch on.  Of course, such a commit has never been pushed to a known repository.  (Well, until we generalize to try, which is dicy for other reasons.)

It's possible cinnabar knows about hg's phase state, and we could do better, but that is *definitely* followup :)

Comment 42

[Nick Alexander :nalexander [he/him]]

https://reviewboard.mozilla.org/r/37265/#review33995

::: python/mozbuild/mozbuild/artifacts.py:810
(Diff revisions 1 - 2)
> +        to be known to mozilla automation.

nit: capital M.  (Through-out.)

::: python/mozbuild/mozbuild/artifacts.py:839
(Diff revisions 1 - 2)
> +                            'Search started with {rev}, which must be known to mozilla automation.\n\n'

nit: capital M.

Comment 43

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8725030 [details]
MozReview Request: Bug 1234913 - Support git in |mach artifact install|. r=nalexander

https://reviewboard.mozilla.org/r/37267/#review33997

Nice!

Let's get a tester or two to try this before we post to dev.*.  Great work!

::: python/mozbuild/mozbuild/artifacts.py:995
(Diff revisions 1 - 2)
> -            revision = revset
> +            if len(revision.split('\n')) != 1:

I feel it's worth throwing if `git2hg` gives all 0s here, since it'll be common.

Comment 44

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f1dc3fbce25d
https://hg.mozilla.org/integration/mozilla-inbound/rev/1b7bcd5eefb3
https://hg.mozilla.org/integration/mozilla-inbound/rev/205f1459dcb8
https://hg.mozilla.org/integration/mozilla-inbound/rev/ac1fff91cf4d
https://hg.mozilla.org/integration/mozilla-inbound/rev/643e7face8c8

Comment 45

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f1dc3fbce25d
https://hg.mozilla.org/mozilla-central/rev/1b7bcd5eefb3
https://hg.mozilla.org/mozilla-central/rev/205f1459dcb8
https://hg.mozilla.org/mozilla-central/rev/ac1fff91cf4d
https://hg.mozilla.org/mozilla-central/rev/643e7face8c8

Comment 46

[Andrzej Hunt :ahunt]

(In reply to Nick Alexander :nalexander from comment #43)
> Comment on attachment 8725030 [details]
> MozReview Request: Bug 1234913 - Support git in |mach artifact install|.
> r=nalexander
> 
> https://reviewboard.mozilla.org/r/37267/#review33997
> 
> Nice!
> 
> Let's get a tester or two to try this before we post to dev.*.  Great work!
> 

It worked for me on a fresh pull of fx-team, building fennec.

(This was on a build where nalexander manually fixed my virtualenv last week, I've  not tried a completely from scratch build yet.)

Comment 47

[Henrik Skupin [:whimboo][⌚️UTC+1]]

So what's necessary to get this running with git-cinnabar? For me it still fails with the latest code on inbound in artifact install.

Getting https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Artifact_builds updated would be nice too.