1257478 - Add a git-setup command to mach

Status: RESOLVED FIXED

(Firefox Build System :: Bootstrap Configuration, enhancement)

Description

[Panos Astithas (he/him) [:past] (please ni?)]

I would expect to have a git-setup command, similar to mercurial-setup, that would ensure git is installed and properly configured according to our supported configuration. I guess that includes git-cinnabar and git-bz-moz, but probably not moz-git-bz.

Any objections to the above? Any other things I am missing?

Comment 1

[Gregory Szorc [:gps]]

There should be a single command that covers both VCS systems. We try to avoid mach command bloat. In addition, there has been talking unifying mach {bootstrap, mercurial-setup, doctor} into a single command. (Probably `mach bootstrap`.)

Comment 2

[Ralph Giles (:rillian)]

As an update, `./mach bootstrap` now runs `./mach mercurial-setup` if invoked from a mercurial repository. I think it would be reasonable to add a parallel 'git-setup' command and similar logic to invoke it from `./mach bootstrap`.

Comment 3

[Gregory Szorc [:gps]]

I don't want to provide stop energy here. While we want to unify behind `mach bootstrap`, if `mach git-setup` is easier to implement before `mercurial-setup` is merged into `bootstrap`, implement `mach git-setup`.

Comment 4

[Panos Astithas (he/him) [:past] (please ni?)]

I have a working patch that I will put up shortly.

Comment 5

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Bug 1257478 - Turn mercurial-setup into vcs-setup and add git support.

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

Comment 6

[Gregory Szorc [:gps]]

Comment on attachment 8989965 [details]
Bug 1257478 - Turn mercurial-setup into vcs-setup and add git support.

I'll look at this.

Comment 7

[Panos Astithas (he/him) [:past] (please ni?)]

Feedback is of course more than welcome, but I have another upcoming update that will take care of cloning as well. I just need to test it first before posting.

Comment 8

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8989965 [details]
Bug 1257478 - Turn mercurial-setup into vcs-setup and add git support.

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

Comment 9

[Mike Hommey [:glandium]]

There are way too many mach commands. I think we should factor mercurial-setup and git-setup into one vcs-setup that takes an option for which vcs to choose, which maybe defaults to mercurial.

Comment 10

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8989965 [details]
Bug 1257478 - Turn mercurial-setup into vcs-setup and add git support.

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

Comment 11

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8989965 [details]
Bug 1257478 - Turn mercurial-setup into vcs-setup and add git support.

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

Comment 12

[Panos Astithas (he/him) [:past] (please ni?)]

This is now ready for review. In the latest iteration I've consolidated the two commands to vcs-setup, which can take a --git argument to specify the use of git instead of mercurial, which is the default.

I have tested on Mac and Windows, but I don't have a Linux system around to test there.

Comment 13

[Gregory Szorc [:gps]]

Comment on attachment 8989965 [details]
Bug 1257478 - Turn mercurial-setup into vcs-setup and add git support.

https://reviewboard.mozilla.org/r/254970/#review267632

This looks pretty good!

You may want to run things by glandium to make sure the cinnabar settings are reasonable.

Also, we may have old docs on MDN, etc that reference `mach mercurial-setup`. It /might/ be worth keeping an alias to the old command around until old references and muscle memory can be purged. There's no way to define a command alias, so you would need to implement a separate `@Command` that prints a deprecation warning and calls into the new one.

::: python/mozboot/mozboot/bootstrap.py:610
(Diff revision 4)
> +        print(e)
> +
> +    # Ensure the latest revision of git-bz-moz is present.
> +    bz_dir = os.path.join(root_state_dir, 'git-bz-moz')
> +
> +    # Ensure the latest revision of git-cinnabar is present.

Nit: this comment is wrong.

Comment 14

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8989965 [details]
Bug 1257478 - Turn mercurial-setup into vcs-setup and add git support.

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

Comment 15

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8989965 [details]
Bug 1257478 - Turn mercurial-setup into vcs-setup and add git support.

(In reply to Gregory Szorc [:gps] from comment #13)
> This looks pretty good!
> 
> You may want to run things by glandium to make sure the cinnabar settings
> are reasonable.

Thanks, I used the instructions in this page as a guide, but let's get glandium's opinion as well:

https://github.com/glandium/git-cinnabar/wiki/Mozilla:-A-git-workflow-for-Gecko-development

Comment 16

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Gregory Szorc [:gps] from comment #13)
> Also, we may have old docs on MDN, etc that reference `mach
> mercurial-setup`. It /might/ be worth keeping an alias to the old command
> around until old references and muscle memory can be purged. There's no way
> to define a command alias, so you would need to implement a separate
> `@Command` that prints a deprecation warning and calls into the new one.

I thought about this, but the current spell-checking logic in mach seemed to do a good enough job:

$ ./mach mercurial-setup
It looks like you are trying to run an unknown mach command: mercurial-setup

Did you want to run any of these commands instead: vcs-setup?

Run |mach help| to show a list of commands.

I'm curious what glandium thinks, too.

Comment 17

[Gregory Szorc [:gps]]

The spelling correction should hopefully be "good enough." If it's not, we can add the alias back in as a follow-up.

Comment 18

[Mike Hommey [:glandium]]

Comment on attachment 8989965 [details]
Bug 1257478 - Turn mercurial-setup into vcs-setup and add git support.

https://reviewboard.mozilla.org/r/254970/#review267960

::: python/mozboot/mozboot/bootstrap.py:179
(Diff revision 5)
>      '"elementary OS"',
>      '"elementary"'
>  )
>  
> +ADD_GIT_TOOLS_PATH = '''
> +To add git-cinnabar and git-bz-moz to the PATH, edit your shell

A few weeks away from phabricator being mandatory, it seems backwards to add anything related to git-bz-moz.

::: python/mozboot/mozboot/bootstrap.py:597
(Diff revision 5)
> +    """Ensure git-cinnabar and git-bz-moz are up to date."""
> +    cinnabar_dir = os.path.join(root_state_dir, 'git-cinnabar')
> +
> +    # Ensure the latest revision of git-cinnabar is present.
> +    update_git_repo(git, 'https://github.com/glandium/git-cinnabar.git',
> +                    cinnabar_dir, 'master')

use the release branch, not master.

::: python/mozboot/mozboot/bootstrap.py:657
(Diff revision 5)
> +    try:
> +        subprocess.check_call([git, 'init', dest])
> +        # Configure git per the git-cinnabar requirements.
> +        subprocess.check_call([git, 'remote', 'add', 'mozilla',
> +                               'hg::https://hg.mozilla.org/mozilla-unified',
> +                               '-t', 'bookmarks/central'], cwd=dest)

if you're setting remote.mozilla.fetch, -t bookmarks/central is useless.

Also note that I recently (3 weeks ago) changed the wiki page, and you should probably update this accordingly.

::: python/mozboot/mozboot/bootstrap.py:671
(Diff revision 5)
> +        subprocess.check_call([git, 'config', 'push.default', 'upstream'], cwd=dest)
> +        subprocess.check_call([git, 'config', 'pull.ff', 'only'], cwd=dest)

For instance, I don't recommend these anymore.

::: python/mozboot/mozboot/bootstrap.py:677
(Diff revision 5)
> +        subprocess.check_call([git, 'config', 'pull.ff', 'only'], cwd=dest)
> +
> +        watchman_sample = os.path.join(dest, '.git/hooks/fsmonitor-watchman.sample')
> +        # Older versions of git didn't include fsmonitor-watchman.sample.
> +        if watchman_sample:
> +            print('Configuring watchman')

maybe only do that if watchman is available?

Comment 19

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8989965 [details]
Bug 1257478 - Turn mercurial-setup into vcs-setup and add git support.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/254970/diff/5-6/

Comment 20

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8989965 [details]
Bug 1257478 - Turn mercurial-setup into vcs-setup and add git support.

I updated the patch per the comments above with two small exceptions:
- I still see the recommendation for pull.ff=only in the wiki: "I'd recommend setting the pull.ff configuration to "only", by the way"
- watchman is installed earlier in the mach bootstrap process, so it should be available at this point, and the git hook already contains fallback logic in case watchman isn't found

Let me know if you feel strongly about either of these.

Comment 21

[Mike Hommey [:glandium]]

(In reply to Panos Astithas (he/him) [:past] (please ni?) (away until Aug 12) from comment #20)
> - watchman is installed earlier in the mach bootstrap process

Only on osx and freebsd at the moment, apparently.

> so it should be available at this point, and the git hook already contains fallback logic
> in case watchman isn't found

It does so rather noisily.
"open2: exec of watchman -j --no-pretty failed: No such file or directory at .git/hooks/query-watchman line 46."

Comment 22

[Mike Hommey [:glandium]]

Comment on attachment 8989965 [details]
Bug 1257478 - Turn mercurial-setup into vcs-setup and add git support.

https://reviewboard.mozilla.org/r/254970/#review268688

::: python/mozboot/mozboot/bootstrap.py:633
(Diff revisions 5 - 6)
> -                               '-t', 'bookmarks/central'], cwd=dest)
> -        subprocess.check_call([git, 'remote', 'set-url', '--push', 'mozilla',
> +        subprocess.check_call([git, 'remote', 'add', 'inbound',
> +                               'hg::ssh://hg.mozilla.org/integration/mozilla-inbound'],

Not sure we should do that by default, considering phabricator has auto-landing.

Why did you remove try, though? Because mach try? I guess that's fair.

::: python/mozboot/mozboot/bootstrap.py:659
(Diff revisions 5 - 6)
>      res = subprocess.call([git, 'remote', 'update'], cwd=dest)
>      print('')
>      if res:
>          print('error updating; try running `git remote update` manually')
>          return False
>  
> -    res = subprocess.call([git, 'checkout', '-b', 'central', 'mozilla/central'], cwd=dest)
> +    res = subprocess.call([git, 'checkout', '-b', 'central', 'origin/bookmarks/central'], cwd=dest)

Since you're only setting up one useful remote, and you're cloning, now, you don't need to git remote update anymore, because that will be a no-op.

Same is true for checkout.

Comment 23

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8989965 [details]
Bug 1257478 - Turn mercurial-setup into vcs-setup and add git support.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/254970/diff/6-7/

Comment 24

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8989965 [details]
Bug 1257478 - Turn mercurial-setup into vcs-setup and add git support.

(In reply to Mike Hommey [:glandium] from comment #22)
> Not sure we should do that by default, considering phabricator has
> auto-landing.

Adding inbound seems useful to provide support for a still widely-used workflow, even though the plan is very much to stop supporting it at some point.

> Why did you remove try, though? Because mach try? I guess that's fair.

Exactly.

> Since you're only setting up one useful remote, and you're cloning, now, you
> don't need to git remote update anymore, because that will be a no-op.
> 
> Same is true for checkout.

Done. The reason I added checkout for the central bookmark is that the repo seemed to be in a weird state by checking out hg tip. If we expect most users to quickly add a central branch for their regular workflow (I certainly do), then it would seem appropriate to get them quickly in the default state, wouldn't it? I'm not going to push very strongly for this though if you disagree.

I've also added a check for the existence of watchman as you suggested.

Comment 25

[Mike Hommey [:glandium]]

Comment on attachment 8989965 [details]
Bug 1257478 - Turn mercurial-setup into vcs-setup and add git support.

https://reviewboard.mozilla.org/r/254970/#review269392

::: python/mozboot/mozboot/bootstrap.py:618
(Diff revision 7)
> +    """Ensure git-cinnabar is up to date."""
> +    cinnabar_dir = os.path.join(root_state_dir, 'git-cinnabar')
> +
> +    # Ensure the latest revision of git-cinnabar is present.
> +    update_git_repo(git, 'https://github.com/glandium/git-cinnabar.git',
> +                    cinnabar_dir, 'release')

Having now changed the default branch to master, it would be better to just not explicitly give a branch here.

Comment 26

[Mike Hommey [:glandium]]

> If we expect most users to quickly add a central branch for their regular workflow (I certainly do), then it would seem appropriate to get them quickly in the default state, wouldn't it? 

That seems fair, but you don't need a separate checkout step for that. You can git clone url -b branch. The ideal solution would be for bug 1325327 to be fixed, though, as it would make a plain git clone work (as well as hg clone, btw).

Comment 27

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8989965 [details]
Bug 1257478 - Turn mercurial-setup into vcs-setup and add git support.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/254970/diff/7-8/

Comment 28

[Panos Astithas (he/him) [:past] (please ni?)]

> Having now changed the default branch to master, it would be better to just
> not explicitly give a branch here.

Done.

> That seems fair, but you don't need a separate checkout step for that. You
> can git clone url -b branch. The ideal solution would be for bug 1325327 to
> be fixed, though, as it would make a plain git clone work (as well as hg
> clone, btw).

The patch is now using 'git clone -b' with the only downside being that the branch name is 'bookmarks/central' instead of central, but I guess that's ok.

Comment 29

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8989965 [details]
Bug 1257478 - Turn mercurial-setup into vcs-setup and add git support.

(In reply to Panos Astithas (he/him) [:past] (please ni?) from comment #28)
> > Having now changed the default branch to master, it would be better to just
> > not explicitly give a branch here.
> 
> Done.
> 
> > That seems fair, but you don't need a separate checkout step for that. You
> > can git clone url -b branch. The ideal solution would be for bug 1325327 to
> > be fixed, though, as it would make a plain git clone work (as well as hg
> > clone, btw).
> 
> The patch is now using 'git clone -b' with the only downside being that the
> branch name is 'bookmarks/central' instead of central, but I guess that's ok.

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/138014f66617
Turn mercurial-setup into vcs-setup and add git support. r=gps

Comment 31

[Tiberius Oros[:tiberius_oros]]

https://hg.mozilla.org/mozilla-central/rev/138014f66617