check_spidermonkey_style.py errors out if it is run while there is a new file that is included or compiled but has not been added to the repo. That was a reasonable situation previously but it is no longer that, since the checker runs as part of any local build (bug 1063728). It is unreasonable to require every file to be in the repo before it can be part of a build, it breaks natural development workflows.

I notice that in mozilla-inbound, we have the following in config/check_spidermonkey_style.py: def main(): # Suppress the build time check if MOZ_NO_BUILD_TIME_SM_CHECKS is set. if "MOZ_NO_BUILD_TIME_SM_CHECKS" in os.environ: sys.exit(0) so as a workaround, one can run MOZ_NO_BUILD_TIME_SM_CHECKS=1 ./mach build to skip the check. I believe that would be enough of a solution, but only if the error message is improved to directly suggest that workaround to users. Currently the error message was not self-explanatory, as the message was like this: js/src/vm/ObjectImpl.cpp:12: error: "vm/TypedArrayCommon.h" is included using the wrong path; did you forget a prefix, or is the file not yet committed? which is talking about includes. If it is required that each file that is part of the build should be committed to the repo before it can be built(?), then it would be good if the error message explicitly said that, along with explaining the why of it, something like "To avoid pushing commits that are missing files, each file must be committed into the repository before it can take part in the build.". Also, it would be good if it would state the workaround, e.g. "To skip this check, set the environment variable MOZ_NO_BUILD_TIME_SM_CHECKS=1 before running mach build."

We should just fix it. I think there is no point in looking at the hg manifest.

> We should just fix it. I think there is no point in looking at the hg > manifest. The hard part is reliably knowing which files should be analyzed. E.g. if you have a build dir inside the srcdir it gets tricky.

(In reply to Nicholas Nethercote [:njn] from comment #3) > > We should just fix it. I think there is no point in looking at the hg > > manifest. > > The hard part is reliably knowing which files should be analyzed. E.g. if > you have a build dir inside the srcdir it gets tricky. Does this not work? find js/src | grep '\.cpp$'

(In reply to Nicholas Nethercote [:njn] from comment #3) > The hard part is reliably knowing which files should be analyzed. E.g. if > you have a build dir inside the srcdir it gets tricky. What if we search for all *.h and *.cpp files in js/src and ignore directories that contain typical obj-dir stuff, let's say a config.status file. I know it isn't perfect (but the current script also isn't) and I think it should work pretty well in practice.

So the suggestion in comment 5 works nicely. It's also a lot faster :) My rewrite did find a real style issue: +js/src/jit/none/Lowering-none.cpp:7:9: error: + "jit/x64/Lowering-x64.h" should be included after "jit/Lowering.h" I'll see if I can figure out why the current script didn't catch this...

(In reply to Jan de Mooij [:jandem] from comment #7) > I'll see if I can figure out why the current script didn't catch this... Ah this is an untracked file in my repo! So the current version ignores it but the one that looks at all files in js/src doesn't, so that's exactly the bug we're trying to fix here :)

Comment on attachment 8967677 [details] [diff] [review] Patch It also works on Windows.

Comment on attachment 8967677 [details] [diff] [review] Patch Review of attachment 8967677 [details] [diff] [review]: ----------------------------------------------------------------- Thanks! This script is such a hack, but it's so effective. SpiderMonkey is the only part of Firefox where the #include statement ordering isn't a total disaster.

Pushed by jandemooij@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/4a4a8ad2c4af Rewrite check_spidermonkey_style.py to use os.walk instead of looking at the repo data. r=njn

Keeping this open, I want to fix check_macroassembler_style.py and check_js_msg_encoding.py too, for consistency. These should be simpler because they look at a few files.

This uses os.walk in check_macroassembler_style.py, similar to the previous patch. It makes the script faster and simpler. I also added a sanity check, to make sure the script fails if it doesn't find any MacroAssembler files.

check_spidermonkey_style.py and check_macroassembler_style.py should now be run from topsrcdir instead of topsrcdir/js/src

Pushed by jandemooij@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/ff7588bba148 Rewrite check_macroassembler_style.py to use os.walk instead of looking at the repo data. r=nbp

(In reply to Jan de Mooij [:jandem] from comment #13) > I want to fix check_macroassembler_style.py and > check_js_msg_encoding.py too Actually I'm not rewriting check_js_msg_encoding.py because that one searches the whole repo for *.msg files, and os.walk is slower than |hg manifest| for that. Furthermore, check_js_msg_encoding.py is a lot less likely to fail so I'm not interested in running it as part of the build.

Pushed by jandemooij@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/909e130076ec part 3 - Fix |mach check-spidermonkey| to use correct cwd. r=gps