1473278 - Integrating the infer static analyzer into mach

Status: RESOLVED FIXED

(Developer Infrastructure :: Source Code Analysis, enhancement, P3)

Description

[Robert Bartlensky [:rbartlensky]]

I would like to integrate infer into mach. So far I managed to enable infer when running `./mach static-analysis check`.

The next step is to enable infer to be downloaded with the PackageFrontend class.

Comment 1

[Robert Bartlensky [:rbartlensky]]

Attachment: Bug 1473278: Add infer to java-check, install, clear-cache, and print-checks subcommands.

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

Comment 2

[Gregory Szorc [:gps]]

Comment on attachment 8995219 [details]
Bug 1473278: Add infer to java-check, install, clear-cache, and print-checks subcommands.

https://reviewboard.mozilla.org/r/259704/#review266722

I'm not an expert on gradle's workings. But this seems reasonable as a first implementation.

The use of `gradle clean` in here does seem like use of this command could be a bit developer hostile. If I'm interpreting behavior correctly, it will blow away a build so it can run infer. So if a developer is trying to run infer as part of a edit-test cycle, then it could be annoying have to build with and without infer over and over. I'm not sure if there is a better way to deal with this though :/

Comment 3

[Robert Bartlensky [:rbartlensky]]

Comment on attachment 8995219 [details]
Bug 1473278: Add infer to java-check, install, clear-cache, and print-checks subcommands.

https://reviewboard.mozilla.org/r/259704/#review266722

Yes you are right about gradle clean, but consider this case:

The user builds the android project, and all is fine. Then they run the check-java. Because the project is already built, `gradlew compile` will return immediately. Because there is nothing left to compile, `infer` will not manage to capture and analyze any file, because they are all compiled. If for instance some files are out of date, gradle will only build those, which infer picks up and analyzes, but it still does not analyze all files. I have to think of another solution to this then. Touching files seems to not trigger a recompile so I might need to do something else.

Comment 4

[Robert Bartlensky [:rbartlensky]]

Comment on attachment 8995219 [details]
Bug 1473278: Add infer to java-check, install, clear-cache, and print-checks subcommands.

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

Comment 5

[Robert Bartlensky [:rbartlensky]]

Comment on attachment 8995219 [details]
Bug 1473278: Add infer to java-check, install, clear-cache, and print-checks subcommands.

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

Comment 6

[Robert Bartlensky [:rbartlensky]]

Comment on attachment 8995219 [details]
Bug 1473278: Add infer to java-check, install, clear-cache, and print-checks subcommands.

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

Comment 7

[Robert Bartlensky [:rbartlensky]]

I had to do some refactoring. I couldn't find an alternative to running gradle clean before each run of infer. I will open a separate bug for that. Now infer is part of the static-analysis command, and works correctly!

Comment 8

[Gregory Szorc [:gps]]

I guess this means you want me to look at the code again. I'll do that now.

Comment 9

[Gregory Szorc [:gps]]

Comment on attachment 8995219 [details]
Bug 1473278: Add infer to java-check, install, clear-cache, and print-checks subcommands.

https://reviewboard.mozilla.org/r/259704/#review267612

The addition of infer to various commands seems like it will break `mach static-analysis` on platforms / build configurations that don't support infer. This seems wrong and is the reason for the r- of this version. There's a chance I'm wrong here, as I'm unsure of the intended audience of `mach static-analysis`. But since it is doing things with Clang, I assume it applies to multiple platforms and non-Android builds.

THe rest of the comments are nits.

::: python/mozbuild/mozbuild/mach_commands.py:1759
(Diff revision 4)
>                       help='Number of concurrent jobs to run.'
>                       ' Default is the number of CPUs.')
>      @CommandArgument('--task', '-t', type=str,
> -                     default='compileLocalWithGeckoBinariesNoMinApiReleaseSources',
> +                     default='compileLocalWithGeckoBinariesNoMinApiDebugSources',
>                       help='Which gradle tasks to use to compile the java codebase.')
> -    def check_java(self, source=[], jobs=2, strip=1, verbose=False, checks=[],
> +    def check_java(self, source=['mobile'], jobs=2, strip=1, verbose=False, checks=[],

This likely isn't relevant to this code, but assigning a mutable value to the default argument value in a Python function should be avoided. See https://docs.python-guide.org/writing/gotchas/#mutable-default-arguments.

::: python/mozbuild/mozbuild/mach_commands.py:1771
(Diff revision 4)
> +        check_all = False
> +        for i in source:
> +            # if last folder is mobile
> +            if i.rstrip(os.sep).split(os.sep)[-1] == 'mobile':
> +                check_all = True
> +                break

Nit: this could be rewritten as a one-liner using `any()`. e.g. `check_all = any(i.rstrip()... == 'mobile' for i in source)`.

::: python/mozbuild/mozbuild/mach_commands.py:1781
(Diff revision 4)
> +                break
>          # gather all java sources from the source variable
>          java_sources = []
> -        for i in source:
> -            f = mozpath.join(self.topsrcdir, i)
> -            if os.path.isdir(f):
> +        if not check_all:
> +            java_sources = self._get_java_files(source)
> +            if java_sources == []:

`if not java_sources`.

::: python/mozbuild/mozbuild/mach_commands.py:1793
(Diff revision 4)
>          if rc != 0:
>              return rc
>          # which checkers to use, and which folders to exclude
> -        checkers, excludes = self._get_infer_config(checks)
> +        all_checkers, third_party_path = self._get_infer_config()
> +        checkers, excludes = self._get_infer_args(
> +            checks=all_checkers if checks == [] else checks,

`checks=checks or all_checkers`

::: python/mozbuild/mozbuild/mach_commands.py:1820
(Diff revision 4)
> +                for root, _, files in os.walk(f):
> +                    for file in files:

This should probably be deterministic. Do that via:

```
for root, dirs, files in os.walk(f):
    dirs.sort()
    for file in sorted(files):
        ...

```

::: python/mozbuild/mozbuild/mach_commands.py:1997
(Diff revision 4)
> +        if rc == 0:
> +            rc = self._get_infer(force=True, skip_cache=skip_cache, verbose=verbose)

Does this mean we try to install infer on all build configurations and platforms? Won't this mean `mach static-analysis install` will only work on Linux64 builds and fail everywhere else?

Comment 10

[Robert Bartlensky [:rbartlensky]]

Comment on attachment 8995219 [details]
Bug 1473278: Add infer to java-check, install, clear-cache, and print-checks subcommands.

https://reviewboard.mozilla.org/r/259704/#review267710

::: python/mozbuild/mozbuild/mach_commands.py:1759
(Diff revision 4)
>                       help='Number of concurrent jobs to run.'
>                       ' Default is the number of CPUs.')
>      @CommandArgument('--task', '-t', type=str,
> -                     default='compileLocalWithGeckoBinariesNoMinApiReleaseSources',
> +                     default='compileLocalWithGeckoBinariesNoMinApiDebugSources',
>                       help='Which gradle tasks to use to compile the java codebase.')
> -    def check_java(self, source=[], jobs=2, strip=1, verbose=False, checks=[],
> +    def check_java(self, source=['mobile'], jobs=2, strip=1, verbose=False, checks=[],

This was a great read! I don't modify the `task` variable at all in this case, but you mentioned that it isn't relevant here. I'll keep that in mind next time!

::: python/mozbuild/mozbuild/mach_commands.py:1771
(Diff revision 4)
> +        check_all = False
> +        for i in source:
> +            # if last folder is mobile
> +            if i.rstrip(os.sep).split(os.sep)[-1] == 'mobile':
> +                check_all = True
> +                break

love one-liners, this is great!

::: python/mozbuild/mozbuild/mach_commands.py:1820
(Diff revision 4)
> +                for root, _, files in os.walk(f):
> +                    for file in files:

I might be inclined to say that `dirs.sort` does not do anything, but I assume `os.walk` has a reference to that.

::: python/mozbuild/mozbuild/mach_commands.py:1997
(Diff revision 4)
> +        if rc == 0:
> +            rc = self._get_infer(force=True, skip_cache=skip_cache, verbose=verbose)

Infer will only be installed in the case where we are building Fennec. I could rework this such that it isn't mandatory, and maybe if the user tries to use `check-java` on something other than linux64 an error message is printed.

Comment 11

[Robert Bartlensky [:rbartlensky]]

Comment on attachment 8995219 [details]
Bug 1473278: Add infer to java-check, install, clear-cache, and print-checks subcommands.

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

Comment 12

[Robert Bartlensky [:rbartlensky]]

Comment on attachment 8995219 [details]
Bug 1473278: Add infer to java-check, install, clear-cache, and print-checks subcommands.

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

Comment 13

[Robert Bartlensky [:rbartlensky]]

Fixed all your comments.

Comment 14

[Gregory Szorc [:gps]]

Comment on attachment 8995219 [details]
Bug 1473278: Add infer to java-check, install, clear-cache, and print-checks subcommands.

https://reviewboard.mozilla.org/r/259704/#review268830

Comment 15

[Bogdan Tara[:bogdan_tara | bogdant]]

could not land. qimport says "no patches found for this bug"

Comment 16

[Mozilla Autoland]

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s d51540b9552839d9545a68a89e4d62936cc45822 -d 9de74f5039a4: rebasing 477126:d51540b95528 "Bug 1473278: Add infer to install, clear-cache, and print-checks subcommands. r=gps" (tip)
other [source] changed tools/infer/config.yaml which local [dest] deleted
use (c)hanged version, leave (d)eleted, or leave (u)nresolved? u
merging python/mozbuild/mozbuild/mach_commands.py
warning: conflicts while merging python/mozbuild/mozbuild/mach_commands.py! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

Comment 17

[Bogdan Tara[:bogdan_tara | bogdant]]

please disregard my previous comment.

Comment 18

[Robert Bartlensky [:rbartlensky]]

Comment on attachment 8995219 [details]
Bug 1473278: Add infer to java-check, install, clear-cache, and print-checks subcommands.

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

Comment 19

[Mozilla Autoland]

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 126dd4a5cd6b86aadeacf13d4d33e31cae0711c6 -d 8a23e3023c92: rebasing 477127:126dd4a5cd6b "Bug 1473278: Add infer to install, clear-cache, and print-checks subcommands. r=gps" (tip)
other [source] changed tools/infer/config.yaml which local [dest] deleted
use (c)hanged version, leave (d)eleted, or leave (u)nresolved? u
merging python/mozbuild/mozbuild/mach_commands.py
warning: conflicts while merging python/mozbuild/mozbuild/mach_commands.py! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

Comment 20

[Robert Bartlensky [:rbartlensky]]

Comment on attachment 8995219 [details]
Bug 1473278: Add infer to java-check, install, clear-cache, and print-checks subcommands.

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

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9fb094b1e5f6
Add infer to java-check, install, clear-cache, and print-checks subcommands. r=gps

Comment 22

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/9fb094b1e5f6