Errors after ./mach mercurial-setup

RESOLVED FIXED in mozilla36

Status

Firefox Build System
Mach Core
RESOLVED FIXED
4 years ago
3 months ago

People

(Reporter: anirudhgp, Assigned: Dawid Pura, Mentored)

Tracking

unspecified
mozilla36
x86_64
Windows 8
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [npotb])

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

4 years ago
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.
(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
Mentor: mh+mozilla
Whiteboard: [good-first-bug][lang=python]

Updated

4 years ago
Whiteboard: [good-first-bug][lang=python] → [good first bug][lang=python]

Comment 2

4 years ago
Mike, could you provide a bit of information about the expected changes here?
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?
It is. The expected changes would be to make mercurial-setup never ask about the extensions mentioned in comment 1 on windows.

Comment 5

4 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

4 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

4 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".)
bzpost fails because of Queue missing.
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.
How does Queue (presumably from requests) leak into bzpost? Through Bugsy?

`hg --traceback` should tell you.
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)
We can write a minimal Mercurial extension that prints available extensions and invoke `hg --config extensions.moduletest=/path/to/dummy/module.py printmodules`

Updated

4 years ago
Blocks: 1092476
(Assignee)

Comment 13

4 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

4 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?
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.
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?
actually, after I disabled mqext and bzpost, firefoxtree started working properly... looks like there's a dependency somewhere.
(Assignee)

Comment 18

4 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
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

4 years ago
Created attachment 8523445 [details] [diff] [review]
Patch with testing hg's extensions requirements

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

4 years ago
Attachment #8523445 - Flags: review?(mh+mozilla)
Attachment #8523445 - Flags: review+
Attachment #8523445 - Flags: feedback+
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

4 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

4 years ago
Created attachment 8523496 [details] [diff] [review]
Updated patch with inversion pointed out by jdm

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 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

4 years ago
Created attachment 8527280 [details] [diff] [review]
bug-1077326-fix.patch

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 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

4 years ago
Created attachment 8528496 [details] [diff] [review]
Updated patch with nits problems pointed out by glandium removed

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)
Attachment #8528496 - Flags: review?(mh+mozilla) → review+
Keywords: checkin-needed
Whiteboard: [good first bug][lang=python] → [npotb]
https://hg.mozilla.org/integration/mozilla-inbound/rev/71d1b1fad33e
Assignee: nobody → puradawid
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/71d1b1fad33e
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Depends on: 1112431

Updated

3 months ago
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.