Closed Bug 1063728 Opened 5 years ago Closed 2 years ago

check_spidermonkey_style.py needs to run as part of the build, not make check

Categories

(Core :: JavaScript Engine, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla35
Tracking Status
firefox61 --- fixed

People

(Reporter: ehsan, Assigned: jandem)

References

Details

Attachments

(1 file, 4 obsolete files)

I have at least been caught by this script not running locally once on inbound and several times on try.  And I don't even hack on the JS engine.  :-)
Assignee: nobody → ehsan.akhgari
Attachment #8485181 - Flags: review?(mshal)
check_spidermonkey_style.py takes at least 3 seconds on my very fast Mac + SSD, likely more on most other machines. Most JS hackers build the shell many times per day, it'd be sad if we made this slower by running the style checker on every build IMO...
So do you think the current situation is preferred?
Flags: needinfo?(jdemooij)
(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #3)
> So do you think the current situation is preferred?

Perhaps instead we could make the builders run |make check-style| instead?
Or rather, in addition to |make| and instead of |make check| (which I guess got disabled awhile ago).
(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #3)
> So do you think the current situation is preferred?

It sucks when the build breaks due to some trivial #include issue that's not caught locally. I run the script locally whenever I touch includes in a non-trivial way, but it's not ideal.

Can you add an environment variable to disable this check? We can just make the Python script exit immediately if that's set.

Then I (and other SM hackers who want this) can just set this locally to get the current behavior. What do you think? :)
Flags: needinfo?(jdemooij)
(In reply to Jan de Mooij [:jandem] from comment #2)
> check_spidermonkey_style.py takes at least 3 seconds on my very fast Mac +
> SSD, likely more on most other machines. Most JS hackers build the shell
> many times per day, it'd be sad if we made this slower by running the style
> checker on every build IMO...

I don't think 3-5s or so is that onerous.

And even if it were.  I count several times in the last few months, since filing bug 996385, where this would have avoided tree closures, eliminated the need for review comments, etc.  A minute a day per JS hacker is easily outweighed by the cost of the occasional tree closure affecting everyone working on Mozilla code.
Duplicate of this bug: 996385
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #7)
> (In reply to Jan de Mooij [:jandem] from comment #2)
> > check_spidermonkey_style.py takes at least 3 seconds on my very fast Mac +
> > SSD, likely more on most other machines. Most JS hackers build the shell
> > many times per day, it'd be sad if we made this slower by running the style
> > checker on every build IMO...
> 
> I don't think 3-5s or so is that onerous.

That would double my touch-a-single-cpp-file rebuild time. I'd be fine with an env variable to disable, however.
Attachment #8485181 - Attachment is obsolete: true
Attachment #8485181 - Flags: review?(mshal)
Attachment #8485208 - Flags: review?(jdemooij)
Comment on attachment 8485208 [details] [diff] [review]
Run check_spidermonkey_style.py before starting to build; r=jandem

Review of attachment 8485208 [details] [diff] [review]:
-----------------------------------------------------------------

Great, thanks for adding the override!
Attachment #8485208 - Flags: review?(jdemooij) → review+
(And to be clear, I do think it's a great idea to enable this by default. It's much nicer for new contributors as well.)
I wrote check_spidermonkey_style.py without any consideration to performance, largely because it was only run as a part of |make check|. I bet a small amount of profiling would lead to significant speedups.
(In reply to Nicholas Nethercote [:njn] from comment #14)
> I wrote check_spidermonkey_style.py without any consideration to
> performance, largely because it was only run as a part of |make check|. I
> bet a small amount of profiling would lead to significant speedups.

I filed bug 1063924 to make the script a bit faster.
Depends on: 1063972
check_spidermonkey_style.py now runs if you invoke |mach build| without having made any modifications. Is this expected?
Flags: needinfo?(ehsan.akhgari)
The way this change was made, yes.  It's probably possible to make check-style only run if relevant source files changed, tho -- something like putting $(CPPSRCS) or so in the dependencies list for the check-style rule.  New bug for that?
Flags: needinfo?(ehsan.akhgari)
AFAICT, builds are now broken by check_spidermonkey_style if a new file in your local repo has not yet been added to the repo.  This is not really a feature so long as running check-style is not optional: it precludes applying patches and running with them in a straightforward way.
https://hg.mozilla.org/mozilla-central/rev/9a796d86bae8
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #17)
> The way this change was made, yes.  It's probably possible to make
> check-style only run if relevant source files changed, tho -- something like
> putting $(CPPSRCS) or so in the dependencies list for the check-style rule. 
> New bug for that?

Yes.  Please file a bug and list the dependencies for this file.

(In reply to Lars T Hansen [:lth] from comment #18)
> AFAICT, builds are now broken by check_spidermonkey_style if a new file in
> your local repo has not yet been added to the repo.  This is not really a
> feature so long as running check-style is not optional: it precludes
> applying patches and running with them in a straightforward way.

Please file a bug.  I think this is a bug in the checker script, and I don't know how it works.  Please CC :njn on the bug.
(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #20)
> (In reply to Lars T Hansen [:lth] from comment #18)
> > AFAICT, builds are now broken by check_spidermonkey_style if a new file in
> > your local repo has not yet been added to the repo.  This is not really a
> > feature so long as running check-style is not optional: it precludes
> > applying patches and running with them in a straightforward way.
> 
> Please file a bug.  I think this is a bug in the checker script, and I don't
> know how it works.  Please CC :njn on the bug.

Bug 1064316.
It looks like this never got a build peer review.  Offhand, I'd think that would be needed for something that runs anytime anybody runs build...
Flags: needinfo?(ehsan.akhgari)
Comment on attachment 8485208 [details] [diff] [review]
Run check_spidermonkey_style.py before starting to build; r=jandem

Sorry I originally requested mshal's review but redirected to jandem since he had more opinions on the patch.
Attachment #8485208 - Flags: review?(mshal)
Flags: needinfo?(ehsan.akhgari)
I think this change should be backed out. It has caused numerous problems so far:

- Speed (improved by jandem in bug 1063924).

- The "always runs in a no-op |mach build| invocation (comment 16 and comment 17).

- The "files must be committed to be checked problem" (bug 1064316).

- Bug 1063972, which I don't even understand yet.

These problems are currently greater than the problem that this bug is solving, IMO.

Plus it wasn't appropriately reviewed.
Flags: needinfo?(ehsan.akhgari)
I disagree strongly with comment 24.
Sigh.  I'm giving up here, guys.  Feel free to do what you will with this patch.

Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/792b0721903e
Assignee: ehsan.akhgari → nobody
Status: RESOLVED → REOPENED
Flags: needinfo?(ehsan.akhgari)
Resolution: FIXED → ---
Did you still want me to review this? It sounds like there is some disagreement on the direction here.
Flags: needinfo?(ehsan.akhgari)
Comment on attachment 8485208 [details] [diff] [review]
Run check_spidermonkey_style.py before starting to build; r=jandem

I have given up on this bug.  :-)  I'll let others champion this, or close it.
Attachment #8485208 - Flags: review?(mshal)
Flags: needinfo?(ehsan.akhgari)
This bit me today again when pushing <https://hg.mozilla.org/integration/mozilla-inbound/rev/84853fde4f55>.  It would be really really really nice if we did something here.  The current state of affairs is quite hostile to everyone who doesn't hack on the JS engine (and therefore know to run this script) on a daily basis.
Depends on: 1064316
Running the script only when relevant files are changed seems complicated, because the script looks for header/source files (some platform-dependent) in various directories and it would have to depend on all of them.

Using just the *.cpp files in moz.build would miss header file changes or changes to *.cpp files that aren't compiled for the current platform.

Maybe we *could* create the list of input files in moz.build (using os.walk, see the patch for bug 1064316) instead of in check_spidermonkey_style.py, and then pass this list of files to check_spidermonkey_style.py. However we're talking hundreds of files so I don't know if the build system peers would be happy with that.
Flags: needinfo?(nfroyd)
(In reply to Jan de Mooij [:jandem] from comment #30)
> Running the script only when relevant files are changed seems complicated,
> because the script looks for header/source files (some platform-dependent)
> in various directories and it would have to depend on all of them.

This seems like it would also fall afoul of the complain that the style checker would only look at files that are known to hg, or somesuch?  I guess you could also look for files that aren't known to version control and blindly check those "just because".

> Maybe we *could* create the list of input files in moz.build (using os.walk,
> see the patch for bug 1064316) instead of in check_spidermonkey_style.py,
> and then pass this list of files to check_spidermonkey_style.py. However
> we're talking hundreds of files so I don't know if the build system peers
> would be happy with that.

It does sound a lot better than enumerating all the files at test time, every time!

I don't know that we'd necessarily be *unhappy*, per se, but folks have already complained that moz.build files are Turing-complete by virtue of being Python.  I don't think we'd want to add bits that now touch the filesystem in addition to declaring a bunch of things.

Putting it in moz.configure would be more appropriate, I think, but that means that you'd have to re-run configure to update the list of files, which is probably not desirable.  (Then again, you'd have to do the same if you walked the filesystem from within moz.build, so....)
Flags: needinfo?(nfroyd)
(In reply to Nathan Froyd [:froydnj] from comment #31)
> This seems like it would also fall afoul of the complain that the style
> checker would only look at files that are known to hg, or somesuch?  I guess
> you could also look for files that aren't known to version control and
> blindly check those "just because".

I'm fixing this in bug 1064316 by changing it to os.walk instead of (basically) |hg manifest|.

> It does sound a lot better than enumerating all the files at test time,
> every time!
> 
> I don't know that we'd necessarily be *unhappy*, per se, but folks have
> already complained that moz.build files are Turing-complete by virtue of
> being Python.  I don't think we'd want to add bits that now touch the
> filesystem in addition to declaring a bunch of things.
> 
> Putting it in moz.configure would be more appropriate, I think, but that
> means that you'd have to re-run configure to update the list of files, which
> is probably not desirable.  (Then again, you'd have to do the same if you
> walked the filesystem from within moz.build, so....)

Hm so it sounds like there's no easy way to run check_spidermonkey-style when there are js/src changes but not for no-op builds.
(In reply to Jan de Mooij [:jandem] from comment #32)
> (In reply to Nathan Froyd [:froydnj] from comment #31)
> > It does sound a lot better than enumerating all the files at test time,
> > every time!
> > 
> > I don't know that we'd necessarily be *unhappy*, per se, but folks have
> > already complained that moz.build files are Turing-complete by virtue of
> > being Python.  I don't think we'd want to add bits that now touch the
> > filesystem in addition to declaring a bunch of things.
> > 
> > Putting it in moz.configure would be more appropriate, I think, but that
> > means that you'd have to re-run configure to update the list of files, which
> > is probably not desirable.  (Then again, you'd have to do the same if you
> > walked the filesystem from within moz.build, so....)
> 
> Hm so it sounds like there's no easy way to run check_spidermonkey-style
> when there are js/src changes but not for no-op builds.

Just because I am ignorant doesn't mean that there's no way forward. ;)

What we really want is something at the end of the build that says:

1. If we built a target that depended on js/src;
2. If said target was built successfully;
3. If said target needed to build files in js/src (i.e. there were changes); then
4. Run the spidermonkey style checker.

Chris, do we have some way to express that (or that with fewer conditions) in the build system?
Flags: needinfo?(cmanchester)
As a hack we could pretend `check_spidermonkey_style.py` is a `GENERATED_FILES` script and have it claim the js library as a dependency? We should re-link js if we touch a js source, and if that's the case we might want to run the check. We'd run it a little more than necessary (it looks like we don't care about everything that ends up in js), but if the check is very fast (seconds, which it appears to be in automation), that might be fine.

At some point we're going to need to encode more general post-build checks such as `CHECK_STDCXX` outside of make, but I'm not sure when that will happen.
Flags: needinfo?(cmanchester)
Yeah the script is pretty fast these days. I didn't know you could use a library as a dependency - that sounds pretty nice. Let me try something.

Thanks guys!
Flags: needinfo?(jdemooij)
Hm so we would have to add some fake file to GENERATED_FILES? Something like:

  GENERATED_FILES += ['unused.file']
  Foo = GENERATED_FILES['unused.file']
  Foo.script = 'devtools/check_style.py'
  Foo.inputs += ['!js']

It's complicated because in check_style.py how do I find the topsrcdir?

It would be nice if there was a way to say: run this script whenever linking this binary or compiling this file...
Flags: needinfo?(cmanchester)
Yes, something along those lines what what I was thinking, although it might be more like `inputs += ['!/js/src/%sjs.%s' % (config['LIB_PREFIX'], config['LIB_SUFFIX'])]`. You should be able to `import buildconfig` and use `buildconfig.topsrcdir` from within the python script.
Flags: needinfo?(cmanchester)
Attached patch Patch (obsolete) — Splinter Review
Thanks a lot for the help! I verified on Try that this runs the checks as part of the browser and shell builds :)

As a follow-up we could remove these scripts from |make check|, but the scripts run in a few seconds so I don't know if that's necessary.
Assignee: nobody → jdemooij
Attachment #8485208 - Attachment is obsolete: true
Status: REOPENED → ASSIGNED
Flags: needinfo?(jdemooij)
Attachment #8969938 - Flags: review?(cmanchester)
Comment on attachment 8969938 [details] [diff] [review]
Patch

Review of attachment 8969938 [details] [diff] [review]:
-----------------------------------------------------------------

Please also remove this from make check as a part of this change, it will save whoever tries to move that check target from accidentally duplicating this work. It should just be a matter of removing the targets that run these scripts, around https://searchfox.org/mozilla-central/rev/8f06c1b9a080b84435a2906e420fe102e1ed780b/js/src/Makefile.in#52

::: config/run_spidermonkey_checks.py
@@ +16,5 @@
> +
> +def main(output, input_file):
> +    run_script('check_spidermonkey_style.py')
> +    run_script('check_macroassembler_style.py')
> +    run_script('check_js_opcode.py')

If you specify the individual scripts as inputs instead of listing them here the build system will add them as dependencies meaning the checks will run after the scripts change.

::: js/src/build/moz.build
@@ +78,5 @@
> +
> +# Run SpiderMonkey style checker after linking the static library. This avoids
> +# running the script for no-op builds.
> +GENERATED_FILES += ['spidermonkey_checks']
> +RunChecks = GENERATED_FILES['spidermonkey_checks']

In python this would usually be `run_checks`, not `RunChecks`.

@@ +80,5 @@
> +# running the script for no-op builds.
> +GENERATED_FILES += ['spidermonkey_checks']
> +RunChecks = GENERATED_FILES['spidermonkey_checks']
> +RunChecks.script = '/config/run_spidermonkey_checks.py'
> +RunChecks.inputs += ['!/js/src/build/%sjs_static.%s' % (CONFIG['LIB_PREFIX'], CONFIG['LIB_SUFFIX'])]

This can just be "'!%s/js_static.%s' % (CONFIG['LIB_PREFIX'], CONFIG['LIB_SUFFIX'])", and therefore just on a single line.
Attachment #8969938 - Flags: review?(cmanchester)
Attached patch Patch v2 (obsolete) — Splinter Review
Thanks for the great suggestions!
Attachment #8969938 - Attachment is obsolete: true
Attachment #8970454 - Flags: review?(cmanchester)
Attached patch Patch v2Splinter Review
Attachment #8970454 - Attachment is obsolete: true
Attachment #8970454 - Flags: review?(cmanchester)
Attachment #8970455 - Flags: review?(cmanchester)
Comment on attachment 8970455 [details] [diff] [review]
Patch v2

Review of attachment 8970455 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for updating this, it looks good.

I'm not a peer of these js automation bits (autospider.py, nonunified), but the changes looks straightforward, and correct, so I doubt they need extra review.
Attachment #8970455 - Flags: review?(cmanchester) → review+
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ff80b014c00b
Run SpiderMonkey style checkers as part of the build instead of make check. r=chmanchester
https://hg.mozilla.org/mozilla-central/rev/ff80b014c00b
Status: ASSIGNED → RESOLVED
Closed: 5 years ago2 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.