1453854 - Hint message from check_spidermonkey_style.py could be more helpful

Status: NEW

(Core :: JavaScript Engine, defect, P5)

Description

[Botond Ballo [:botond]]

I am mentoring a bug that involves replacing an MFBT header with a standard header across the Gecko codebase (bug 1434710).

The Try server is showing me that SpiderMonkey builds are failing with an error that was not present in a local Gecko build.

The errors shown in the Treeherder failure summary are:

TEST-UNEXPECTED-FAIL | check_spidermonkey_style.py | actual output does not match expected output; diff is above.
TEST-UNEXPECTED-FAIL | check_spidermonkey_style.py | Hint: If the problem is that you renamed a header, and many #includes are no longer in alphabetical order, commit your work and then try `check_spidermonkey_style.py --fixup`. You need to commit first because --fixup modifies your files in place.

I tried running `check_spidermonkey_style.py --fixup` as suggested.

The first thing I noticed is that there is no check_spidermonkey_style.py file in the root of the m-c source tree. It's located in the 'config' subdirectory.

So next I tried running `config/check_spidermonkey_style.py --fixup`. I got:

-bash: config/check_spidermonkey_style.py: Permission denied

It seems the file is not marked as executable, as is therefore intended to be run via 'python'. Third attempt:

$ python ./config/check_spidermonkey_style.py --fixup
Traceback (most recent call last):
  File "./config/check_spidermonkey_style.py", line 45, in <module>
    from mozversioncontrol import get_repository_from_env
ImportError: No module named mozversioncontrol

I don't know what's causing this error or how to fix it, or even where to look to find that out.

I think we could make the experience better for a developer faced an error like this.

Comment 1

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #0)
> The Try server is showing me that SpiderMonkey builds are failing with an
> error that was not present in a local Gecko build.

Correction: it's not only SpiderMonkey builds that are failing with this error, but also regular Linux builds. Which makes me wonder why I am not getting the error in my local Linux build.

Comment 2

[Jeff Walden [:Waldo]]

The style checker doesn't run in local builds because it "takes too long".  Or at least, when I added it to the regular build process awhile back people complained about it taking too long and it got undone.  I continue to think this is Dumb.

The messages here could be better, for sure.  I'll leave this open/alone for that and comment on the original bug about how to fix, without having to block on this getting better.

Comment 3

[Botond Ballo [:botond]]

(In reply to Jeff Walden [:Waldo] from comment #2)
> The messages here could be better, for sure.  I'll leave this open/alone for
> that and comment on the original bug about how to fix, without having to
> block on this getting better.

(Note, I've already figured out how to fix the error in the original bug. While I haven't been able to get check_spidermonkey_style.py to run, I looked at the full error in the automation run's raw log, and made the change manually. It was just a header order issue.)

Comment 4

[Jeff Walden [:Waldo]]

The expected way to run the style checker is to run

  make check-style

inside an objdir, either of JS shell or all of Mozilla, I think.  This too could be better explained in the error messages.

Comment 5

[Botond Ballo [:botond]]

(In reply to Jeff Walden [:Waldo] from comment #4)
> The expected way to run the style checker is to run
> 
>   make check-style
> 
> inside an objdir, either of JS shell or all of Mozilla, I think.  This too
> could be better explained in the error messages.

Thanks. Looks like that needs to be invoked in <objdir>/js/src. It also doesn't accept the --fixup option, so I still don't know how to invoke the checker with the --fixup option.

In any case, it would be nice if there was a mach wrapper for this.

Comment 6

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #5)
> Thanks. Looks like that needs to be invoked in <objdir>/js/src. It also
> doesn't accept the --fixup option, so I still don't know how to invoke the
> checker with the --fixup option.

Never mind. |make check-style| (without any additional options) *does* invoke the checker with the fixup option, and that makes the necessary changes to the source directory.

Comment 7

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #6)
> (In reply to Botond Ballo [:botond] from comment #5)
> > Thanks. Looks like that needs to be invoked in <objdir>/js/src. It also
> > doesn't accept the --fixup option, so I still don't know how to invoke the
> > checker with the --fixup option.
> 
> Never mind. |make check-style| (without any additional options) *does*
> invoke the checker with the fixup option, and that makes the necessary
> changes to the source directory.

Er... ignore that, I just got confused. |make check-style| does not run the checker with the --fixup option.

Comment 8

[Jan de Mooij [:jandem]]

(In reply to Botond Ballo [:botond] from comment #5)
> In any case, it would be nice if there was a mach wrapper for this.

|./mach check-spidermonkey| runs the JS tests including the style checker. IIRC it runs the style checker pretty early on so you don't have to wait too long (after jsapi-tests but before the much slower jit-tests and jstests).