search

mozilla / release-services

Mozilla Release Engineering Services
https://docs.mozilla-releng.net
Mozilla Public License 2.0
49 stars 93 forks source link

shipit_static_analysis: Enable Phabricator reviews in production #1166

Closed jankeromnes closed 6 years ago

jankeromnes commented 6 years ago

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:

jankeromnes commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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:

jankeromnes commented 6 years ago

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 commented 6 years ago

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

jankeromnes commented 6 years ago

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

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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

La0 commented 6 years ago

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 commented 6 years ago

The codespell error is not in the dictionary (depandent)

sylvestre commented 6 years ago

I created a PR https://github.com/codespell-project/codespell/pull/549 for the depandent thing

jankeromnes commented 6 years ago

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 commented 6 years ago

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

jankeromnes commented 6 years ago

Thanks @La0! Please ping me if I can help. :slightly_smiling_face:

La0 commented 6 years ago

The review summary is generated by Phabricator, we only push the same comment as on Mozreview, without any extra infos about issues.

The differential.createcomment is used to publish these comments.

I just looked at the Phabricator source code, and it seems we could specify an action based on the analysis result :

The request_review & reject seem interesting for our use case

La0 commented 6 years ago

I published a test review from my local version fixing the offset lines + bug report link

jankeromnes commented 6 years ago

The request_review & reject seem interesting for our use case

Indeed, thanks!

I published a test review from my local version fixing the offset lines + bug report link

Thanks a lot! The bug report link looks good, but the comments still seem off by one line (e.g. the first eslint: space-infix-ops issue should be on line 7, but the latest comment is still on lines 7 & 8). Maybe we could ping the Conduit folks about this (and they may or may not forward it via our paid Phabricator support account).

EDIT: Oh, wait, my mistake, the "latest comment" I mention is actually an older comment (from the "BastienAbadie" bot, against the latest diff). The actual "latest comment" was posted by the "ReviewBot" bot, against an old diff (so it appears grey instead of orange), and the off-by-one issue is actually fixed there. So, thanks! :tada:

jankeromnes commented 6 years ago

Thanks for merging the fixes!

But this issue is not quite done yet. (I'll close it when we've enabled Phabricator in production.)

La0 commented 6 years ago

Sorry @jankeromnes, I did not want to close this issue, i used refs instead of closes thinking that would do the trick. There is indeed some work left !

jankeromnes commented 6 years ago

No worries! I think GitHub's heuristic triggered on the word "Fixes", followed by an issue number (regardless of the text in between). :stuck_out_tongue:

jankeromnes commented 6 years ago

In the TaskCluster staging secrets, I changed PUBLICATION: from BEFORE_AFTER to IN_PATCH, in order to use what production will use soon.

I then uploaded a new diff to https://phabricator-dev.allizom.org/D324, attempting to re-trigger the bot (with new staging code and IN_PATCH).

However, the new task https://tools.taskcluster.net/groups/AO98K3mTTJOKBLNQBMUyLA instantly failed with: AssertionError: Specify a phabricator XOR mozreview parameters.

EDIT: I'll restart the staging pulse listener, as maybe some code changed and I think you need to restart the dynos for changes to take effect.

EDIT 2: No dice, same error. Will try to fix it.

jankeromnes commented 6 years ago

:warning: It turns out that there are several commits on staging that are not on master: https://github.com/mozilla-releng/services/compare/staging?expand=1

@La0 I would like to trigger a staging run of shipit-static-analysis, but it fails now. Could you please help me run another validation run? (on phabricator, with the exact code that will go to production tomorrow, and the correct staging configuration that also looks like production)

jankeromnes commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 https://github.com/mozilla-releng/services/pull/1238).

jankeromnes commented 6 years ago

Updates:

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 commented 6 years ago

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):

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

@jankeromnes Staging is back up since monday afternoon.

jankeromnes commented 6 years ago

Great! Thanks. I have:

jankeromnes commented 6 years ago

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:

jankeromnes commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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

jankeromnes commented 6 years ago

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

EDIT: Success again!

screenshot-2018-7-5 d324 bug 1387052 - make firefox code worse intentionally r reviewbot

Go for launchrelease!

jankeromnes commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 24e7ffa7b7e43d6adb4c67d1dc3af7b5d0241864 not being in staging/production yet. Proceeding as some comments are better than no comments.

jankeromnes commented 6 years ago

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 commented 6 years ago

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.