shipit_static_analysis: Enable Phabricator reviews in production

[jankeromnes]

Our automated review bot is able to analyze Phabricator code reviews.

We should evaluate its results on staging, and once satisfied we should enable it in production (and evaluate it there as well).

TODO:

• Ensure that our bot detects new Phabricator code reviews (on staging and production)
• Ensure that our bot publishes automated reviews when it finds issues
• Validate published reviews on staging (iterate if adjustments are needed)
  • Ensure that clang-tidy and eslint issues work as well
• Enable in production
• Confirm that published reviews still look good/valuable in production
  • Ensure that published reviews including C/C++ defects also look good/valuable (once #1263 is fixed)
• As a drive-by, also validate comments on MozReview again, just in case

[jankeromnes]

I published an offending patch to staging at https://phabricator-dev.allizom.org/D324

Nothing happened. Further troubleshooting is needed to discover if the bot actually detected this new code review or not.

Also, I've never received an email report where the subject includes "New Static Analysis Phabricator", so I suspect that either (1) Phabricator code reviews are never detected by our bot, or (2) some error happens before an email report can be sent.

We could try to run the bot locally to see if there are errors during analysis, but I don't know how to troubleshoot if the hook works.

[marco-c]

We could try to run the bot locally to see if there are errors during analysis, but I don't know how to troubleshoot if the hook works.

We can look at the Heroku logs for shipit-staging-pulse-listener, it logs when it starts tasks from Phabricator or MozReview, and then we can see the logs for the task on Taskcluster.

[jankeromnes]

So, I ran the bot manually against the staging patch using the following commands:

$ ./please shell shipit-static-analysis --taskcluster-client-id="mozilla-auth0/ad|Mozilla-LDAP|jkeromnes/static-analysis-dev" --taskcluster-access-token="$TOKEN"
[nix-shell:/app/src/shipit_static_analysis]$ export PHABRICATOR="324:PHID-DIFF-y3axo2xumikkf5chiivb"
[nix-shell:/app/src/shipit_static_analysis]$ mkdir -p /app/tmp
[nix-shell:/app/src/shipit_static_analysis]$ shipit-static-analysis --taskcluster-secret=repo:github.com/mozilla-releng/services:branch:staging --cache-root=/app/tmp

It successfully published a review to https://phabricator-dev.allizom.org/D324, so we can conclude that the bot is able to submit reviews.

However, since it didn't do that automatically, I suspect that the Phabricator hook doesn't trigger on staging (maybe that's specific to staging, or our hook is broken; I'm not sure).

Additionally, you can see in the review that only clang-format and flake8 issues were published. No eslint and no clang-tidy issues were detected, which looks like a big regression. I'll update our top comment so that we keep track of this.

[jankeromnes]

With @marco-c we spied on the Phabricator staging hook to see if it actually triggers if I send a rebased patch up for review.

A task does get triggered, but somehow it says the revision is 622:PHID-DIFF-i4b753ptojloi2tjwm2z while we expect it to start with 324 (there is no such https://phabricator-dev.allizom.org/D622 because 622 is a "diff ID" and not a "revision ID").

There seems to be a confusion between the latest "diff ID" 622 (there can be many diffs per revision) and the "revision ID" 324 (found in the URL).

This causes analysis to succeed (it only uses the PHID) but then posting the comments fail, because there is no revision 622:

Logs: https://gzkxjriaaaawhe5jggl2nxqakxlyo6aikvl6cjfyuftrgqvv.taskcluster-worker.net:32792/log/HWdDR_LASSKtlQ6_17H0iw

We can either:

• Fix shipit-pulse-listener to give a "revision ID" (324) instead of the latest "diff ID" (622)
• Fix shipit-static-analysis to take in an actual "diff ID" instead of a "revision ID" as expected today

[jankeromnes]

So, running shipit-static-analysis a second time with:

export PHABRICATOR="324:PHID-DIFF-i4b753ptojloi2tjwm2z"
shipit-static-analysis --taskcluster-secret=repo:github.com/mozilla-releng/services:branch:staging --cache-root=/app/tmp

Successfully published new comments against the latest diff. I conclude that shipit-static-analysis doesn't need the "diff ID" (622), but just the "revision ID" (324).

Let's fix shipit-pulse-listener.

[jankeromnes]

The phabricator hook parameter issue should be fixed by 3b24784. Hopefully auto-reviews will now work on staging, allowing us to move on to review validation.

[jankeromnes]

Now automatically published reviews on staging! 🎉 Thanks @marco-c and @garbas for the help, and especially @La0 for writing all the code! 😛

Latest review on https://phabricator-dev.allizom.org/D324 was automatically submitted by the bot after I pushed a new revision, and you should have received an email titled:

[staging] New Static Analysis Phabricator #645 - PHID-DIFF-y5cvex4pzvr6g4epqf3g

Edit: FYI successful triggered task was https://tools.taskcluster.net/groups/Zl9BpvZYR0GbkOobShA1pg/tasks/Zl9BpvZYR0GbkOobShA1pg/runs/0/logs/public%2Flogs%2Flive.log

[jankeromnes]

Ok, in the latest review (which was automated), we appear to have eslint comments now, but still no clang-tidy.

The staging configuration looks like this:

shipit-static-analysis:
  [...]
  PUBLICATION: BEFORE_AFTER
  [...]
  ANALYZERS:
    - clang-tidy
    - clang-format
    - mozlint

From the logs, it looks like clang-tidy did run, but it's hard to tell exactly what happened (partly because clang-format adds a lot of noise).

Relevant log excerpts:

shipit_static_analysis.workflow: Run ClangTidy
shipit_static_analysis.clang.tidy: Available clang-tidy checks:
	
	[... a lot of checkers]

shipit_static_analysis.clang.tidy: Running static-analysis (cmd='gecko-env ./mach --log-no-times static-analysis check --header-filter=scroll.js|.arcconfig|prep_cif.c|menus.js|jsapi.cpp|gen_event_data.py|definitions.js --checks=-*,misc-forward-declaration-namespace,clang-analyzer-deadcode.DeadStores,clang-analyzer-security.FloatLoopCounter,clang-analyzer-security.insecureAPI.UncheckedReturn,clang-analyzer-security.insecureAPI.getpw,clang-analyzer-security.insecureAPI.mkstemp,clang-analyzer-security.insecureAPI.mktemp,clang-analyzer-security.insecureAPI.rand,clang-analyzer-security.insecureAPI.strcpy,clang-analyzer-security.insecureAPI.vfork,misc-argument-comment,misc-assert-side-effect,misc-suspicious-missing-comma,misc-suspicious-semicolon,misc-unused-using-decls,modernize-avoid-bind,modernize-loop-convert,modernize-raw-string-literal,modernize-redundant-void-arg,modernize-shrink-to-fit,modernize-use-equals-default,modernize-use-equals-delete,modernize-use-nullptr,modernize-use-override,mozilla-*,performance-for-range-copy,performance-inefficient-string-concatenation,performance-inefficient-vector-operation,performance-type-promotion-in-math-fn,performance-unnecessary-copy-initialization,performance-unnecessary-value-param,readability-container-size-empty,readability-else-after-return,readability-misleading-indentation,readability-redundant-control-flow,readability-redundant-smartptr-get,readability-redundant-string-cstr,readability-redundant-string-init,readability-uniqueptr-delete-release,modernize-use-auto,modernize-use-bool-literals devtools/client/shared/scroll.js .arcconfig js/src/ctypes/libffi/src/prep_cif.c devtools/client/menus.js js/src/jsapi.cpp toolkit/components/telemetry/gen_event_data.py devtools/client/definitions.js')
+ exec ./mach --log-no-times static-analysis check '--header-filter=scroll.js|.arcconfig|prep_cif.c|menus.js|jsapi.cpp|gen_event_data.py|definitions.js' '--checks=-*,misc-forward-declaration-namespace,clang-analyzer-deadcode.DeadStores,clang-analyzer-security.FloatLoopCounter,clang-analyzer-security.insecureAPI.UncheckedReturn,clang-analyzer-security.insecureAPI.getpw,clang-analyzer-security.insecureAPI.mkstemp,clang-analyzer-security.insecureAPI.mktemp,clang-analyzer-security.insecureAPI.rand,clang-analyzer-security.insecureAPI.strcpy,clang-analyzer-security.insecureAPI.vfork,misc-argument-comment,misc-assert-side-effect,misc-suspicious-missing-comma,misc-suspicious-semicolon,misc-unused-using-decls,modernize-avoid-bind,modernize-loop-convert,modernize-raw-string-literal,modernize-redundant-void-arg,modernize-shrink-to-fit,modernize-use-equals-default,modernize-use-equals-delete,modernize-use-nullptr,modernize-use-override,mozilla-*,performance-for-range-copy,performance-inefficient-string-concatenation,performance-inefficient-vector-operation,performance-type-promotion-in-math-fn,performance-unnecessary-copy-initialization,performance-unnecessary-value-param,readability-container-size-empty,readability-else-after-return,readability-misleading-indentation,readability-redundant-control-flow,readability-redundant-smartptr-get,readability-redundant-string-cstr,readability-redundant-string-init,readability-uniqueptr-delete-release,modernize-use-auto,modernize-use-bool-literals' devtools/client/shared/scroll.js .arcconfig js/src/ctypes/libffi/src/prep_cif.c devtools/client/menus.js js/src/jsapi.cpp toolkit/components/telemetry/gen_event_data.py devtools/client/definitions.js

I'm not sure what happened, as all analysis steps seem to be running in random order, I guess before and after the patch.

Maybe the problem is related to the BEFORE_AFTER behavior? I'll try to dig further in the email report soon, but I'm afraid that it is truncated because there are way too many clang-format comments.

Maybe we could temporarily disable clang-format on staging to better understand what's happening?

[La0]

By using #1209, i now get the following issues:

[warning] readability-else-after-return js/src/ctypes/libffi/src/prep_cif.c 88:3
[error] mozilla-dangling-on-temporary js/src/jsapi.cpp 131:39
[error] mozilla-dangling-on-temporary js/src/jsapi.cpp 132:38
[warning] modernize-redundant-void-arg js/src/jsapi.cpp 243:42
[warning] modernize-redundant-void-arg js/src/jsapi.cpp 647:29
[warning] modernize-raw-string-literal js/src/jsapi.cpp 1770:32
[warning] modernize-use-auto js/src/jsapi.cpp 3277:5
[warning] modernize-use-equals-default js/src/jsapi.cpp 3903:5
[warning] modernize-use-equals-default js/src/jsapi.cpp 3906:5
[warning] modernize-use-equals-default js/src/jsapi.cpp 5819:35
[warning] modernize-use-equals-default js/src/jsapi.cpp 7076:15
[warning] modernize-use-equals-default js/src/jsapi.cpp 7080:15
[warning] modernize-redundant-void-arg js/src/jsapi.cpp 7908:15
eslint issue error devtools/client/shared/scroll.js line 7
eslint issue error devtools/client/menus.js line 31
eslint issue error devtools/client/menus.js line 41
eslint issue error devtools/client/definitions.js line 7
flake8 issue error toolkit/components/telemetry/gen_event_data.py line 37
flake8 issue error toolkit/components/telemetry/gen_event_data.py line 37
flake8 issue error toolkit/components/telemetry/gen_event_data.py line 37
flake8 issue error toolkit/components/telemetry/gen_event_data.py line 57

[jankeromnes]

So, our bot seems to catch all the issues again (on staging), which is great.

However, yesterday's push to Phabricator staging didn't get any review comments.

I attempted another push today, watching the logs. I did find a bug with the email report (#1210), but his time the Phabricator comments did work. Maybe it's a race condition, but I guess we'll have to try again a few times to make sure it works consistently.

[jankeromnes]

Now testing on a new phabricator-dev revision: https://phabricator-dev.allizom.org/D431

Triggered analysis task: https://tools.taskcluster.net/groups/DFq61B7ISwaYdCYqUc6QCw/

Last try didn't post any clang-tidy comments. Let's see if this one does.

EDIT: It worked!

[jankeromnes]

I have a few nits about the automated review that was published to D431 (using the BEFORE_AFTER heuristic):

• the bug report link is wrong (the template requests a mozreview URL)
• off-by-one error on the lines a review comment is attached to (e.g. for a defect on line 7, the comment is attached to lines 7+8, placing one line too low)
• the review summary includes all the comments that are already displayed inline (not sure if this is a Phabricator thing or our bot duplicating all comments)
• the bot missed 1/9 mozlint defects:
  • codespell (the fake word "depandent" isn't actually a frequent typo, so it's not in the dictionary)
• the bot missed 10/12 clang-tidy defects (that's 17% hit rate, which is bad) (that's bug 1472721):
  • readability-else-after-return (supposedly enabled)
  • misc-suspicious-missing-comma (supposedly enabled)
  • readability-misleading-indentation (supposedly enabled)
  • clang-analyzer-security.FloatLoopCounter (supposedly enabled)
  • clang-analyzer-security.insecureAPI.UncheckedReturn (supposedly enabled)
  • clang-analyzer-security.insecureAPI.gets (not enabled?!) (deprecated)
  • misc-suspicious-semicolon (supposedly enabled)
  • modernize-redundant-void-arg (supposedly enabled)
  • clang-analyzer-deadcode.DeadStores (supposedly enabled)
  • performance-faster-string-find (not enabled?!) (enabled in bug 1468811)
  • performance-implicit-conversion-in-loop (not enabled?!) (not in 5.0, only in 6.0)
• the bot reported 6 extra defects that were not in the patch (pre-existing issues, should not be reported) (that's a BEFORE_AFTER bug, see #1236):
  • modernize-raw-string-literal
  • modernize-use-equals-default * 5

[La0]

10 issues were found before applying the patch, as we are running with new BEFORE_AFTER mode on staging:

Mach static analysis found some issues (nb=10)
Skipping clang-diagnostic-error: [error] clang-diagnostic-error js/src/ctypes/libffi/include/ffi_common.h 23:12
Found 3rd party code issue [warning] readability-else-after-return js/src/ctypes/libffi/src/prep_cif.c 88:3
Found in-tree code issue [warning] modernize-redundant-void-arg js/src/jsapi.cpp 608:29
Found in-tree code issue [warning] modernize-raw-string-literal js/src/jsapi.cpp 1675:32
Found in-tree code issue [warning] modernize-use-auto js/src/jsapi.cpp 3190:5
Found in-tree code issue [warning] modernize-use-equals-default js/src/jsapi.cpp 3816:5
Found in-tree code issue [warning] modernize-use-equals-default js/src/jsapi.cpp 3819:5
Found in-tree code issue [warning] modernize-use-equals-default js/src/jsapi.cpp 5732:35
Found in-tree code issue [warning] modernize-use-equals-default js/src/jsapi.cpp 6989:15
Found in-tree code issue [warning] modernize-use-equals-default js/src/jsapi.cpp 6993:15
Found in-tree code issue [warning] modernize-redundant-void-arg js/src/jsapi.cpp 7821:15
Skipping clang-diagnostic-error: [error] clang-diagnostic-error obj-x86_64-pc-linux-gnu/dist/system_wrappers/cstddef 3:15

[La0]

The codespell error is not in the dictionary (depandent)

[sylvestre]

I created a PR codespell-project/codespell#549 for the depandent thing

[jankeromnes]

Thanks! But I just made it up, because I didn't know there was a misspelling dictionary (I thought it was Levenstein distance against an actual dictionary).

I don't think that "depandent" is a typo that people could actually make by mistake.

[La0]

I'll be working on the other issues this week, i'll update this ticket when i get some code

[jankeromnes]

I've force-pushed master to staging again, and I'm waiting for shipit-static-analysis to finish building before triggering another Phabricator analysis.

[jankeromnes]

shipit-static-analysis and shipit-pulse-listener were successfully built and deployed to staging, and I triggered another analysis.

This time the task went on longer, but it failed in the first ./mach configure with:

 0:53.84 Reticulating splines...
 0:54.52 Traceback (most recent call last):
 0:54.52   File "/cache/sa-central/configure.py", line 123, in <module>
 0:54.52     sys.exit(main(sys.argv))
 0:54.52   File "/cache/sa-central/configure.py", line 34, in main
 0:54.52     return config_status(config)
 0:54.52   File "/cache/sa-central/configure.py", line 118, in config_status
 0:54.52     return config_status(args=[], **encode(sanitized_config, encoding))
 0:54.52   File "/cache/sa-central/python/mozbuild/mozbuild/config_status.py", line 143, in config_status
 0:54.52     definitions = list(definitions)
 0:54.52   File "/cache/sa-central/python/mozbuild/mozbuild/frontend/emitter.py", line 171, in emit
 0:54.52     for out in output:
 0:54.52   File "/cache/sa-central/python/mozbuild/mozbuild/frontend/reader.py", line 880, in read_topsrcdir
 0:54.52     for r in self.read_mozbuild(path, self.config):
 0:54.52   File "/cache/sa-central/python/mozbuild/mozbuild/frontend/reader.py", line 1047, in read_mozbuild
 0:54.52     raise bre
 0:54.52 mozbuild.frontend.reader.BuildReaderError:
 0:54.52 ==============================
 0:54.52 FATAL ERROR PROCESSING MOZBUILD FILE
 0:54.52 ==============================
 0:54.52 
 0:54.52 The error occurred while processing the following file:
 0:54.52 
 0:54.52     /cache/sa-central/toolkit/library/rust/gkrust-features.mozbuild
 0:54.52 
 0:54.52 This file was included as part of processing:
 0:54.52 
 0:54.52     /cache/sa-central/toolkit/library/gtest/rust/moz.build
 0:54.52 
 0:54.52 A moz.build file called the error() function.
 0:54.52 
 0:54.52 The error it encountered is:
 0:54.52 
 0:54.52     Builds on automation must use a version of rust that supports OOM hooking
 0:54.52 
 0:54.52 Correct the error condition and try again.
 0:54.52 
 1:06.46 *** Fix above errors and then restart with\
 1:06.46                "/nix/store/lhp5rw0dagi5mgqwr9i3x41240ba4ypz-gnumake-4.2.1/bin/make -f client.mk build"
 1:06.46 make: *** [client.mk:127: configure] Error 1

[marco-c]

For historical purposes: the problem was that we were using a version of Rust (1.27.0) which is not supported for automation on mozilla-central. We were not pinning Rust, so we inadvertently updated from 1.26.2 to 1.27.0 (this is now fixed by #1238).

[jankeromnes]

Updates:

• I worked around this error by simply removing the automation && Rust !== 1.27 assert.
• New analysis/staging task is in progress

EDIT: My workaround failed, because even with the IN_PATCH algorithm, shipit-static-analysis runs ./mach configure before applying the patch, not after. So I'm giving up on the workaround. I pushed the #1238 fix to staging, and now I'm waiting for a full redeployment.

[jankeromnes]

Thanks to Bastien's fix, I was finally able to run another analysis on staging (https://tools.taskcluster.net/groups/Y0GiPYNEQHeSwRZ4x0RFoA/), and the bot posted a new review to https://phabricator-dev.allizom.org/D324.

From what I see, it reveals the following problems (with the IN_PATCH heuristic):

• some comments are malformed (e.g. they include spurious output after the issue body, e.g. some unrelated warning, which looks like a clang-tidy parsing error, filed as #1251) (fixed in #1254)
• the bot missed 10/12 clang-tidy defects (basically it missed all clang-tidy defects except the mozilla-* ones) (it's a ./mach static-anlaysis check bug, so not related to our bot):
  • readability-else-after-return (supposedly enabled) (js/src/ctypes/libffi/src/prep_cif.c is now considered Third Party)
  • misc-suspicious-missing-comma (supposedly enabled) (the array needs to have >= 5 items)
  • readability-misleading-indentation (supposedly enabled)
  • clang-analyzer-security.FloatLoopCounter (supposedly enabled)
  • clang-analyzer-security.insecureAPI.UncheckedReturn (supposedly enabled)
  • misc-suspicious-semicolon (supposedly enabled)
  • modernize-redundant-void-arg (supposedly enabled) (is configured as publish: !!bool no )
  • clang-analyzer-deadcode.DeadStores (supposedly enabled)
  • performance-faster-string-find (supposedly enabled now)
• the bot missed 2/9 mozlint defects
  • eslint: strict (didn't reproduce locally, probably a one-off bug, but we'll validate once more to be sure)
  • codespell ("maching") (js/src/ctypes/libffi/src/prep_cif.c is still Third Party)
• this time, the bot didn't report the 6 pre-existing defects that were not in the patch, so I conclude that it was a BEFORE_AFTER bug (i.e. #1236)

Here is a snippet from the task's debug log:

shipit_static_analysis.report.debug: Debug revision (rev='Phabricator #686 - PHID-DIFF-ocahda37wlvf4mcnyt7n')
shipit_static_analysis.report.debug: Issue silent (issue='[warning] readability-else-after-return js/src/ctypes/libffi/src/prep_cif.c 88:3')
shipit_static_analysis.report.debug: Issue publishable (issue='[error] mozilla-dangling-on-temporary js/src/jsapi.cpp 132:39')
shipit_static_analysis.report.debug: Issue publishable (issue='[error] mozilla-dangling-on-temporary js/src/jsapi.cpp 133:38')
shipit_static_analysis.report.debug: Issue silent (issue='[warning] modernize-redundant-void-arg js/src/jsapi.cpp 244:42')
shipit_static_analysis.report.debug: Issue silent (issue='[warning] modernize-redundant-void-arg js/src/jsapi.cpp 648:29')
shipit_static_analysis.report.debug: Issue silent (issue='[warning] modernize-raw-string-literal js/src/jsapi.cpp 1715:32')
shipit_static_analysis.report.debug: Issue silent (issue='[warning] modernize-use-auto js/src/jsapi.cpp 3204:5')
shipit_static_analysis.report.debug: Issue silent (issue='[warning] modernize-use-equals-default js/src/jsapi.cpp 3830:5')
shipit_static_analysis.report.debug: Issue silent (issue='[warning] modernize-use-equals-default js/src/jsapi.cpp 3833:5')
shipit_static_analysis.report.debug: Issue silent (issue='[warning] modernize-use-equals-default js/src/jsapi.cpp 5746:35')
shipit_static_analysis.report.debug: Issue silent (issue='[warning] modernize-use-equals-default js/src/jsapi.cpp 7006:15')
shipit_static_analysis.report.debug: Issue silent (issue='[warning] modernize-use-equals-default js/src/jsapi.cpp 7010:15')
shipit_static_analysis.report.debug: Issue silent (issue='[warning] modernize-redundant-void-arg js/src/jsapi.cpp 7838:15')
shipit_static_analysis.report.debug: Issue silent (issue='eslint issue error devtools/client/menus.js line 31')
shipit_static_analysis.report.debug: Issue publishable (issue='eslint issue error devtools/client/menus.js line 42')
shipit_static_analysis.report.debug: Issue publishable (issue='flake8 issue error toolkit/components/telemetry/gen_event_data.py line 37')
shipit_static_analysis.report.debug: Issue publishable (issue='flake8 issue error toolkit/components/telemetry/gen_event_data.py line 37')
shipit_static_analysis.report.debug: Issue publishable (issue='flake8 issue error toolkit/components/telemetry/gen_event_data.py line 37')
shipit_static_analysis.report.debug: Issue publishable (issue='flake8 issue error toolkit/components/telemetry/gen_event_data.py line 57')
shipit_static_analysis.report.debug: Issue publishable (issue='eslint issue error devtools/client/shared/scroll.js line 7')
shipit_static_analysis.report.debug: Issue publishable (issue='eslint issue error devtools/client/definitions.js line 7')

[abpostelnicu]

Some comments regarding 'misc-suspicious-missing-comma'.
It didn't work because the initialiser list is < 5.
This can be seen here.
if the list would have contained another field, this would have worked, of course this can customised by having a .clang-tidy file in the root but in the mean time let's use an official example from here.

[jankeromnes]

Thanks a lot @abpostelnicu for figuring out the threshold! I've updated the list of undetected checks.

Still, the checks that are not strikethrough should have reported defects in the patch. I'm not sure why the bot only reported mozilla-* defects.

[jankeromnes]

I've updated our various checklists in this bug. Looking good! Thanks everyone for the hard work.

Now we only need to run analysis on staging one more time (I expect it to be the last), and then move on to validating production (which should take much less time hopefully).

@La0 please ping me when staging is un-broken again, so I can re-trigger an analysis.

[abpostelnicu]

There were sevaral issues why the bot only reported on mozilla checks:

1. the code was broken, it didn't compile so the generated ast was broken.
2. the logic behind the header filter was flawed, this was fixed here: https://hg.mozilla.org/mozilla-central/rev/5049afd1f26a

[La0]

@jankeromnes Staging is back up since monday afternoon.

[jankeromnes]

Great! Thanks. I have:

• updated the staging branch to latest master
• restarted the staging-pulse-listener
• rebased my "bad patch" on top of Andi's fix: https://phabricator-dev.allizom.org/D324
• which triggered this analysis task: https://tools.taskcluster.net/groups/dEVGF9o_TsyZDUmyMkcikg/

[jankeromnes]

And the results are in! We even have two reviews for the price of one on https://phabricator-dev.allizom.org/D324 (I suspect that testing is now also analyzing Phabricator-dev patches).

The following problems still need to be fixed:

• the bot missed 6/10 clang-tidy defects (40% success, see #1256) (fixed in #1258)
  • it missed readability-else-after-return in js/src/jsapi.cpp (found locally)
  • it missed clang-analyzer-security.FloatLoopCounter in js/src/jsapi.cpp (found locally)
  • it missed clang-analyzer-security.insecureAPI.UncheckedReturn in js/src/jsapi.cpp (found locally)
  • it missed misc-suspicious-semicolon in js/src/jsapi.cpp (found locally)
  • it missed modernize-redundant-void-arg in js/src/jsapi.cpp (publish: no)
  • it missed clang-analyzer-deadcode.DeadStores in js/src/jsapi.cpp (found locally)
  • it missed performance-faster-string-find in js/src/jsapi.cpp (found locally)
  • it missed performance-implicit-conversion-in-loop in js/src/jsapi.cpp (not in clang-tidy 5.0 but in 6.0)
• the bot caught all 7 mozlint defect (100% success)
  • it missed eslint: strict in devtools/client/menus.js, but a local ./mach lint finds it (it's reported on an unmodified line, so it's not in_patch)
  • it missed codespell: destributed in js/ductwork/debugger/JSDebugger.cpp (a local codespell doesn't find it either for some reason)

[jankeromnes]

When running ./mach static-analysis check locally, I do find the missing clang-tidy issues. However, the bot doesn't even seem to detect them (they don't appear in the logs or in the email report). Maybe another output parsing issue? It would be great to see the raw clang-tidy output.

Additionally, the eslint: strict defect in devtools/client/menus.js was detected by the bot, but for some reason the bot considers it Publishable: no (see email report):

mozlint - eslint
    Path: devtools/client/menus.js
    Level: error
    Line: 31
    Third Party: no
    Disabled rule: no
    Publishable: no
    Is new: no

Use the global form of 'use strict'.

[jankeromnes]

Update: The missing clang-tidy defects don't seem to be a parsing issue, as revealed by dumping raw clang-tidy outputs. I filed issue #1256 about this, and may have found a relevant difference between the in-services and out-of-services run logs.

[jankeromnes]

Looks like @La0 was able to fix all undetected clang-tidy defects by adding the right dependencies to our Nix env!

With that, the eslint: strict remains the last issue to fix in order to fully validate analysis and enable Phabricator in production.

What's weird is that lint.py says A mozlint issues is publishable when: * file is not 3rd party * rule is not disabled. In the email report, we can see that the eslint: strict defect is neither Third party nor Disabled rule, yet it still shows Publishable: no.

[jankeromnes]

Aha, the analysis' report.json indicates that the eslint: strict issue is not in_patch, which is true because it's reported on the first code line after the commented-out "use strict", but that line was not modified by the patch.

Hopefully this will be fixed with BEFORE_AFTER soon, but we don't need to block our Phabricator release on it.

[jankeromnes]

The fix from #1258 was deployed to staging, and I've sent another bad patch to D324.

Analysis is running: https://tools.taskcluster.net/groups/E3LiY0W6Q72GNJgUdrrX_w/

[jankeromnes]

10/10 clang-tidy defects, 7/7 mozlint defects, 100% success! https://irccloud.mozilla.com/pastebin/goicWMJC/defects.txt

[jankeromnes]

New staging analysis, just in case: https://tools.taskcluster.net/groups/Hpi4TqYKQJaGqugH6yvIgQ

EDIT: Success again!

Go for launchrelease!

[jankeromnes]

Update: I've now enabled automated Phabricator reviews in production, and reviews look good!

https://phabricator.services.mozilla.com/D2066

However, both MozReview and Phabricator reviews are currently broken for patches that touch C/C++ files (because of #1263). Once that issue is fixed, we should also validate that clang-tidy comments still look good.

Additionally, I've had to disable clang-format globally in production (also in debug email reports), because the Phabricator reporter doesn't have a filter yet like MozReview. I've filed issue #1264 to address this.

[jankeromnes]

10/10 and 7/7 on staging again! https://tools.taskcluster.net/groups/ZnCYTYy2QIqzyDnNHPiqWw/

This confirms that #1263's fix worked, and we can deploy it to production.

Additionally, we can see the analyzer raw dumps in the tasks artifacts, which is great (except clang-format.txt, maybe because clang-format is currently disabled on staging, although we do have a clang-format.diff).

[jankeromnes]

While waiting for general release next week, I'm trying to deploy a hot-fix for the Rust issue.

I've deployed the hot-fix to staging, and re-validated the bot: https://phabricator-dev.allizom.org/D541

Results: 4/10 expected clang-tidy defects and 7/7 expected mozlint defects.

EDIT: The missing clang-tidy checks are probably due to 24e7ffa not being in staging/production yet. Proceeding as some comments are better than no comments.

[jankeromnes]

I've deployed a hot-fix to shipit-static-analysis-production with 2 fixes from master, and:

10/10 7/7 in production! 🎉🎉🎉 https://phabricator.services.mozilla.com/D2120

(And deploying on Friday the 13th wasn't so bad... 😝)

[jankeromnes]

I've tried validating MozReview the same way, but the analysis task failed with UnicodeDecodeError: 'utf-8' codec can't decode byte 0xb5 in position 58204297: invalid start byte:

https://tools.taskcluster.net/groups/Lp8DSs3xTHKPou13q85gUA/

I'll try to reproduce this bug locally and then bisect it.

[jankeromnes]

Hm, locally it worked: Production validated 10/10 7/7 on MozReview as well: https://reviewboard.mozilla.org/r/257216/#review264084

That intermittent UnicodeDecodeError is worrisome though, we should keep an eye on it.

[jankeromnes]

reviewbot was enabled on Phabricator last Friday, and it works great! 🎉