Closed Bug 1402302 Opened 3 years ago Closed 3 years ago

[Static Analysis] Change the default checkers that the static-analysis ran through mach has

Categories

(Firefox Build System :: Source Code Analysis, enhancement)

enhancement
Not set

Tracking

(firefox58 fixed)

RESOLVED FIXED
mozilla58
Tracking Status
firefox58 --- fixed

People

(Reporter: andi, Assigned: andi)

References

()

Details

Attachments

(1 file, 2 obsolete files)

The static analysis that is ran through mach must be coherent with the one that is ran by the infrastructure bot during mozreview.
For this we need the default checkers to be in sync.
I think we should move the yaml file in the firefox tree and have releng-service to use it from the m-cc tree.
Also ./mach clang-tidy should use it


In the future, we will probably want to be able to override some settings for some specific components (but later)
(In reply to Sylvestre Ledru [:sylvestre] from comment #1)
> I think we should move the yaml file in the firefox tree and have
> releng-service to use it from the m-cc tree.
> Also ./mach clang-tidy should use it
> 
> 
> In the future, we will probably want to be able to override some settings
> for some specific components (but later)

Yes definitely this is the approach to keep consistency. But until then we can use them hard-coded.
Comment on attachment 8911260 [details]
Bug 1402302 - sync checkers for static-analysis with the mozreview bot.

https://reviewboard.mozilla.org/r/182748/#review188038

::: python/mozbuild/mozbuild/mach_commands.py:2303
(Diff revision 1)
>              return rc
>          args = [self._clang_tidy_path, '-list-checks', '-checks=-*,mozilla-*']
>          return self._run_command_in_objdir(args=args, pass_thru=True)
>  
> +    def _get_checkers(self):
> +        checkers_list = '-*,mozilla-*'

mozilla-* is already defined in the config.yaml

::: tools/clang-tidy/config.yaml:2
(Diff revision 1)
> +
> +target: obj-x86_64-pc-linux-gnu

You should add a comment to explain that it is for releng-service and it is not used by mach
Comment on attachment 8911260 [details]
Bug 1402302 - sync checkers for static-analysis with the mozreview bot.

https://reviewboard.mozilla.org/r/182748/#review188050

::: python/mozbuild/mozbuild/mach_commands.py:2303
(Diff revision 1)
>              return rc
>          args = [self._clang_tidy_path, '-list-checks', '-checks=-*,mozilla-*']
>          return self._run_command_in_objdir(args=args, pass_thru=True)
>  
> +    def _get_checkers(self):
> +        checkers_list = '-*,mozilla-*'

I could have sworn it wasn't in the config.yaml, I definitly need glasses.

::: tools/clang-tidy/config.yaml:2
(Diff revision 1)
> +
> +target: obj-x86_64-pc-linux-gnu

I'm gonna add it in the next patch.
Comment on attachment 8911260 [details]
Bug 1402302 - sync checkers for static-analysis with the mozreview bot.

https://reviewboard.mozilla.org/r/182748/#review190116

::: python/mozbuild/mozbuild/mach_commands.py:2305
(Diff revision 2)
>          return self._run_command_in_objdir(args=args, pass_thru=True)
>  
> +    def _get_checks(self):
> +        checks = '-*'
> +        import yaml
> +        with open(mozpath.join(self.topsrcdir,"tools","clang-tidy", "config.yaml")) as f:

please run flake8 on your changes.
for example, in python, you should have " " after a ","

::: tools/clang-tidy/config.yaml:10
(Diff revision 2)
> +clang_checkers:
> +# modernize-use-auto (controversial, see bug 1371052)
> +# modernize-use-bool-literals (too noisy because of `while (0)` in many macros)
> + - name: -*
> +   publish: no
> + - name: clang-analyzer-deadcode.DeadStores

Please detail why it is "no"
btw, I think this one should be enabled

ditto for dead increment/assignement but this is a different bug

::: tools/clang-tidy/config.yaml:34
(Diff revision 2)
> +   publish: yes
> + - name: modernize-use-equals-delete
> +   publish: yes
> + - name: modernize-use-nullptr
> +   publish: yes
> + - name: modernize-use-override

please explain why "no"
Attachment #8911260 - Flags: review?(sledru)
(In reply to Sylvestre Ledru [:sylvestre] from comment #7)
> Comment on attachment 8911260 [details]
> Bug 1402302 - sync checkers for static-analysis with the mozreview bot.
> 
> https://reviewboard.mozilla.org/r/182748/#review190116
> 
> ::: python/mozbuild/mozbuild/mach_commands.py:2305
> (Diff revision 2)
> >          return self._run_command_in_objdir(args=args, pass_thru=True)
> >  
> > +    def _get_checks(self):
> > +        checks = '-*'
> > +        import yaml
> > +        with open(mozpath.join(self.topsrcdir,"tools","clang-tidy", "config.yaml")) as f:
> 
> please run flake8 on your changes.
> for example, in python, you should have " " after a ","
> 
Thanks for this!

> ::: tools/clang-tidy/config.yaml:10
> (Diff revision 2)
> > +clang_checkers:
> > +# modernize-use-auto (controversial, see bug 1371052)
> > +# modernize-use-bool-literals (too noisy because of `while (0)` in many macros)
> > + - name: -*
> > +   publish: no
> > + - name: clang-analyzer-deadcode.DeadStores
> 
> Please detail why it is "no"
> btw, I think this one should be enabled
> 
> ditto for dead increment/assignement but this is a different bug
>
Completely agree with you.
 
> ::: tools/clang-tidy/config.yaml:34
> (Diff revision 2)
> > +   publish: yes
> > + - name: modernize-use-equals-delete
> > +   publish: yes
> > + - name: modernize-use-nullptr
> > +   publish: yes
> > + - name: modernize-use-override
> 
> please explain why "no"
Indeed this should be set to yes, I don't see any reason why this should be omitted. 

I didn't check the file after I pulled it from the GitHub repo of releng. I took it ad it was.
Attachment #8911260 - Attachment is obsolete: true
Comment on attachment 8913687 [details]
Bug 1402302 - sync checkers for static-analysis with the mozreview bot.

https://reviewboard.mozilla.org/r/185092/#review190160

::: tools/clang-tidy/config.yaml:6
(Diff revision 1)
> +target: obj-x86_64-pc-linux-gnu
> +# It is used by 'mach static-analysis' and 'mozreview static-analysis bot'
> +# in order to have consistency across the used checkers.
> +# All the clang checks used by the static-analysis tools.
> +clang_checkers:
> +# modernize-use-auto (controversial, see bug 1371052)

why don't you move them next to the checkers?
Attachment #8913687 - Flags: review?(sledru) → review+
Pushed by bpostelnicu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/048af66f7711
sync checkers for static-analysis with the mozreview bot. r=sylvestre
Backout by kwierso@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/810eae249662
Backed out changeset 048af66f7711 for lint issues in config.yaml a=backout
(In reply to Wes Kocher (:KWierso) from comment #13)
> Backed out for linting issues in
> https://treeherder.mozilla.org/logviewer.html#?job_id=134140352&repo=autoland
This was unexpected, according to our yamllint rules, the bool should be explicitly qualified, see http://yamllint.readthedocs.io/en/latest/rules.html#module-yamllint.rules.truthy, or use strings.
Flags: needinfo?(bpostelnicu)
Attachment #8913687 - Attachment is obsolete: true
Attached patch Bug 1402302.patch (obsolete) — Splinter Review
Sorry, but I had to re-update the patch like this since reviewboard didn't let me update the patch on an already submitted revision.
Attachment #8913948 - Flags: review?(sledru)
The only difference of this patch is the adder of the qualification of the type of the bool by adding: 
>>!!bool
You have to reopen the review to be able to update it
Attachment #8913948 - Attachment is obsolete: true
Attachment #8913948 - Flags: review?(sledru)
Attachment #8913687 - Attachment is obsolete: false
(In reply to Sylvestre Ledru [:sylvestre] from comment #18)
> You have to reopen the review to be able to update it

Thanks, done!
Pushed by bpostelnicu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/57692300acee
sync checkers for static-analysis with the mozreview bot. r=sylvestre
https://hg.mozilla.org/mozilla-central/rev/57692300acee
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Depends on: 1420366
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.