Closed Bug 1473278 Opened 6 years ago Closed 6 years ago

Integrating the infer static analyzer into mach

Categories

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

Product:
Developer Infrastructure
Component:
Source Code Analysis
Type:
enhancement

Tracking

(firefox63 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla63
Tracking Flags:
Tracking Status
firefox63 --- fixed

People

(Reporter: rbartlensky, Assigned: rbartlensky)

References

Details

Attachments

(1 file)

Bug 1473278: Add infer to java-check, install, clear-cache, and print-checks subcommands.
59 bytes, text/x-review-board-request
gps
: review+
Details 
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.
Depends on: 1473951
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+
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.
Depends on: 1479401
Blocks: 1479503
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 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-
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.
Fixed all your comments.
Flags: needinfo?(gps)
Flags: needinfo?(gps)
Keywords: checkin-needed
Keywords: checkin-needed
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+
Keywords: checkin-needed
could not land. qimport says "no patches found for this bug"
Keywords: checkin-needed
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.
Keywords: checkin-needed
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)
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
https://hg.mozilla.org/mozilla-central/rev/9fb094b1e5f6
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox63: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Product: Firefox Build System → Developer Infrastructure
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: