Closed
Bug 1182677
Opened 9 years ago
Closed 9 years ago
More aggressively prompt about running `mach mercurial-setup`
Categories
(Firefox Build System :: Mach Core, enhancement)
Firefox Build System
Mach Core
Tracking
(firefox42 fixed)
RESOLVED
FIXED
mozilla42
Tracking | Status | |
---|---|---|
firefox42 | --- | fixed |
People
(Reporter: gps, Assigned: gps)
References
Details
Attachments
(5 files)
40 bytes,
text/x-review-board-request
|
smacleod
:
review+
|
Details |
40 bytes,
text/x-review-board-request
|
smacleod
:
review+
|
Details |
40 bytes,
text/x-review-board-request
|
smacleod
:
review+
|
Details |
MozReview Request: Bug 1182677 - Support calling a function during mach command dispatch; r?smacleod
40 bytes,
text/x-review-board-request
|
smacleod
:
review+
|
Details |
40 bytes,
text/x-review-board-request
|
lmandel
:
review+
smacleod
:
review+
|
Details |
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.
Assignee | ||
Comment 1•9 years ago
|
||
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.
Attachment #8632357 -
Flags: review?(smacleod)
Assignee | ||
Comment 2•9 years ago
|
||
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`.
Attachment #8632358 -
Flags: review?(smacleod)
Assignee | ||
Comment 3•9 years ago
|
||
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.
Attachment #8632359 -
Flags: review?(smacleod)
Assignee | ||
Comment 4•9 years ago
|
||
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.
Attachment #8632360 -
Flags: review?(smacleod)
Assignee | ||
Comment 5•9 years ago
|
||
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.
Attachment #8632361 -
Flags: review?(smacleod)
Attachment #8632361 -
Flags: review?(lmandel)
Comment 6•9 years ago
|
||
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?
Assignee | ||
Comment 7•9 years ago
|
||
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.
Updated•9 years ago
|
Attachment #8632357 -
Flags: review?(smacleod) → review+
Comment 8•9 years ago
|
||
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•9 years ago
|
||
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.
Attachment #8632358 -
Flags: review?(smacleod) → review+
Comment 10•9 years ago
|
||
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!
Attachment #8632359 -
Flags: review?(smacleod) → review+
Updated•9 years ago
|
Attachment #8632360 -
Flags: review?(smacleod) → review+
Comment 11•9 years ago
|
||
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•9 years ago
|
||
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.
Attachment #8632361 -
Flags: review?(smacleod) → review+
Assignee | ||
Comment 13•9 years ago
|
||
url: https://hg.mozilla.org/integration/fx-team/rev/82d949cc91ddedf8ba1c3d9ba5765f4a927ab7b1 changeset: 82d949cc91ddedf8ba1c3d9ba5765f4a927ab7b1 user: Gregory Szorc <gps@mozilla.com> 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 <gps@mozilla.com> 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 <gps@mozilla.com> 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 <gps@mozilla.com> 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 <gps@mozilla.com> 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.
Assignee | ||
Comment 14•9 years ago
|
||
I increased the prompt interval to 31 days before landing. Although I did forget to update the commit message. Bleh.
Assignee | ||
Comment 15•9 years ago
|
||
url: https://hg.mozilla.org/integration/fx-team/rev/f91b8c846c14d62563f4ee4c94e9f461dfa9cdc0 changeset: f91b8c846c14d62563f4ee4c94e9f461dfa9cdc0 user: Gregory Szorc <gps@mozilla.com> date: Tue Jul 14 13:58:43 2015 -0700 description: Backed out changeset f06616ee7b2b (bug 1182677) TaskCluster is not amused.
Assignee | ||
Comment 16•9 years ago
|
||
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.
Assignee | ||
Comment 17•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=30c37c88656d
Assignee | ||
Comment 18•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=47b6304b6ac2
Assignee | ||
Comment 19•9 years ago
|
||
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.
Assignee | ||
Comment 20•9 years ago
|
||
url: https://hg.mozilla.org/integration/fx-team/rev/6c924c96ea8595785c87f95a89b90c93cf3c7694 changeset: 6c924c96ea8595785c87f95a89b90c93cf3c7694 user: Gregory Szorc <gps@mozilla.com> 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•9 years ago
|
||
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
Assignee: nobody → gps
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox42:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
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 manually create folder '.mozbuild/mercurial', the problem will be resolved, but somehow it blocks me for a while.
Comment 24•9 years ago
|
||
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.
Attachment #8632361 -
Flags: review?(lmandel) → review+
Comment 25•9 years ago
|
||
(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.
Assignee | ||
Comment 26•9 years ago
|
||
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•9 years ago
|
||
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•9 years ago
|
||
> We'll try this to start and can tweak based on dev feedback. Dev feedback is "very unhappy": Bug 1185354
Updated•7 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•