1077326 - Errors after ./mach mercurial-setup

Status: RESOLVED FIXED

(Firefox Build System :: Mach Core, enhancement)

Description

[Anirudh GP(:anirudhgp)]

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

[Mike Hommey [:glandium]]

(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

Comment 2

[Josh Matthews [:jdm]]

Mike, could you provide a bit of information about the expected changes here?

Comment 3

[(not currently active) Ted Mielczarek]

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

[Mike Hommey [:glandium]]

It is. The expected changes would be to make mercurial-setup never ask about the extensions mentioned in comment 1 on windows.

Comment 5

[Gregory Szorc [:gps]]

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?

Comment 6

[Anirudh GP(:anirudhgp)]

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

Comment 7

[Gregory Szorc [:gps]]

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

[Mike Hommey [:glandium]]

bzpost fails because of Queue missing.

Comment 9

[Mike Hommey [:glandium]]

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

[Gregory Szorc [:gps]]

How does Queue (presumably from requests) leak into bzpost? Through Bugsy?

`hg --traceback` should tell you.

Comment 11

[Mike Hommey [:glandium]]

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

[Gregory Szorc [:gps]]

We can write a minimal Mercurial extension that prints available extensions and invoke `hg --config extensions.moduletest=/path/to/dummy/module.py printmodules`

Comment 13

[Dawid Pura]

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

Comment 14

[Dawid Pura]

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

[Josh Matthews [:jdm]]

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

[Marco Bonardo [:mak]]

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

[Marco Bonardo [:mak]]

actually, after I disabled mqext and bzpost, firefoxtree started working properly... looks like there's a dependency somewhere.

Comment 18

[Dawid Pura]

I disabled only bzpost and it's working perfectly (win 8.1)

Comment 19

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(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

[Gregory Szorc [:gps]]

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

Comment 21

[Dawid Pura]

Attachment: Patch with testing hg's extensions requirements

I prepared solution for this issue, patch as attachement. Please if someone can review, thanks!

Comment 22

[Josh Matthews [:jdm]]

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?

Comment 23

[Dawid Pura]

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

Comment 24

[Dawid Pura]

Attachment: Updated patch with inversion pointed out by jdm

Thanks to jdm i fix condition without inversion. See and review this attachement please.

Comment 25

[Mike Hommey [:glandium]]

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.

Comment 26

[Dawid Pura]

Attachment: 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.

Comment 27

[Mike Hommey [:glandium]]

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

Comment 28

[Dawid Pura]

Attachment: 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!

Comment 29

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/71d1b1fad33e

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/71d1b1fad33e