Closed Bug 1064316 Opened 10 years ago Closed 6 years ago

check_spidermonkey_style.py fails if a file is not in the manifest

Categories

(Core :: JavaScript Engine, defect, P3)

x86_64
Linux
defect

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: lth, Assigned: jandem)

References

Details

Attachments

(3 files, 1 obsolete file)

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$'
Priority: -- → P3
(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.
Flags: needinfo?(n.nethercote)
Blocks: 1063728
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 :)
Attached patch PatchSplinter Review
Comment on attachment 8967677 [details] [diff] [review]
Patch

It also works on Windows.
Attachment #8967677 - Flags: review?(n.nethercote)
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.
Attachment #8967677 - Flags: review?(n.nethercote) → review+
Flags: needinfo?(n.nethercote)
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.
Keywords: leave-open
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
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.
Attachment #8968885 - Flags: review?(nicolas.b.pierron)
Attached patch Fix |mach check-spidermonkey| (obsolete) — Splinter Review
check_spidermonkey_style.py and check_macroassembler_style.py should now be run from topsrcdir instead of topsrcdir/js/src
Attachment #8968900 - Flags: review?(gps)
Attachment #8968900 - Attachment is obsolete: true
Attachment #8968900 - Flags: review?(gps)
Attachment #8968901 - Flags: review?(gps)
Attachment #8968885 - Flags: review?(nicolas.b.pierron) → review+
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.
Attachment #8968901 - Flags: review?(gps) → review+
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
Keywords: leave-open
https://hg.mozilla.org/mozilla-central/rev/909e130076ec
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in before you can comment on or make changes to this bug.