1182677 - More aggressively prompt about running `mach mercurial-setup`

Status: RESOLVED FIXED

(Firefox Build System :: Mach Core, enhancement)

Description

[Gregory Szorc [:gps]]

Not running tools or running out-of-date tools is bad for developer productivity and makes Mozilla move slower. Let's have mach more aggressively poke developers to have a good experience. First step: ensuring `mach mercurial-setup` is executed more frequently.

Comment 1

[Gregory Szorc [:gps]]

Attachment: MozReview Request: Bug 1182677 - Touch last check file before main command invocation; r?smacleod

Bug 1182677 - Touch last check file before main command invocation; r?smacleod

While not common, there are a few cases where `mach mercurial-setup`
fails. When checking the last time this command was executed, it is
sufficient to record the intent to run the command, not the fact that it
executed to completion.

Comment 2

[Gregory Szorc [:gps]]

Attachment: MozReview Request: Bug 1182677 - Add doctstring for `mach mercurial-setup`; r?smacleod

Bug 1182677 - Add doctstring for `mach mercurial-setup`; r?smacleod

Mach grew support for docstrings in `mach help` output a few weeks ago.
Add a docstring for `mach mercurial-setup`.

Comment 3

[Gregory Szorc [:gps]]

Attachment: MozReview Request: Bug 1182677 - Refactor state directory lookup into own function; r?smacleod

Bug 1182677 - Refactor state directory lookup into own function; r?smacleod

A subsequent commit will want to access the state directory path without
possibly creating it. Make that possible by extracting path resolution
to its own function.

Comment 4

[Gregory Szorc [:gps]]

Attachment: MozReview Request: Bug 1182677 - Support calling a function during mach command dispatch; r?smacleod

Bug 1182677 - Support calling a function during mach command dispatch; r?smacleod

This will give us the ability to execute custom code at command dispatch
time. We can use this for global tests before dispatch.

Comment 5

[Gregory Szorc [:gps]]

Attachment: MozReview Request: Bug 1182677 - Aggressively prompt to run `mach mercurial-setup`; r?smacleod, lmandel

Bug 1182677 - Aggressively prompt to run `mach mercurial-setup`; r?smacleod, lmandel

Having not configured or out-of-date tools benefits nobody. It slows
people down.

Version control tools are an integral part of working on Firefox. It is
important for version control tools to be configured optimally and to be
continuously updated so they stay optimal.

The `mach mercurial-setup` command exists to optimally configure
Mercurial for working on Firefox and other Mozilla projects.

This commit adds a pre-dispatch handler to mach that will verify
Mercurial is in a happy state. If `mach mercurial-setup` has never
executed, it will complain. If `mach mercurial-setup` hasn't been
executed in the past 2 weeks, it will complain.

Yes, aborting command execution and forcing people to context switch to
run `mach mercurial-setup` is annoying. First, we have carved out
several exceptions to this behavior, including detection for running in
automation, on the machines of curmudgeons, when Mercurial isn't being
used, and from non-interactive processes. Second, I argue that people
ignore optional notifications and that having persistently
poorly-configured tools is worse than a single context switch at most
every 2 weeks. Therefore, the heavyhanded approach is justified.

In addition, if we did support a non-fatal notification, we would
introduce the problem of extra output from commands. If anyone was e.g.
parsing mach output, we could very likely break those systems. These
cases should be caught by the isatty() check or be running in a context
with MOZ_AUTOMATION set. But you never know.

Comment 6

[Lawrence Mandel [:lmandel] (use needinfo)]

https://reviewboard.mozilla.org/r/13043/#review11641

::: build/mach_bootstrap.py:54
(Diff revision 1)
> +MERCURIAL_SETUP_FATAL_INTERVAL = 14 * 24 * 60 * 60

Is two weeks the right refresh period? Seems pretty frequent to require running setup.

What's stopping the tool from simply running mercurial-setup at a set refresh period?

Comment 7

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/13043/#review11641

> Is two weeks the right refresh period? Seems pretty frequent to require running setup.
> 
> What's stopping the tool from simply running mercurial-setup at a set refresh period?

I picked two weeks because it isn't too frequent and isn't too infrequent. That being said, literally anything is better than what we have now (nothing), so if you think 2 weeks is too frequent, we can back off.

If mach were running in the background, we could definitely code in background auto refresh. But it isn't. So, this needs to be injected as part of regular commands. And, I'm not a huge fan of a) silently doing things b) changing command output.

Comment 8

[Steven MacLeod [:smacleod]]

Comment on attachment 8632357 [details]
MozReview Request: Bug 1182677 - Touch last check file before main command invocation; r?smacleod

https://reviewboard.mozilla.org/r/13045/#review11837

::: tools/mercurial/mach_commands.py:37
(Diff revision 1)
> +        # persistently fail and we people to get credit for the intention,

"and we people to get credit"

Comment 9

[Steven MacLeod [:smacleod]]

Comment on attachment 8632358 [details]
MozReview Request: Bug 1182677 - Add doctstring for `mach mercurial-setup`; r?smacleod

https://reviewboard.mozilla.org/r/13047/#review11839

::: tools/mercurial/mach_commands.py:29
(Diff revision 1)
> +        This command will inspect your Mercurial configuration and will

get rid of the "will" at the end of this line

::: tools/mercurial/mach_commands.py:30
(Diff revision 1)
> +        guide you through an interactive wizard that will help you configure

s/that will help you/,helping you

::: tools/mercurial/mach_commands.py:33
(Diff revision 1)
> +        The command respects user choice: no changes are made without

switch from "this command" to "the command" seems strange to me for some reason. How about "User choice is respected: ..."

::: tools/mercurial/mach_commands.py:36
(Diff revision 1)
> +        If "--update-only" is used, the command becomes non-interactive

"the command" vs "this command" again.

Comment 10

[Steven MacLeod [:smacleod]]

Comment on attachment 8632359 [details]
MozReview Request: Bug 1182677 - Refactor state directory lookup into own function; r?smacleod

https://reviewboard.mozilla.org/r/13049/#review11841

Ship It!

Comment 11

[Steven MacLeod [:smacleod]]

Comment on attachment 8632360 [details]
MozReview Request: Bug 1182677 - Support calling a function during mach command dispatch; r?smacleod

https://reviewboard.mozilla.org/r/13051/#review11845

::: python/mach/mach/registrar.py:65
(Diff revision 1)
> +            try:
> +                prerun = context.pre_dispatch_handler
> +            except AttributeError:
> +                prerun = None

`prerun = getattr(context, 'pre_dispatch_handler', None)`

Comment 12

[Steven MacLeod [:smacleod]]

Comment on attachment 8632361 [details]
MozReview Request: Bug 1182677 - Aggressively prompt to run `mach mercurial-setup`; r?smacleod, lmandel

https://reviewboard.mozilla.org/r/13053/#review11849

::: build/mach_bootstrap.py:41
(Diff revision 1)
> +mach has detected that it has been a while since you have ran

s/ran/run

::: build/mach_bootstrap.py:29
(Diff revision 1)
> +mach has detected that you have never ran `mach mercurial-setup`.

s/ran/run

::: build/mach_bootstrap.py:54
(Diff revision 1)
> +MERCURIAL_SETUP_FATAL_INTERVAL = 14 * 24 * 60 * 60

I'm completely fine with increasing this a bit, as lawrence suggested - but I really don't have a strong opinion on what the value should be in the end.

::: build/mach_bootstrap.py:243
(Diff revision 1)
> +        # We are a neckbeard who has found this undocumented variable.

Let's replace "neckbeard" here with "curmudgen". You did however give me a good chuckle with this comment and env var name, heh.

Comment 13

[Gregory Szorc [:gps]]

url:        https://hg.mozilla.org/integration/fx-team/rev/82d949cc91ddedf8ba1c3d9ba5765f4a927ab7b1
changeset:  82d949cc91ddedf8ba1c3d9ba5765f4a927ab7b1
user:       Gregory Szorc <[e-mail]>
date:       Tue Jul 14 13:42:02 2015 -0700
description:
Bug 1182677 - Touch last check file before main command invocation; r=smacleod

While not common, there are a few cases where `mach mercurial-setup`
fails. When checking the last time this command was executed, it is
sufficient to record the intent to run the command, not the fact that it
executed to completion.

url:        https://hg.mozilla.org/integration/fx-team/rev/425a2a8a9ac00a0e8886ff4539fe35edeb857a8c
changeset:  425a2a8a9ac00a0e8886ff4539fe35edeb857a8c
user:       Gregory Szorc <[e-mail]>
date:       Tue Jul 14 13:44:08 2015 -0700
description:
Bug 1182677 - Add doctstring for `mach mercurial-setup`; r=smacleod

Mach grew support for docstrings in `mach help` output a few weeks ago.
Add a docstring for `mach mercurial-setup`.

url:        https://hg.mozilla.org/integration/fx-team/rev/ccc815fce6d29f8ed8cd5e63d82468e864ea2dd6
changeset:  ccc815fce6d29f8ed8cd5e63d82468e864ea2dd6
user:       Gregory Szorc <[e-mail]>
date:       Tue Jul 14 13:44:59 2015 -0700
description:
Bug 1182677 - Refactor state directory lookup into own function; r=smacleod

A subsequent commit will want to access the state directory path without
possibly creating it. Make that possible by extracting path resolution
to its own function.

url:        https://hg.mozilla.org/integration/fx-team/rev/6dd0ea2de7e25c50740e8d241607e714b9a4ae5f
changeset:  6dd0ea2de7e25c50740e8d241607e714b9a4ae5f
user:       Gregory Szorc <[e-mail]>
date:       Tue Jul 14 13:46:33 2015 -0700
description:
Bug 1182677 - Support calling a function during mach command dispatch; r=smacleod

This will give us the ability to execute custom code at command dispatch
time. We can use this for global tests before dispatch.

url:        https://hg.mozilla.org/integration/fx-team/rev/f06616ee7b2b54d63d20ee4795539514d1df8c7b
changeset:  f06616ee7b2b54d63d20ee4795539514d1df8c7b
user:       Gregory Szorc <[e-mail]>
date:       Tue Jul 14 13:53:50 2015 -0700
description:
Bug 1182677 - Aggressively prompt to run `mach mercurial-setup`; r=smacleod

Having not configured or out-of-date tools benefits nobody. It slows
people down.

Version control tools are an integral part of working on Firefox. It is
important for version control tools to be configured optimally and to be
continuously updated so they stay optimal.

The `mach mercurial-setup` command exists to optimally configure
Mercurial for working on Firefox and other Mozilla projects.

This commit adds a pre-dispatch handler to mach that will verify
Mercurial is in a happy state. If `mach mercurial-setup` has never
executed, it will complain. If `mach mercurial-setup` hasn't been
executed in the past 2 weeks, it will complain.

Yes, aborting command execution and forcing people to context switch to
run `mach mercurial-setup` is annoying. First, we have carved out
several exceptions to this behavior, including detection for running in
automation, on the machines of curmudgeons, when Mercurial isn't being
used, and from non-interactive processes. Second, I argue that people
ignore optional notifications and that having persistently
poorly-configured tools is worse than a single context switch at most
every 2 weeks. Therefore, the heavyhanded approach is justified.

In addition, if we did support a non-fatal notification, we would
introduce the problem of extra output from commands. If anyone was e.g.
parsing mach output, we could very likely break those systems. These
cases should be caught by the isatty() check or be running in a context
with MOZ_AUTOMATION set. But you never know.

Comment 14

[Gregory Szorc [:gps]]

I increased the prompt interval to 31 days before landing. Although I did forget to update the commit message. Bleh.

Comment 15

[Gregory Szorc [:gps]]

url:        https://hg.mozilla.org/integration/fx-team/rev/f91b8c846c14d62563f4ee4c94e9f461dfa9cdc0
changeset:  f91b8c846c14d62563f4ee4c94e9f461dfa9cdc0
user:       Gregory Szorc <[e-mail]>
date:       Tue Jul 14 13:58:43 2015 -0700
description:
Backed out changeset f06616ee7b2b (bug 1182677)

TaskCluster is not amused.

Comment 16

[Gregory Szorc [:gps]]

This broke the TaskCluster decision graph job. I thought MOZ_AUTOMATION was supposed to be universally defined in automation. I guess that's only for buildbot/mozharness jobs. I have a few ideas.

Comment 17

[Gregory Szorc [:gps]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=30c37c88656d

Comment 18

[Gregory Szorc [:gps]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=47b6304b6ac2

Comment 19

[Gregory Szorc [:gps]]

I added |if 'TASK_ID' in os.environ| to the skip conditions and Try appears much happier. Still, I've been burned once. I'll let the full Try push complete before relanding this.

Comment 20

[Gregory Szorc [:gps]]

url:        https://hg.mozilla.org/integration/fx-team/rev/6c924c96ea8595785c87f95a89b90c93cf3c7694
changeset:  6c924c96ea8595785c87f95a89b90c93cf3c7694
user:       Gregory Szorc <[e-mail]>
date:       Tue Jul 14 14:20:03 2015 -0700
description:
Bug 1182677 - Aggressively prompt to run `mach mercurial-setup`; r=smacleod

Having not configured or out-of-date tools benefits nobody. It slows
people down.

Version control tools are an integral part of working on Firefox. It is
important for version control tools to be configured optimally and to be
continuously updated so they stay optimal.

The `mach mercurial-setup` command exists to optimally configure
Mercurial for working on Firefox and other Mozilla projects.

This commit adds a pre-dispatch handler to mach that will verify
Mercurial is in a happy state. If `mach mercurial-setup` has never
executed, it will complain. If `mach mercurial-setup` hasn't been
executed in the past 31 days, it will complain.

Yes, aborting command execution and forcing people to context switch to
run `mach mercurial-setup` is annoying. First, we have carved out
several exceptions to this behavior, including detection for running in
automation, on the machines of curmudgeons, when Mercurial isn't being
used, and from non-interactive processes. Second, I argue that people
ignore optional notifications and that having persistently
poorly-configured tools is worse than a single context switch at most
every month. Therefore, the heavyhanded approach is justified.

In addition, if we did support a non-fatal notification, we would
introduce the problem of extra output from commands. If anyone was e.g.
parsing mach output, we could very likely break those systems. These
cases should be caught by the isatty() check or be running in a context
with MOZ_AUTOMATION set. But you never know.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/82d949cc91dd
https://hg.mozilla.org/mozilla-central/rev/425a2a8a9ac0
https://hg.mozilla.org/mozilla-central/rev/ccc815fce6d2
https://hg.mozilla.org/mozilla-central/rev/6dd0ea2de7e2
https://hg.mozilla.org/mozilla-central/rev/6c924c96ea85

Comment 22

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

I saw error message after executing "mach mercurial-setup".

Error running mach:

    ['mercurial-setup']

The error occurred in the implementation of the invoked mach command.

This should never occur and is likely a bug in the implementation of that
command. Consider filing a bug for this issue.

If filing a bug, please include the full output of mach, including this error
message.

The details of the failure are as follows:

IOError: [Errno 2] No such file or directory: u'/home/shawnjohnjr/.mozbuild/mercurial/setup.lastcheck'

  File "/code/b2g-inbound/b2g-inbound/tools/mercurial/mach_commands.py", line 54, in mercurial_setup
    with open(state_path, 'a')

Comment 23

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

I manually create folder '.mozbuild/mercurial', the problem will be resolved, but somehow it blocks me for a while.

Comment 24

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8632361 [details]
MozReview Request: Bug 1182677 - Aggressively prompt to run `mach mercurial-setup`; r?smacleod, lmandel

Greg and I discussed the review frequency yesterday. There is enough change to warrant prompting every two weeks. We'll try this to start and can tweak based on dev feedback.

Comment 25

[Gabriel Luong [:gl] (ΦωΦ)]

(In reply to Shawn Huang [:shawnjohnjr] from comment #22)
> I saw error message after executing "mach mercurial-setup".
> 
> Error running mach:
> 
>     ['mercurial-setup']
> 
> The error occurred in the implementation of the invoked mach command.
> 
> This should never occur and is likely a bug in the implementation of that
> command. Consider filing a bug for this issue.
> 
> If filing a bug, please include the full output of mach, including this error
> message.
> 
> The details of the failure are as follows:
> 
> IOError: [Errno 2] No such file or directory:
> u'/home/shawnjohnjr/.mozbuild/mercurial/setup.lastcheck'
> 
>   File "/code/b2g-inbound/b2g-inbound/tools/mercurial/mach_commands.py",
> line 54, in mercurial_setup
>     with open(state_path, 'a')

I got a similar error. This is a huge problem because I ran 'mach mercurial-setup' and it created an empty '.mozbuild/' folder in my home directory. Now I am expecting to just be able to run 'mach setup', but it still tells me my mercurial is not configured and to run 'mach mercurial-setup'. This meant I was infinitely blocked on building. I think 'mach mercurial-setup' should be 'fixed' before this is option is forced upon us.

Comment 26

[Gregory Szorc [:gps]]

The IOError was fixed in mozilla-central ~12 hours ago in bug 1183982. Pull latest mozilla-central or `mkdir ~/.mozbuild/mercurial` to get out of the infinite loop.

Comment 27

[Philip Chee]

Also the mercurial I'm using comes with TortoiseHg and is version synced with TortoiseHg. Changing the version of mercurial without changing TortoiseHg will probably beak something. Same for my mercurial.ini which is optimized for THG.

Please make sure that any changes you make to mercurial.ini don't break non-mozilla repositories.

Comment 28

[Ben Bucksch (:BenB)]

> We'll try this to start and can tweak based on dev feedback.

Dev feedback is "very unhappy": Bug 1185354