Closed
Bug 1077326
Opened 10 years ago
Closed 9 years ago
Errors after ./mach mercurial-setup
Categories
(Firefox Build System :: Mach Core, enhancement)
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla36
People
(Reporter: anirudhgp, Assigned: puradawid, Mentored)
References
Details
(Whiteboard: [npotb])
Attachments
(1 file, 3 obsolete files)
2.35 KB,
patch
|
glandium
:
review+
|
Details | Diff | Splinter Review |
User Agent: Mozilla/5.0 (Windows NT 6.2; WOW64; rv:33.0) Gecko/20100101 Firefox/33.0 Build ID: 20140929180120 Steps to reproduce: On windows 8: after running ./mach mercurial-setup i tried running any hg command. for example hg update. Actual results: i got these errors: https://pastebin.mozilla.org/6695440 Expected results: It should have updated my build.
Comment 1•10 years ago
|
||
(In reply to anirudh.gp from comment #0) > i got these errors: https://pastebin.mozilla.org/6695440 For completeness of the bug, this is the content of the paste: Black Ops@BLACKOPS /c/mozilla-source/mozilla-central $ hg update *** failed to import extension reviewboard from c:\Users\Black Ops\.mozbuild\ver sion-control-tools\hgext\reviewboard\client.py: No module named json *** failed to import extension bzpost from c:\Users\Black Ops\.mozbuild\version- control-tools\hgext\bzpost: No module named Queue *** failed to import extension firefoxtree from c:\Users\Black Ops\.mozbuild\ver sion-control-tools\hgext\firefoxtree: No module named json *** failed to import extension mqext from c:\Users\Black Ops\.mozbuild\mercurial \extensions\mqext: No module named json
Status: UNCONFIRMED → NEW
Ever confirmed: true
Updated•10 years ago
|
Mentor: mh+mozilla
Whiteboard: [good-first-bug][lang=python]
Updated•10 years ago
|
Whiteboard: [good-first-bug][lang=python] → [good first bug][lang=python]
Comment 2•10 years ago
|
||
Mike, could you provide a bit of information about the expected changes here?
Comment 3•10 years ago
|
||
Isn't this the same problem we've had with bzexport on Windows forever, in that the hg binary's baked-in Python doesn't include the full set of standard modules?
Comment 4•10 years ago
|
||
It is. The expected changes would be to make mercurial-setup never ask about the extensions mentioned in comment 1 on windows.
Comment 5•10 years ago
|
||
The reviewboard extension should load without json. I recently regressed this accidentally in https://hg.mozilla.org/hgcustom/version-control-tools/rev/533b26c8bfae because I thought that file was imported with Mercurial's lazy module loading enabled. Derp. I don't see how firefoxtree and mqext are failing. That might be Mercurial being silly. Does it work if you disable the reviewboard extension?
Flags: needinfo?(anirudh.gp)
Reporter | ||
Comment 6•10 years ago
|
||
(In reply to Gregory Szorc [:gps] from comment #5) > The reviewboard extension should load without json. I recently regressed > this accidentally in > https://hg.mozilla.org/hgcustom/version-control-tools/rev/533b26c8bfae > because I thought that file was imported with Mercurial's lazy module > loading enabled. Derp. > > I don't see how firefoxtree and mqext are failing. That might be Mercurial > being silly. > > Does it work if you disable the reviewboard extension? To fix these errors i edited the mercurial.ini config file and removed the reviewboard,bzpost,firefoxtree and mqext extensions from it.
Flags: needinfo?(anirudh.gp)
Comment 7•10 years ago
|
||
Oh, I guess bzpost will fail because Bugsy (Python Bugzilla REST API client) uses json. Not sure about firefoxtree though. Hmmm... mach mercurial-setup should just not prompt to install these extensions if importing the "json" module fails. (Always test the director cause, not a proxy like "is windows".)
Comment 8•10 years ago
|
||
bzpost fails because of Queue missing.
Comment 9•10 years ago
|
||
Also, testing what specific modules are not available in the python that comes with mercurial is not exactly trivial. IOW, here, testing the director cause is a pita.
Comment 10•10 years ago
|
||
How does Queue (presumably from requests) leak into bzpost? Through Bugsy? `hg --traceback` should tell you.
Comment 11•10 years ago
|
||
And, the director causes are a moving target... maybe should just try running hg -c extensions.foo= bar before asking whether to enable foo (bar would need to be some random hg command that does nothing, is fast, and triggers extension loading)
Comment 12•10 years ago
|
||
We can write a minimal Mercurial extension that prints available extensions and invoke `hg --config extensions.moduletest=/path/to/dummy/module.py printmodules`
Assignee | ||
Comment 13•10 years ago
|
||
(In reply to Gregory Szorc [:gps] from comment #12) > We can write a minimal Mercurial extension that prints available extensions > and invoke `hg --config extensions.moduletest=/path/to/dummy/module.py > printmodules` Greg, i will do this task.
Assignee | ||
Comment 14•10 years ago
|
||
What about loading each extension to python, checking modules loaded and see what wont work at particular system? I can't tell how to do second part. Any thoughts?
Comment 15•10 years ago
|
||
I think following the suggestion in comment 11 makes more sense - we can detect if loading each extension will succeed or not (we can run the printmodules command that gps mentions), and add it to the config if it does.
Comment 16•10 years ago
|
||
does this mean on Windows we can't use firefoxtree and thus the new suggested hg workflow with a single local repository? That's sad :( Any suggested workaround?
Comment 17•10 years ago
|
||
actually, after I disabled mqext and bzpost, firefoxtree started working properly... looks like there's a dependency somewhere.
Assignee | ||
Comment 18•10 years ago
|
||
I disabled only bzpost and it's working perfectly (win 8.1)
(In reply to Marco Bonardo [::mak] (needinfo? me) from comment #17) > actually, after I disabled mqext and bzpost, firefoxtree started working > properly... looks like there's a dependency somewhere. See bug 1095016 for a workaround
Comment 20•10 years ago
|
||
Yeah, bzpost doesn't work in MozillaBuild because of a json dependency. firefoxtree and reviewboard should work. Those are the most important ones for your productivity. bzpost will eventually be deprecated once we have the servers update bugs automatically on push :)
Assignee | ||
Comment 21•10 years ago
|
||
I prepared solution for this issue, patch as attachement. Please if someone can review, thanks!
Attachment #8523445 -
Flags: review+
Attachment #8523445 -
Flags: feedback+
Assignee | ||
Updated•10 years ago
|
Attachment #8523445 -
Flags: review?(mh+mozilla)
Attachment #8523445 -
Flags: review+
Attachment #8523445 -
Flags: feedback+
Comment 22•10 years ago
|
||
Comment on attachment 8523445 [details] [diff] [review] Patch with testing hg's extensions requirements Isn't the test inverted here? >+ if test_if_extension_installable(c, name): >+ return Also, won't that skip all remaining extensions?
Assignee | ||
Comment 23•10 years ago
|
||
(In reply to Josh Matthews [:jdm] from comment #22) > Comment on attachment 8523445 [details] [diff] [review] > Patch with testing hg's extensions requirements > > Isn't the test inverted here? > > >+ if test_if_extension_installable(c, name): > >+ return > > Also, won't that skip all remaining extensions? Thanks for pointing it out Josh! Indeed this condition needs an inverse, i would attach new patch ASAP. But about your second question - with all do respect - i guess no, prompt_external_extension function is called for each of external extensions.
Assignee | ||
Comment 24•10 years ago
|
||
Thanks to jdm i fix condition without inversion. See and review this attachement please.
Attachment #8523445 -
Attachment is obsolete: true
Attachment #8523445 -
Flags: review?(mh+mozilla)
Attachment #8523496 -
Flags: review?(mh+mozilla)
Comment 25•10 years ago
|
||
Comment on attachment 8523496 [details] [diff] [review] Updated patch with inversion pointed out by jdm Review of attachment 8523496 [details] [diff] [review]: ----------------------------------------------------------------- Preliminaty note: to help the people who will land this patch, please refresh it with a commit message. See https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F for more details. ::: tools/mercurial/hgsetup/wizard.py @@ +383,5 @@ > if self._prompt_yn(prompt_text): > c.activate_extension(name) > print('Activated %s extension.\n' % name) > > + def test_if_extenstion_installable(self, c, name): typo: extension. A more usual method name would be is_extension_installable or can_install_extension, but maybe can_use_extension is even better. </bikeshedding> @@ +385,5 @@ > print('Activated %s extension.\n' % name) > > + def test_if_extenstion_installable(self, c, name): > + # Load extension to hg and search stdout for some import problems > + path = os.path.join(selt.vcs_tools_dir, 'hgext', name) typo: self. @@ +388,5 @@ > + # Load extension to hg and search stdout for some import problems > + path = os.path.join(selt.vcs_tools_dir, 'hgext', name) > + tmpfile = tempfile.TemporaryFile() > + from subprocess import call > + call('hg --config extensions.textmodule='+path + ' --help', stdout=tmpfile) It is preferable to separate each command line argument when calling call(), like: call(['hg', '--config', 'extensions.textmodule=%s' % path, '--help']) You should use check_output() instead of call() to check the command's stdout. This avoids using a temporary file. BTW, is there a particular reason why you're adding --help? If not, you can skip it. @@ +392,5 @@ > + call('hg --config extensions.textmodule='+path + ' --help', stdout=tmpfile) > + result = True > + for line in tmpfile: > + if('failed to import extension' in line): > + result = False Now here's the bummer: localized mercurial doesn't print "failed to import extension". For example, this is what it tells me: *** foo のインポートに失敗: No module named foo You'll note that "No module named foo" is always in english because it comes from python directly and is not localized, but it varies depending on the error that actually happened. Looking at mercurial code, adding --config ui.traceback=true makes it print a traceback from python, which always starts with "Traceback", in english. You could check for that instead.
Attachment #8523496 -
Flags: review?(mh+mozilla) → feedback-
Assignee | ||
Comment 26•9 years ago
|
||
Bug 1077326 fix - removing all problems existing in pre-review version. Needs review once again. Changelog: 1. Commit message changed. 2. Typos corrected. 3. Wrong parametrization fixed 4. Using subprocess.check_output instead of subprocess.call. 5. Temporary file no longer used. 6. Failcheck based on "Traceback" keyword in error message. 7. Tested for Linux and Windows. 8. "--help" removed.
Attachment #8523496 -
Attachment is obsolete: true
Attachment #8527280 -
Flags: review?(mh+mozilla)
Comment 27•9 years ago
|
||
Comment on attachment 8527280 [details] [diff] [review] bug-1077326-fix.patch Review of attachment 8527280 [details] [diff] [review]: ----------------------------------------------------------------- ::: tools/mercurial/hgsetup/wizard.py @@ +385,4 @@ > c.activate_extension(name) > print('Activated %s extension.\n' % name) > > + def cannot_use_extension(self, c, name, path=None): nit: can_use_extension @@ +388,5 @@ > + def cannot_use_extension(self, c, name, path=None): > + # Load extension to hg and search stdout for printed exceptions > + if not path: > + path = os.path.join(self.vcs_tools_dir, 'hgext', name) > + result = subprocess.check_output(['hg', '--config', 'extensions.testmodule=%s' % path, '--config', 'ui.traceback=true'], stderr=subprocess.STDOUT, shell=True) Please wrap so that it fits in 80 columns. One way to do this is to put each command line option on a different line, like so: result = subprocess.check_output(['hg', '--config', 'extensions.testmodule=%s' % path, '--config', 'ui.traceback=true'], stderr=subprocess.STDOUT, shell=True) Does it fail on Windows without shell=True? I'd hope it wouldn't. It's usually better to avoid shell=True @@ +389,5 @@ > + # Load extension to hg and search stdout for printed exceptions > + if not path: > + path = os.path.join(self.vcs_tools_dir, 'hgext', name) > + result = subprocess.check_output(['hg', '--config', 'extensions.testmodule=%s' % path, '--config', 'ui.traceback=true'], stderr=subprocess.STDOUT, shell=True) > + return "Traceback" in result nit: return "Traceback" not in result @@ +397,5 @@ > # to treating the extension as one in version-control-tools/hgext/ > # in a directory with the same name as the extension and thus also > # flagging the version-control-tools repo as needing an update. > if name not in c.extensions: > + if self.cannot_use_extension(c, name, path): nit: if not self.can_use_extension
Attachment #8527280 -
Flags: review?(mh+mozilla) → feedback+
Assignee | ||
Comment 28•9 years ago
|
||
Thanks for pointing it out, @glandium! I didn't realize that cannot* functions cant be used, i thought it will be less "negated" than using two of 'nots', but ok, now fixed. You're right, shell=True isn't required even by Windows, i just put it during bugfixing i guess. Changelog: 1. line width adjusted to 80 columns standard 2. negated boolean function changed to positive 3. removed shell=True argument from check_output function If something is still wrong please let me know, thanks for your patience Glandium!
Attachment #8527280 -
Attachment is obsolete: true
Attachment #8528496 -
Flags: review?(mh+mozilla)
Updated•9 years ago
|
Attachment #8528496 -
Flags: review?(mh+mozilla) → review+
Updated•9 years ago
|
Keywords: checkin-needed
Whiteboard: [good first bug][lang=python] → [npotb]
Comment 29•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/71d1b1fad33e
Assignee: nobody → puradawid
Keywords: checkin-needed
Comment 30•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/71d1b1fad33e
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Updated•6 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•