1328454 - Enable running static analysis checks locally through mach

Status: RESOLVED FIXED

(Developer Infrastructure :: Source Code Analysis, defect)

Description

[(no longer active)]

I'm planning to use the clang-tidy builds that we are generating on TaskCluster to enable a mach command that allows developers to run our static analysis checks locally, without needing to switch to clang and/or change their development environment.

The rough plan is to enable all of our clang-tidy builds on the infra, registering the artifacts from them in the TaskCluster index, use the index to download the latest clang-tidy for the developer's platform, and run it locally for them.

Comment 1

[(no longer active)]

Attachment: Add a mach static-analysis command (WIP)

Michael, can you please try this out and see how well it works for you, if at all?  Thanks!

Comment 2

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 8824526 [details] [diff] [review]
Add a mach static-analysis command (WIP)

Here's what I did:

Clone of M-C with --enable-clang-plugin mozconfig as you suggested.
Applied this patch, and changed the tree from 'try' to 'mozilla-inbound' (as you suggested).
Ran `machclang static-analysis install` which failed, complaining I had not configured yet.
Ran `machclang configure` to configure.
Ran `machclang static-analysis install`.
Ran `machclang static-analysis check`.

The check ran and exited with a 0 status code. However, there were quite a few files which were skipped (e.g.: Skipping /Volumes/Devel/Code/mozilla/mozilla-central/obj-clang-x86_64-apple-darwin16.3.0/UnifiedProtocols26.cpp. Compile command not found.) and one error was generated which didn't seem to change the exit code or fail the static analysis run, and I only found by grepping through the output:

1 error generated.
Error while processing /Volumes/Devel/Code/mozilla/mozilla-central/obj-clang-x86_64-apple-darwin16.3.0/security/nss/lib/ckfw/builtins/builtins_nssckbi/certdata.c.
error: error reading '/Volumes/Devel/Code/mozilla/mozilla-central/obj-clang-x86_64-apple-darwin16.3.0/security/nss/lib/ckfw/builtins/builtins_nssckbi/certdata.c' [clang-diagnostic-error]

I would suggest changing the output from the command to generate considerably less clutter (I don't think the command should be printed unless it fails, and a simple percentage readout seems like it might be more practical).

In addition, it would be nice I would think if clang static-analysis install configured your tree for you. Also, as we talked about before, this should ideally run successfully on a tree which doesn't have --enable-clang-plugin.

Comment 3

[Andi [:andi]]

I can help here since this is very similar to what i want to do, i want to use clang-tidy from the command line in order to migrate code to C++11.

So my goal is to integrate in mach the usage of something like:

>> ./mach migrateC11 path_to_directory/

And for this i was thinking of using the following checkers:

modernize-loop-convert
modernize-use-auto
modernize-use-default
modernize-raw-string-literal
modernize-use-bool-literals
modernize-use-override

I think this would be very helpful for the engineers after before they want to publish a patch to run this suit of test in order to better align our code with the current standards.

Comment 4

[(no longer active)]

(In reply to Michael Layzell [:mystor] from comment #2)
> Clone of M-C with --enable-clang-plugin mozconfig as you suggested.
> Applied this patch, and changed the tree from 'try' to 'mozilla-inbound' (as
> you suggested).

I can now use mozilla-central!

> Ran `machclang static-analysis install` which failed, complaining I had not
> configured yet.

I fixed this issue.  Now we'll run configure if needed.

> Ran `machclang configure` to configure.
> Ran `machclang static-analysis install`.
> Ran `machclang static-analysis check`.
> 
> The check ran and exited with a 0 status code. However, there were quite a
> few files which were skipped (e.g.: Skipping
> /Volumes/Devel/Code/mozilla/mozilla-central/obj-clang-x86_64-apple-darwin16.
> 3.0/UnifiedProtocols26.cpp. Compile command not found.)

This was fixed by outputting the full file path name in the compilation database generation.

> and one error was
> generated which didn't seem to change the exit code or fail the static
> analysis run, and I only found by grepping through the output:
> 
> 1 error generated.
> Error while processing
> /Volumes/Devel/Code/mozilla/mozilla-central/obj-clang-x86_64-apple-darwin16.
> 3.0/security/nss/lib/ckfw/builtins/builtins_nssckbi/certdata.c.
> error: error reading
> '/Volumes/Devel/Code/mozilla/mozilla-central/obj-clang-x86_64-apple-darwin16.
> 3.0/security/nss/lib/ckfw/builtins/builtins_nssckbi/certdata.c'
> [clang-diagnostic-error]

This is bug 1337391.

> I would suggest changing the output from the command to generate
> considerably less clutter (I don't think the command should be printed
> unless it fails, and a simple percentage readout seems like it might be more
> practical).

Yeah...  This junk is coming from run-clang-tidy.py.  We should be able to parse this out relatively easily and show something better.  I'll put this in my todo list.

> In addition, it would be nice I would think if clang static-analysis install
> configured your tree for you.

See above.

> Also, as we talked about before, this should
> ideally run successfully on a tree which doesn't have --enable-clang-plugin.

Yeah, now we pass -extra-arg=-DMOZ_CLANG_PLUGIN to run-clang-tidy.py so the commands should work correctly on any type of build.

Comment 5

[(no longer active)]

Attachment: Add a mach static-analysis command (WIP)

Latest snapshot of the work in progress patch.

Comment 6

[Andi [:andi]]

I'm gonna test this and use it for my work. I'm gonna give you feedback if there are any issues. Thank you!

Comment 7

[Mike Hommey [:glandium]]

FWIW, I'm going to create a mach command to get the artifacts from toolchain tasks (not limited to the clang-tidy ones), and it's going to pull the appropriate version wrt the contents of the scripts that generate them in the current tree. I'll also probably backout bug 1328459 after bug 1335651 lands.

Comment 8

[(no longer active)]

(In reply to Mike Hommey [:glandium] from comment #7)
> FWIW, I'm going to create a mach command to get the artifacts from toolchain
> tasks (not limited to the clang-tidy ones), and it's going to pull the
> appropriate version wrt the contents of the scripts that generate them in
> the current tree.

I'm currently using the same code that the mach artifact command uses.  Are you planning to change the underlying code?  If yes, may I ask how so that I can plan what to do here?

> I'll also probably backout bug 1328459 after bug 1335651 lands.

I'd appreciate if you didn't backout that bug since I'm currently using it.  Once I have ported this work to whatever the replacement is, we can back it out.  That bug shouldn't get in your way.

Comment 9

[Andi [:andi]]

Any news on this, because it would help us a lot to have this landed for our static analysis bot that's being developed.

Comment 10

[Nika Layzell [:nika] (ni? for response)]

Ehsan has asked me to take this over, I just haven't had the time to work on it. I'll try to get something done quicklyish.

Comment 11

[Andi [:andi]]

Michael I can help you on this if you like.

Comment 12

[Nika Layzell [:nika] (ni? for response)]

Can you attach your most recent version of the patches to this bug ehsan? Thanks :)

Comment 13

[(no longer active)]

Attachment: Add a mach static-analysis command (WIP)

This is the latest version of the patch that I have.  This is based on the hg changeset ID af8a2573d0f1e9cc6f2ba0ab67d7a702a197f177 which is pretty old, so the first step would be to rebase this on top of the current trunk.  :-)

The ./mach static-analysis check command should work mostly fine.  The install command and the other two should also work fine, IIRC.  I would appreciate if you can test them all some more to make sure I haven't missed any corner cases.  I hope the couple of months of inactivity haven't caused any serious bitrot. :-)

Two remaining things to be addressed in this patch are:

* Figuring out how to handle clang-tidy toolchain updates.  Right now you can run ./mach static-analysis install manually to update the static analysis toolchain and ./mach static-analysis clear-cache manually to clear the downloaded binary, but there is no auto-update.  I think it would be nice to periodically check for a newer version, so that as we land newer static analyses, developers periodically get their downloaded clang-tidy toolchains updated.  I think we can easily fall into the trap of designing something overly complex, so I prefer to keep things simple, all else being equal.  Open questions are how frequently to check for updates and when to check them and whether and how to prompt for installing a newer toolchain.  The code to do the installation is already written (it's essentially the install command!) so it's essentially hooking up the update and invoking the check and things like that which are missing.  One thing to note is that we should avoid synchronously opening a connection to a server in order to avoid slowing down the check command.  (FWIW I'd be totally happy to defer the auto-update part to another bug also.)

* Figuring out what to do with the check command accepting diffs.  Right now the check command can operate in two modes, either a full-tree check or by piping in a diff of what has changed (think of `hg/git diff | ./mach static-analysis check`).  In order to make this work, I have hacked around the fact that our compilation database has entries for both unified files and normal cpp files which we actually don't compile.  This allows us to find the compiler invocation commands for .cpp files that we may find when looking at the list of changed files in a diff.  This part works pretty well, but since we're cheating around on unified builds, what the compiler actually sees isn't what it builds during a normal compile, so there is a chance that in this mode you'd get random unrelated errors coming from some .cpp files that are caused by unified build failures.  Also, header changes in this mode we can't deal with at all, since we don't have any information about them in the compilation database.  All in all, the diff mode support sort of half works.  When it works, it's really nice and much faster than a full tree check, but I'm not super convinced that it's worth keeping around in the final version.  Some more testing on it is needed, but based on the amount of testing that I had done before (which is just a little bit) I'd probably rip it out before submitting the final version for review.

Last but not least, note that this was mostly written and tested on Mac OSX and I also tested it a bit on Linux at some point but it would be nice to test it on Windows as well before it's prepared for review.

Let me know if you have any questions about how some part works, etc.  I hope it's mostly sane and easily to follow.  :-)

Comment 14

[Nika Layzell [:nika] (ni? for response)]

Andi, you offered to help with this, do you have time / want to take it over?

Comment 15

[Andi [:andi]]

well, i was just about to write you to ask you if i can take this over.

Comment 16

[Mike Hommey [:glandium]]

With the patch I'm about to attach to bug 1356524, you can run `mach artifact toolchain --from-build linux64-clang-tidy` (or macos64-clang-tidy, etc.) to get the exact clang-tidy toolchain that matches the tree (as long as it's been built on automation).

Comment 17

[Andi [:andi]]

Attachment: clang-tidy with mach.patch

Add the possibility auto fix errors, also merged with the last changes for artefacts build.

Comment 18

[Andi [:andi]]

Attachment: Bug 1328454 - Run static analysis based on clang-tidy from mach.

Review commit: https://reviewboard.mozilla.org/r/140190/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/140190/

Comment 19

[Andi [:andi]]

This version of the patch doesn't have the automatic update nor the acceptance of the diffs in order to be used for static analysis. But I think these can be added after we land this in a follow up patch.

Comment 20

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 8868584 [details]
Bug 1328454 - Run static analysis based on clang-tidy from mach.

Redirecting this to glandium as I'm not the person to do build system reviews :).

Thanks Andi! Can you file the follow-ups to add those other features which are currently missing in this version?

Comment 21

[Mike Hommey [:glandium]]

Comment on attachment 8868584 [details]
Bug 1328454 - Run static analysis based on clang-tidy from mach.

https://reviewboard.mozilla.org/r/140190/#review144494

::: python/mozbuild/mozbuild/artifacts.py:464
(Diff revision 1)
>          if not added_entry:
>              raise ValueError('Archive format changed! No pattern from "{patterns}"'
>                               'matched an archive path.'.format(
>                                   patterns=self.artifact_patterns))
>  
> +class ClangTidyArtifactJob(ArtifactJob):

Before going further in this review, I'll start with this: as commented previously, please use mach artifact toolchain to download clang-tidy.

Comment 22

[Andi [:andi]]

(In reply to Mike Hommey [:glandium] from comment #21)
> Comment on attachment 8868584 [details]
> Bug 1328454 - Add a mach static-analysis command that uses clang-tdy.
> 
> https://reviewboard.mozilla.org/r/140190/#review144494
> 
> ::: python/mozbuild/mozbuild/artifacts.py:464
> (Diff revision 1)
> >          if not added_entry:
> >              raise ValueError('Archive format changed! No pattern from "{patterns}"'
> >                               'matched an archive path.'.format(
> >                                   patterns=self.artifact_patterns))
> >  
> > +class ClangTidyArtifactJob(ArtifactJob):
> 
> Before going further in this review, I'll start with this: as commented
> previously, please use mach artifact toolchain to download clang-tidy.

I think I'm having a problem running the artefacts for clang-tidy, If i do this in m-c:

>>./mach artifact toolchain --from-build macosx64-clang-tidy

I get this error:
>>Could not find a toolchain build named `macosx64-clang-tidy`

That seems to come from here:

>>            for b in from_build:
>>                user_value = b
>>
>>                if '/' not in b:
>>                    b = '{}/opt'.format(b)
>>
>>                if not b.startswith('toolchain-'):
>>                    b = 'toolchain-{}'.format(b)
>>
>>                task = toolchains.get(b)
>>                if not task:
>>                    self.log(logging.ERROR, 'artifact', {'build': user_value},
>>                             'Could not find a toolchain build named `{build}`')
>>                    return 1

Is there anything that i need to configure?

Comment 23

[Mike Hommey [:glandium]]

bug 1345863 broke it by removing the '/opt' from the toolchain jobs. Filed bug 1369630.

Comment 24

[Andi [:andi]]

(In reply to Mike Hommey [:glandium] from comment #23)
> bug 1345863 broke it by removing the '/opt' from the toolchain jobs. Filed
> bug 1369630.

Thanks for letting me know about this! I'll update the patch with the new artifact job for clang-tidy.

Comment 25

[Andi [:andi]]

After this 1369630 landed I can proceed after:

>>                if not b.startswith('toolchain-'):
>>                    b = 'toolchain-{}'.format(b)
>>
>>                task = toolchains.get(b)

And i get a valid |task| but still when i want to actually run the optimize_task:

>>                task_id = optimize_task(task, {})
>>                if task_id in (True, False):
>>                    self.log(logging.ERROR, 'artifact', {'build': user_value},
>>                             'Could not find artifacts for a toolchain build '
>>                             'named `{build}`')

I get:
>> Could not find artifacts for a toolchain build named `macosx64-clang-tidy`

The same applies to when i want to use the toolchain for linux64-clang-tidy. Could you please advise on what do i have to do to move this further using the new toolchain?

Comment 26

[Mike Hommey [:glandium]]

I don't know what you're trying to do. Just run the mach artifact toolchain command, it should work.

Comment 27

[Andi [:andi]]

(In reply to Mike Hommey [:glandium] from comment #26)
> I don't know what you're trying to do. Just run the mach artifact toolchain
> command, it should work.

Sorry but maybe I'm missing the obvious here, if i do:
>>./mach artifact toolchain --from-build macosx64-clang-tidy
I will get:
>>Could not find artifacts for a toolchain build named `macosx64-clang-tidy`

Comment 28

[Mike Hommey [:glandium]]

What tree are you running that on?

Comment 29

[Andi [:andi]]

I'm doing this on m-c

Comment 30

[Mike Hommey [:glandium]]

It works for me on an unmodified m-c. Have you patched something?

Comment 31

[Andi [:andi]]

Nope, everything is fresh, revision: 362412. I'm on mac

Comment 32

[Mike Hommey [:glandium]]

I bet you have files in build/clang-plugin/ or build/build-clang/ that are not tracked by mercurial.

Comment 33

[Andi [:andi]]

(In reply to Mike Hommey [:glandium] from comment #32)
> I bet you have files in build/clang-plugin/ or build/build-clang/ that are
> not tracked by mercurial.

Yes, that worked, thank you, I've had a .pyc in the clang-plugin folder.

Comment 34

[Andi [:andi]]

Comment on attachment 8868584 [details]
Bug 1328454 - Run static analysis based on clang-tidy from mach.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/140190/diff/1-2/

Comment 35

[Andi [:andi]]

Comment on attachment 8868584 [details]
Bug 1328454 - Run static analysis based on clang-tidy from mach.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/140190/diff/2-3/

Comment 36

[Andi [:andi]]

Comment on attachment 8868584 [details]
Bug 1328454 - Run static analysis based on clang-tidy from mach.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/140190/diff/3-4/

Comment 37

[Andi [:andi]]

Comment on attachment 8868584 [details]
Bug 1328454 - Run static analysis based on clang-tidy from mach.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/140190/diff/4-5/

Comment 38

[Andi [:andi]]

Comment on attachment 8868584 [details]
Bug 1328454 - Run static analysis based on clang-tidy from mach.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/140190/diff/5-6/

Comment 39

[Mike Hommey [:glandium]]

Comment on attachment 8868584 [details]
Bug 1328454 - Run static analysis based on clang-tidy from mach.

https://reviewboard.mozilla.org/r/140190/#review155506

::: python/mozbuild/mozbuild/backend/configenvironment.py:135
(Diff revision 6)
>              self.lib_suffix = '.%s' % self.substs['LIB_SUFFIX']
>          if 'RUST_LIB_SUFFIX' in self.substs:
>              self.rust_lib_suffix = '.%s' % self.substs['RUST_LIB_SUFFIX']
>          self.dll_prefix = self.substs.get('DLL_PREFIX', '')
>          self.dll_suffix = self.substs.get('DLL_SUFFIX', '')
> +        self.bin_suffix = self.substs.get('BIN_SUFFIX', '')

with only one consumer of this value, I'd rather just use substs.get('BIN_SUFFIX', '') there instead.

::: python/mozbuild/mozbuild/base.py:630
(Diff revision 6)
>      def _activate_virtualenv(self):
>          self.virtualenv_manager.ensure()
>          self.virtualenv_manager.activate()
>  
>  
> +    def _set_log_level(self, verbose):

Moving this is fine, but please do so in a separate patch.

::: python/mozbuild/mozbuild/compilation/database.py:171
(Diff revision 6)
>              if per_source_flags is not None:
>                  c.extend(per_source_flags)
>              db.append({
>                  'directory': directory,
>                  'command': ' '.join(shell_quote(a) for a in c),
> -                'file': filename,
> +                'file': mozpath.join(directory, filename),

This would probably be preferable as a separate patch, with a rationale why this is needed, and how it affects the existing compilation database.

::: python/mozbuild/mozbuild/mach_commands.py:132
(Diff revision 6)
> -class BuildProgressFooter(object):
> -    """Handles display of a build progress indicator in a terminal.
> +class Footer(object):
> +    """Handles display of a footer in a terminal.

Same remark, refactoring is not bad, but please do so separately. For the footer-related refactor, please r?gps instead of me.

::: python/mozbuild/mozbuild/mach_commands.py:1877
(Diff revision 6)
>              if files:
>                  return 1
>  
>          return 0
>  
> +class StaticAnalysisSubCommand(SubCommand):

I think you have enough stuff going on here to put them in a different module.

::: python/mozbuild/mozbuild/mach_commands.py:1909
(Diff revision 6)
> +    def num_files(self):
> +        return self._total
> +
> +    def num_files_processed(self):
> +        return self._processed
> +
> +    def current_file(self):
> +        return self._current
> +
> +    def warnings_db(self):
> +        return self._warnings_database

Maybe make those @property?

::: python/mozbuild/mozbuild/mach_commands.py:1929
(Diff revision 6)
> +        try:
> +            warning = self._warnings_collector.process_line(line)
> +        except:
> +            pass
> +
> +        if line.find('clang-tidy') != -1:

if 'clang-tidy' in line

That said, that seems a little brute force... it seems to me it would be better ot make the run-clang-tidy.py output more parsable.

::: python/mozbuild/mozbuild/mach_commands.py:2042
(Diff revision 6)
> +        if fix:
> +            common_args=['-clang-tidy-binary', self._clang_tidy_path,
> +                        '-checks=%s' % checks,
> +                        '-fix',
> +                        '-extra-arg=-DMOZ_CLANG_PLUGIN']
> +        else:
> +            common_args=['-clang-tidy-binary', self._clang_tidy_path,
> +                        '-checks=%s' % checks,
> +                        '-extra-arg=-DMOZ_CLANG_PLUGIN']

common_args = [
    '-clang-tidy-binary',
    self._clang_tidy_pathm
    '-checks=%s' % checks,
    '-extra-arg=-DMOZ_CLANG_PLUGIN',
]
if fix:
    common_args.append('-fix')

::: python/mozbuild/mozbuild/mach_commands.py:2086
(Diff revision 6)
> +    @CommandArgument('source', nargs='?', type=str,
> +                     help='Where to fetch the helper tool from.  Can be omitted, in '
> +                          'which case the latest helper for the platform would be '

It's not clear what's expected from the user here. "source" is also a loaded term, which can be confusing in this context, where it's not about source code. From the code, it looks like this is expecting the path to an already downloaded clang-tidy.

::: python/mozbuild/mozbuild/mach_commands.py:2117
(Diff revision 6)
> +    def print_checks(self, verbose=False):
> +        self._set_log_level(verbose)
> +        rc = self._get_clang_tidy(verbose=verbose)
> +        if rc != 0:
> +            return rc
> +        args = [self._clang_tidy_path, '-list-checks', '-checks=-*,mozilla-*']

Shouldn't you allow to override the checks?

::: python/mozbuild/mozbuild/mach_commands.py:2221
(Diff revision 6)
> +            import platform
> +            if sys.platform == 'linux2':
> +                if platform.architecture()[0] == '64bit':
> +                    job = "linux64"
> +            elif sys.platform.startswith('win'):
> +                if platform.architecture()[0] == '64bit':
> +                    job = "win64"
> +                else:
> +                    job = "win32"
> +            elif sys.platform == 'darwin':
> +                job = 'macosx64'

I'm not a big fan of rolling your own host platform detection here. It would be better to reuse some other function that would be doing the same. I'm sure we have that somewhere. Maybe gps knows?

::: python/mozbuild/mozbuild/mach_commands.py:2252
(Diff revision 6)
> +            shutil.copytree(local, remote)
> +            shutil.rmtree(local)

That doesn't seem very desirable. Why not just run the artifact download from the directory you want your stuff in?

::: python/mozbuild/mozbuild/mach_commands.py:2258
(Diff revision 6)
> +        args = ['tar', '-xf', filename]
> +        rc = self.run_process(args, cwd=self._mach_context.state_dir)

It feels like you should use the tooltool unpack function that artifact_toolchain uses.

Comment 40

[Mike Hommey [:glandium]]

Greg, there's a question for you hidden in comment 39 (search for "Maybe gps knows")

Comment 41

[Andi [:andi]]

Attachment: 365134.patch

I've broke the modification of the compilation database into another patch, we need this for two reasons:

1. Distinguish files that enter the unified compilation
2. Distinguish duplicate file names from different directories that shouldn't enter the analysis process. Also this is useful to differentiate

I've taken a look at the implementation of clang-tidy to see how it uses in particular the compilation db and it seems that it only uses the field |command| from witch it can extract all of the necessary info to build the analysis tree.

Comment 42

[Andi [:andi]]

Attachment: 365483.patch

This is only the modification of the footer that's used by the static analysis in order to display it's progress.

Comment 43

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1d018372232d
Add full file path to compilation database. r=glandium

Comment 44

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1d018372232d

Comment 45

[Gregory Szorc [:gps]]

(In reply to Mike Hommey [:glandium] from comment #40)
> Greg, there's a question for you hidden in comment 39 (search for "Maybe gps
> knows")

It seems like something we should have in a generic and reusable function. But I don't see anything :/

Comment 46

[Gregory Szorc [:gps]]

Comment on attachment 8880381 [details] [diff] [review]
365483.patch

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

This seems like a reasonable refactor. Nice cleanup!

At some point, I imagine we'll need to move this to mach core (python/mach) so it is reusable outside this mach_commands.py. But until then, this is fine.

Comment 47

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4cc39579c80c
add footer for static analysis job. r=gps

Comment 48

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4cc39579c80c

Comment 49

[Andrew Halberstadt [:ahal]]

Comment on attachment 8868584 [details]
Bug 1328454 - Run static analysis based on clang-tidy from mach.

https://reviewboard.mozilla.org/r/140190/#review171274

::: python/mozbuild/mozbuild/mach_commands.py:2000
(Diff revision 6)
> +    @Command('static-analysis', category='testing',
> +             description='Run C++ static analysis checks')

Is this significantly different from |mach lint|? 

It's probably fine to land for now.. but if it can be written as a mozlint linter, I'm probably going to push for mozlint integration + removing this mach command in the future.

I'm still not 100% sure this makes sense as a mozlint linter or not though, so I'd need to try it out.

Comment 50

[Sylvestre Ledru [:Sylvestre]]

(In reply to Andrew Halberstadt [:ahal] from comment #49)
> It's probably fine to land for now.. but if it can be written as a mozlint
> linter, I'm probably going to push for mozlint integration + removing this
> mach command in the future.
Yeah, we should land this first and iterate on the next steps later.
The delay on this bug has been slowing us down in static analyzer effort for a while now :(

Comment 51

[(no longer active)]

(In reply to Andrew Halberstadt [:ahal] from comment #49)
> Comment on attachment 8868584 [details]
> Bug 1328454 - Add a mach static-analysis command that uses clang-tdy.
> 
> https://reviewboard.mozilla.org/r/140190/#review171274
> 
> ::: python/mozbuild/mozbuild/mach_commands.py:2000
> (Diff revision 6)
> > +    @Command('static-analysis', category='testing',
> > +             description='Run C++ static analysis checks')
> 
> Is this significantly different from |mach lint|? 

Yes, it is very different.  For one thing, it really compiles your code.  Another thing, down the road I'm planning to use this same infrastructure to do automated C++ rewriting.  It also is based on a custom build of clang-tidy with our custom C++ static analysis framework built on top of it, which requires installation etc.  And hiding the fact that there is clang-tidy running under the hood too much is a non-goal here.

> It's probably fine to land for now.. but if it can be written as a mozlint
> linter, I'm probably going to push for mozlint integration + removing this
> mach command in the future.
> 
> I'm still not 100% sure this makes sense as a mozlint linter or not though,
> so I'd need to try it out.

I actually think this not only doesn't make sense but it goes right against the goals of this command.  Also, linting and static analysis are two distinct things and I think the distinction is generally well understood by developers and the ergonomics of running a linter vs. a C++ static analyzer (for example in terms of the runtime) are also vastly different.  The choice of implementing this as a separate mach command was quite intentional.  :-)

Comment 52

[Andrew Halberstadt [:ahal]]

I replied over in bug 1389073 comment 2. Let's get this bug landed as is, and move mozlint related discussion to that other bug.

Comment 53

[Andi [:andi]]

Comment on attachment 8868584 [details]
Bug 1328454 - Run static analysis based on clang-tidy from mach.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/140190/diff/6-7/

Comment 54

[Mike Hommey [:glandium]]

Comment on attachment 8868584 [details]
Bug 1328454 - Run static analysis based on clang-tidy from mach.

https://reviewboard.mozilla.org/r/140190/#review186336

::: python/mozbuild/mozbuild/mach_commands.py:2105
(Diff revision 7)
> +        try:
> +            warning = self._warnings_collector.process_line(line)
> +        except:
> +            pass
> +
> +        if line.find('clang-tidy') != -1:

if 'clang-tidy' in line

As mentioned in previous review, this would be better if run-clang-tidy.py output was more reliably parseable. Please file a followup for that.

::: python/mozbuild/mozbuild/mach_commands.py:2258
(Diff revision 7)
> +            return rc
> +
> +    @StaticAnalysisSubCommand('static-analysis', 'install',
> +                              'Install the static analysis helper tool')
> +    @CommandArgument('source', nargs='?', type=str,
> +                     help='Where to fetch clang-tidy helper tool from.  Can be omitted, in '

Previous comment still applies here. What is the user supposed to pass here if they pass something? A local clang-tidy or an url? If the former, do you want the path to the clang-tidy binary, the directory containing it, or the directory where it's installed (excluding a 'bin' subdirecctory)?

::: python/mozbuild/mozbuild/mach_commands.py:2288
(Diff revision 7)
> +    def print_checks(self, verbose=False):
> +        self._set_log_level(verbose)
> +        rc = self._get_clang_tidy(verbose=verbose)
> +        if rc != 0:
> +            return rc
> +        args = [self._clang_tidy_path, '-list-checks', '-checks=-*,mozilla-*']

Same question as before: shouldn't this be overridable?

Comment 55

[Andi [:andi]]

(In reply to Mike Hommey [:glandium] from comment #54)
> Comment on attachment 8868584 [details]
> Bug 1328454 - Run static analysis based on clang-tidy from mach.
> 
> https://reviewboard.mozilla.org/r/140190/#review186336
> 
> ::: python/mozbuild/mozbuild/mach_commands.py:2105
> (Diff revision 7)
> > +        try:
> > +            warning = self._warnings_collector.process_line(line)
> > +        except:
> > +            pass
> > +
> > +        if line.find('clang-tidy') != -1:
> 
> if 'clang-tidy' in line
> 
> As mentioned in previous review, this would be better if run-clang-tidy.py
> output was more reliably parseable. Please file a followup for that.
Yes you are right that we could have run-clang-tidy.py. What i can do is followup a bug in llvm in order to have some options to format the output in a more parable way. But until we have that bug patched and laded can't we land this as it is?
> 
> ::: python/mozbuild/mozbuild/mach_commands.py:2258
> (Diff revision 7)
> > +            return rc
> > +
> > +    @StaticAnalysisSubCommand('static-analysis', 'install',
> > +                              'Install the static analysis helper tool')
> > +    @CommandArgument('source', nargs='?', type=str,
> > +                     help='Where to fetch clang-tidy helper tool from.  Can be omitted, in '
> 
> Previous comment still applies here. What is the user supposed to pass here
> if they pass something? A local clang-tidy or an url? If the former, do you
> want the path to the clang-tidy binary, the directory containing it, or the
> directory where it's installed (excluding a 'bin' subdirecctory)?
This is option is used in order for the user to be able to specify it's own archive of clang-tidy, this will be installed in the same place as the one from tooltool is. 
> 
> ::: python/mozbuild/mozbuild/mach_commands.py:2288
> (Diff revision 7)
> > +    def print_checks(self, verbose=False):
> > +        self._set_log_level(verbose)
> > +        rc = self._get_clang_tidy(verbose=verbose)
> > +        if rc != 0:
> > +            return rc
> > +        args = [self._clang_tidy_path, '-list-checks', '-checks=-*,mozilla-*']
> 
> Same question as before: shouldn't this be overridable?
I'm not sure we want to override this, since the purpose of this option is to print the mozilla checks that are available in the current clang-tidy binary.

Comment 56

[Mike Hommey [:glandium]]

(In reply to Andi-Bogdan Postelnicu [:andi] from comment #55)
> (In reply to Mike Hommey [:glandium] from comment #54)
> > Comment on attachment 8868584 [details]
> > Bug 1328454 - Run static analysis based on clang-tidy from mach.
> > 
> > https://reviewboard.mozilla.org/r/140190/#review186336
> > 
> > ::: python/mozbuild/mozbuild/mach_commands.py:2105
> > (Diff revision 7)
> > > +        try:
> > > +            warning = self._warnings_collector.process_line(line)
> > > +        except:
> > > +            pass
> > > +
> > > +        if line.find('clang-tidy') != -1:
> > 
> > if 'clang-tidy' in line
> > 
> > As mentioned in previous review, this would be better if run-clang-tidy.py
> > output was more reliably parseable. Please file a followup for that.
> Yes you are right that we could have run-clang-tidy.py. What i can do is
> followup a bug in llvm in order to have some options to format the output in
> a more parable way. But until we have that bug patched and laded can't we
> land this as it is?

cf. my comment: "please file a followup for that". That implies it's not blocking.

> > ::: python/mozbuild/mozbuild/mach_commands.py:2258
> > (Diff revision 7)
> > > +            return rc
> > > +
> > > +    @StaticAnalysisSubCommand('static-analysis', 'install',
> > > +                              'Install the static analysis helper tool')
> > > +    @CommandArgument('source', nargs='?', type=str,
> > > +                     help='Where to fetch clang-tidy helper tool from.  Can be omitted, in '
> > 
> > Previous comment still applies here. What is the user supposed to pass here
> > if they pass something? A local clang-tidy or an url? If the former, do you
> > want the path to the clang-tidy binary, the directory containing it, or the
> > directory where it's installed (excluding a 'bin' subdirecctory)?
> This is option is used in order for the user to be able to specify it's own
> archive of clang-tidy, this will be installed in the same place as the one
> from tooltool is. 

Please make this clear in the help.

Comment 57

[Andi [:andi]]

(In reply to Mike Hommey [:glandium] from comment #56)
> (In reply to Andi-Bogdan Postelnicu [:andi] from comment #55)
> > (In reply to Mike Hommey [:glandium] from comment #54)
> > > Comment on attachment 8868584 [details]
> > > Bug 1328454 - Run static analysis based on clang-tidy from mach.
> > > 
> > > https://reviewboard.mozilla.org/r/140190/#review186336
> > > 
> > > ::: python/mozbuild/mozbuild/mach_commands.py:2105
> > > (Diff revision 7)
> > > > +        try:
> > > > +            warning = self._warnings_collector.process_line(line)
> > > > +        except:
> > > > +            pass
> > > > +
> > > > +        if line.find('clang-tidy') != -1:
> > > 
> > > if 'clang-tidy' in line
> > > 
> > > As mentioned in previous review, this would be better if run-clang-tidy.py
> > > output was more reliably parseable. Please file a followup for that.
> > Yes you are right that we could have run-clang-tidy.py. What i can do is
> > followup a bug in llvm in order to have some options to format the output in
> > a more parable way. But until we have that bug patched and laded can't we
> > land this as it is?
> 
> cf. my comment: "please file a followup for that". That implies it's not
> blocking.
> 
> > > ::: python/mozbuild/mozbuild/mach_commands.py:2258
> > > (Diff revision 7)
> > > > +            return rc
> > > > +
> > > > +    @StaticAnalysisSubCommand('static-analysis', 'install',
> > > > +                              'Install the static analysis helper tool')
> > > > +    @CommandArgument('source', nargs='?', type=str,
> > > > +                     help='Where to fetch clang-tidy helper tool from.  Can be omitted, in '
> > > 
> > > Previous comment still applies here. What is the user supposed to pass here
> > > if they pass something? A local clang-tidy or an url? If the former, do you
> > > want the path to the clang-tidy binary, the directory containing it, or the
> > > directory where it's installed (excluding a 'bin' subdirecctory)?
> > This is option is used in order for the user to be able to specify it's own
> > archive of clang-tidy, this will be installed in the same place as the one
> > from tooltool is. 
> 
> Please make this clear in the help.

Thanks for clarifying this to me, I'll update the patch and repost it.

Comment 58

[Andi [:andi]]

Comment on attachment 8868584 [details]
Bug 1328454 - Run static analysis based on clang-tidy from mach.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/140190/diff/7-8/

Comment 59

[Andi [:andi]]

The main difference in the latest patch is the modification of the help for the custom clang-tidy path that now is:

>>    @CommandArgument('source', nargs='?', type=str,
>>                     help='Where to fetch a local clang-tidy archive helper tool.'
>>                          'It will be installed in ~/.mozbuild/clang-tidy/.'
>>                          'Can be omitted, in which case the latest helper for '
>>                          'the platform would be automatically detected.')

Comment 60

[Andi [:andi]]

Comment on attachment 8868584 [details]
Bug 1328454 - Run static analysis based on clang-tidy from mach.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/140190/diff/8-9/

Comment 61

[Andi [:andi]]

Improved again the description on the source argument when passed to the installer command:
>>    @CommandArgument('source', nargs='?', type=str,
>>                     help='Where to fetch a local clang-tidy archive helper tool.'
>>                          'It will be installed in ~/.mozbuild/clang-tidy/.'
>>                          'Can be omitted, in which case the latest clang-tidy '
>>                          'helper for the platform would be automatically '
>>                          'detected and installed.')

Comment 62

[Mike Hommey [:glandium]]

Comment on attachment 8868584 [details]
Bug 1328454 - Run static analysis based on clang-tidy from mach.

https://reviewboard.mozilla.org/r/140190/#review186936

::: python/mozbuild/mozbuild/mach_commands.py:2258
(Diff revisions 7 - 9)
>              return rc
>  
>      @StaticAnalysisSubCommand('static-analysis', 'install',
>                                'Install the static analysis helper tool')
>      @CommandArgument('source', nargs='?', type=str,
> -                     help='Where to fetch clang-tidy helper tool from.  Can be omitted, in '
> +                     help='Where to fetch a local clang-tidy archive helper tool.'

"local clang-tidy archive helper tool" reads weird. "local archive containing the clang-tidy helper tool"?

Comment 63

[Andi [:andi]]

Comment on attachment 8868584 [details]
Bug 1328454 - Run static analysis based on clang-tidy from mach.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/140190/diff/9-10/

Comment 64

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b8a7df0b3a46
Run static analysis based on clang-tidy from mach. r=glandium

Comment 65

[(no longer active)]

Yay!  Is there more to land in this bug?  I noticed it has the leave-open keyword?

Comment 66

[Andi [:andi]]

(In reply to :Ehsan Akhgari (needinfo please, extremely long backlog) from comment #65)
> Yay!  Is there more to land in this bug?  I noticed it has the leave-open
> keyword?
Good catch, I will do a followup bug with a neet feature of doing an auto-update if the clang tooltool has changed.

Comment 67

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/b8a7df0b3a46