Integrating the infer static analyzer into mach

RESOLVED FIXED in Firefox 63

Status

P3
normal
RESOLVED FIXED
9 months ago
7 months ago

People

(Reporter: rbartlensky, Assigned: rbartlensky)

Tracking

unspecified
mozilla63
Dependency tree / graph

Firefox Tracking Flags

(firefox63 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

9 months ago
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.
(Assignee)

Updated

9 months ago
Depends on: 1473951

Comment 2

8 months ago
mozreview-review
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 :/
Attachment #8995219 - Flags: review?(gps) → review+
(Assignee)

Comment 3

8 months ago
mozreview-review-reply
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 hidden (mozreview-request)
(Assignee)

Updated

8 months ago
Depends on: 1479401
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

8 months ago
Blocks: 1479503
(Assignee)

Comment 7

8 months ago
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!
Flags: needinfo?(gps)
I guess this means you want me to look at the code again. I'll do that now.
Flags: needinfo?(gps)

Comment 9

8 months ago
mozreview-review
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?
Attachment #8995219 - Flags: review+ → review-
(Assignee)

Comment 10

8 months ago
mozreview-review
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 13

8 months ago
Fixed all your comments.
Flags: needinfo?(gps)

Updated

8 months ago
Flags: needinfo?(gps)
(Assignee)

Updated

8 months ago
Keywords: checkin-needed
Keywords: checkin-needed

Comment 14

8 months ago
mozreview-review
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
Attachment #8995219 - Flags: review?(gps) → review+
(Assignee)

Updated

7 months ago
Keywords: checkin-needed
could not land. qimport says "no patches found for this bug"
Keywords: checkin-needed

Comment 16

7 months ago
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)
please disregard my previous comment.
Comment hidden (mozreview-request)
Keywords: checkin-needed

Comment 19

7 months ago
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 hidden (mozreview-request)

Comment 21

7 months ago
Pushed by ncsoregi@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9fb094b1e5f6
Add infer to java-check, install, clear-cache, and print-checks subcommands. r=gps
Keywords: checkin-needed

Comment 22

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/9fb094b1e5f6
Status: NEW → RESOLVED
Last Resolved: 7 months ago
status-firefox63: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
You need to log in before you can comment on or make changes to this bug.