Closed Bug 1479503 Opened Last year Closed 11 months ago

Integrate infer into static-analysis autotest

Categories

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

enhancement

Tracking

(firefox65 fixed)

RESOLVED FIXED
mozilla65
Tracking Status
firefox65 --- fixed

People

(Reporter: rbartlensky, Assigned: rbartlensky)

References

Details

Attachments

(4 files)

./mach static-analysis autotest should test both clang-tidy and infer.
Could you please tell me if the gradle file that I created makes sense? I had to compile some java files, and add a dependency to them in order to test infer, but I could only come up with this solution.
Flags: needinfo?(nalexander)
Comment on attachment 8996955 [details]
Bug 1479503: Check infer in ./mach static-analysis autotest.

https://reviewboard.mozilla.org/r/260938/#review268106

Fundamentally, this doesn't do what I would expect a Gradle file to do, but I read the ticket and this patch and I still have no idea what you're trying to achieve.

Are you trying to run infer against Fennec?  Then you should follow the |mach android COMMAND| pattern, like |mach android {test,checkstyle,findbugs,lint}|.  If you're not running against Fennec, don't pollute the top-level `settings.gradle`; make some Gradle project rooted in `/tools` or something that's independent.

::: settings.gradle:46
(Diff revision 1)
>  project(':app').projectDir = new File("${json.topsrcdir}/mobile/android/app")
>  project(':geckoview').projectDir = new File("${json.topsrcdir}/mobile/android/geckoview")
>  project(':geckoview_example').projectDir = new File("${json.topsrcdir}/mobile/android/geckoview_example")
>  project(':omnijar').projectDir = new File("${json.topsrcdir}/mobile/android/app/omnijar")
>  project(':thirdparty').projectDir = new File("${json.topsrcdir}/mobile/android/thirdparty")
> +project(':test_infer').projectDir = new File("${json.topsrcdir}/tools/infer/test")

What does this have to do with Android?

::: tools/infer/test/Biabduction.java:1
(Diff revision 1)
> +import javax.annotation.Nullable;

These all need license blocks.

::: tools/infer/test/Biabduction.json:1
(Diff revision 1)
> +[

Trim trailing whitespace, here and everywhere.

::: tools/infer/test/build.gradle:5
(Diff revision 1)
> +apply plugin: 'java'
> +
> +buildDir "${topobjdir}/gradle/build/test-infer"
> +
> +repositories {

This will always hit the network, including in automation.  For Android, we do non-trivial work to not hit the network in automation.  Is this running in automation?

::: tools/infer/test/build.gradle:12
(Diff revision 1)
> +        url "https://maven.google.com"
> +    }
> +}
> +
> +dependencies {
> +    compile "com.google.code.findbugs:jsr305:3.0.2"

Why are you referring to findbugs?  Aren't you running infer at some point?

::: tools/infer/test/build.gradle:15
(Diff revision 1)
> +
> +dependencies {
> +    compile "com.google.code.findbugs:jsr305:3.0.2"
> +}
> +
> +task compileInferTestBiabduction(type: JavaCompile) {

This could be one task, I expect, that takes all the files as input.  But really, this type of stuff should be done using the Java Gradle plugin, which compiles, JARs, etc, in a uniform manner.
Attachment #8996955 - Flags: review-
Flags: needinfo?(nalexander)
Comment on attachment 8996955 [details]
Bug 1479503: Check infer in ./mach static-analysis autotest.

https://reviewboard.mozilla.org/r/260938/#review268212

::: settings.gradle:46
(Diff revision 1)
>  project(':app').projectDir = new File("${json.topsrcdir}/mobile/android/app")
>  project(':geckoview').projectDir = new File("${json.topsrcdir}/mobile/android/geckoview")
>  project(':geckoview_example').projectDir = new File("${json.topsrcdir}/mobile/android/geckoview_example")
>  project(':omnijar').projectDir = new File("${json.topsrcdir}/mobile/android/app/omnijar")
>  project(':thirdparty').projectDir = new File("${json.topsrcdir}/mobile/android/thirdparty")
> +project(':test_infer').projectDir = new File("${json.topsrcdir}/tools/infer/test")

Infer can only be run when Fennec is being built.

::: tools/infer/test/build.gradle:5
(Diff revision 1)
> +apply plugin: 'java'
> +
> +buildDir "${topobjdir}/gradle/build/test-infer"
> +
> +repositories {

Let me expand on what I am trying to achieve. Infer is not a dependency, it is downloaded and compiled by the `infer` task. In order to analyze the java codebase, you need to be building Fennec, and the analysis is started by running the command `./mach static-analysis check-java`. The `static-analysis` command also has a subcommand called `autotest`, which tests the static analyzers by downloading them and running them on some test files. These test files are created in such a way that trigger certain errors, and all the errors are listed in a json file. In the above case, each java file has a corresponding json file which contains all the issues reported by infer. In order for the command to run successfully, all issues must be found by the analyzers. This patch adds support for `infer` to be tested.

::: tools/infer/test/build.gradle:12
(Diff revision 1)
> +        url "https://maven.google.com"
> +    }
> +}
> +
> +dependencies {
> +    compile "com.google.code.findbugs:jsr305:3.0.2"

I need `javax.annotation.*`. Is there another dependency that I can use?

::: tools/infer/test/build.gradle:15
(Diff revision 1)
> +
> +dependencies {
> +    compile "com.google.code.findbugs:jsr305:3.0.2"
> +}
> +
> +task compileInferTestBiabduction(type: JavaCompile) {

Infer has many different checkers. Each file tests a certain checker. My script reads the checkers from a file, and tests each checker in particular. If I were to compile all of them at once, all issues would be listed in the same file, which means I would need to somehow find the correct file for each issue. Also, `infer` needs to be given a compilation command in order to analyze a file. For instance either a `javac Something.java` or a gradle task. I need to use gradle for this because some issues can only be triggered when using certain annotations (Nullable, ThreadSafe), which means I need some dependency.
OK, this is helpful.  I think I understand what you're doing now.  What you really want is to make it easy for a developer to update the infer version (equivalent to vendoring in a new version of some dependency) and be confident that nothing regressed.

In that case, there's no need (and many reasons to avoid) coupling this to the top-level Gradle configuration for Firefox for Android.

Make tools/test/infer its own Gradle project:

tools/test/infer
  settings.gradle
  gradle
    wrapper
       gradle-wrapper.properties
  gradlew
  gradlew.bat
  ...
  autotest
    build.gradle
    src
      java
        File.java
        File.json

Your build.gradle is non-standard but it's not wrong, and it works, which means you don't have dig through the Java plugin, rediscovering things.  I do think you should generalize just a bit:

```Groovy

def createSingleTask = { name ->
    task("compileInferTest${name.capitalize()}", type: JavaCompile) {
        source = fileTree(dir: '.', include: "$name.java")
        classpath = project.configurations.compileClasspath
        destinationDir = file("${project.buildDir}")
    }
}

createSingleTask('Checkers')
createSingleTask('Other')
...

```

Using com.google.code.findbugs:jsr305 is fine, and using Google's Maven repo is too, since this won't ever run in automation.
(In reply to Nick Alexander :nalexander from comment #5)
> OK, this is helpful.  I think I understand what you're doing now.  What you
> really want is to make it easy for a developer to update the infer version
> (equivalent to vendoring in a new version of some dependency) and be
> confident that nothing regressed.
> 

Exactly!

> In that case, there's no need (and many reasons to avoid) coupling this to
> the top-level Gradle configuration for Firefox for Android.
> 
> Make tools/test/infer its own Gradle project:
> 
> tools/test/infer
>   settings.gradle
>   gradle
>     wrapper
>        gradle-wrapper.properties
>   gradlew
>   gradlew.bat
>   ...
>   autotest
>     build.gradle
>     src
>       java
>         File.java
>         File.json
> 
> Your build.gradle is non-standard but it's not wrong, and it works, which
> means you don't have dig through the Java plugin, rediscovering things.  I
> do think you should generalize just a bit:
> 
> ```Groovy
> 
> def createSingleTask = { name ->
>     task("compileInferTest${name.capitalize()}", type: JavaCompile) {
>         source = fileTree(dir: '.', include: "$name.java")
>         classpath = project.configurations.compileClasspath
>         destinationDir = file("${project.buildDir}")
>     }
> }
> 
> createSingleTask('Checkers')
> createSingleTask('Other')
> ...
> 
> ```
> 

This is a lot better, thank you!

> Using com.google.code.findbugs:jsr305 is fine, and using Google's Maven repo
> is too, since this won't ever run in automation.

I have to also provide a task on taskcluster which runs `./mach static-analysis autotest` on a Fennec build. I will shortly push another commit here in which I do just that. Could you please look over that and let me know if I should still move `test/infer` into its own gradle project?
Comment on attachment 8996955 [details]
Bug 1479503: Check infer in ./mach static-analysis autotest.

https://reviewboard.mozilla.org/r/260938/#review268840

nalexander has agreed to review this for me, since this is getting out of my league for understanding.
Attachment #8996955 - Flags: review?(gps)
Comment on attachment 8996955 [details]
Bug 1479503: Check infer in ./mach static-analysis autotest.

https://reviewboard.mozilla.org/r/260938/#review268892

You're mixing this in to the Android (Fennec) world _and I don't understand why_.  This all stands alone -- just make a separate Gradle project, accept that you're going to download your dependency from the network, and run your own |./gradlew COMMAND| in your subdirectory.  There's nothing here that has anything to do with Fennec; you're testing the tool itself.
Attachment #8996955 - Flags: review-
This patch also contains a lot of python code which is mach specific, who can I assign as a reviewer for that part?
Flags: needinfo?(gps)
Adds infer to ./mach static-analysis autotest.
I guess I answered the needinfo by reviewing this patch. But I would prefer we find someone more in tune with the Android side of the world to look at things. I'm not sure who that is due to the changing world of Android at Mozilla.
Flags: needinfo?(gps)
Comment on attachment 9004903 [details]
Bug 1479503: Check infer in ./mach static-analysis autotest.

Nick Alexander :nalexander [he/him] has approved the revision.
Attachment #9004903 - Flags: review+
Pushed by sledru@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/081d8311be59
Check infer in ./mach static-analysis autotest. r=nalexander
Backed out for build bustage - java not found. 

backout: https://hg.mozilla.org/integration/autoland/rev/fca86e2acc89dd0be22f3f4a92f32e823ac395f1

failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=198821787&repo=autoland&lineNumber=1142

[task 2018-09-12T10:12:27.639Z] 10:12:27     INFO -  checking for xargs... /usr/bin/xargs
[task 2018-09-12T10:12:27.639Z] 10:12:27     INFO -  checking for rpmbuild... not found
[task 2018-09-12T10:12:27.639Z] 10:12:27     INFO -  checking for java... not found
[task 2018-09-12T10:12:27.640Z] 10:12:27     INFO -  ERROR: The program java was not found.  Set $JAVA_HOME to your Java SDK directory or use '--with-java-bin-path={java-bin-dir}'
[task 2018-09-12T10:12:27.668Z] 10:12:27     INFO -  *** Fix above errors and then restart with\
[task 2018-09-12T10:12:27.668Z] 10:12:27     INFO -                 "/usr/bin/make -f client.mk build"
[task 2018-09-12T10:12:27.668Z] 10:12:27     INFO -  client.mk:123: recipe for target 'configure' failed
[task 2018-09-12T10:12:27.668Z] 10:12:27     INFO -  make: *** [configure] Error 1
[task 2018-09-12T10:12:27.757Z] 10:12:27    ERROR - Return code: 2
[task 2018-09-12T10:12:27.757Z] 10:12:27  WARNING - setting return code to 2
[task 2018-09-12T10:12:27.758Z] 10:12:27    FATAL - 'mach build -v' did not run successfully. Please check log for errors.
[task 2018-09-12T10:12:27.758Z] 10:12:27    FATAL - Running post_fatal callback...
[task 2018-09-12T10:12:27.758Z] 10:12:27    FATAL - Exiting -1
[task 2018-09-12T10:12:27.758Z] 10:12:27     INFO - [mozharness: 2018-09-12 10:12:27.758424Z] Finished build step (failed)
[task 2018-09-12T10:12:27.758Z] 10:12:27     INFO - Running post-run listener: _summarize
[task 2018-09-12T10:12:27.758Z] 10:12:27    ERROR - # TBPL FAILURE #
[task 2018-09-12T10:12:27.758Z] 10:12:27     INFO - [mozharness: 2018-09-12 10:12:27.758692Z] FxDesktopBuild summary:
[task 2018-09-12T10:12:27.758Z] 10:12:27    ERROR - # TBPL FAILURE #
[task 2018-09-12T10:12:27.758Z] 10:12:27     INFO - Running post-run listener: copy_logs_to_upload_dir
[task 2018-09-12T10:12:27.758Z] 10:12:27     INFO - Copying logs to upload dir...
[task 2018-09-12T10:12:27.759Z] 10:12:27     INFO - mkdir: /builds/worker/workspace/build/upload/logs
[task 2018-09-12T10:12:27.759Z] 10:12:27    DEBUG - Copying /builds/worker/logs/localconfig.json to /builds/worker/workspace/build/upload/logs/localconfig.json
[task 2018-09-12T10:12:27.759Z] 10:12:27    DEBUG - mkdir_p: /builds/worker/workspace/build/upload/logs Already exists.
[task 2018-09-12T10:12:27.759Z] 10:12:27    DEBUG - Copying /builds/worker/logs/log_info.log to /builds/worker/workspace/build/upload/logs/log_info.log
[task 2018-09-12T10:12:27.759Z] 10:12:27    DEBUG - mkdir_p: /builds/worker/workspace/build/upload/logs Already exists.
[task 2018-09-12T10:12:27.759Z] 10:12:27    DEBUG - Copying /builds/worker/logs/log_raw.log to /builds/worker/workspace/build/upload/logs/log_raw.log
[task 2018-09-12T10:12:27.759Z] 10:12:27    DEBUG - mkdir_p: /builds/worker/workspace/build/upload/logs Already exists.
[task 2018-09-12T10:12:27.760Z] 10:12:27    DEBUG - Copying /builds/worker/logs/log_warning.log to /builds/worker/workspace/build/upload/logs/log_warning.log
[task 2018-09-12T10:12:27.760Z] 10:12:27    DEBUG - mkdir_p: /builds/worker/workspace/build/upload/logs Already exists.
[task 2018-09-12T10:12:27.760Z] 10:12:27    DEBUG - Copying /builds/worker/logs/log_critical.log to /builds/worker/workspace/build/upload/logs/log_critical.log
[task 2018-09-12T10:12:27.760Z] 10:12:27    DEBUG - mkdir_p: /builds/worker/workspace/build/upload/logs Already exists.
[task 2018-09-12T10:12:27.760Z] 10:12:27    DEBUG - Copying /builds/worker/logs/log_error.log to /builds/worker/workspace/build/upload/logs/log_error.log
[task 2018-09-12T10:12:27.760Z] 10:12:27    DEBUG - mkdir_p: /builds/worker/workspace/build/upload/logs Already exists.
[task 2018-09-12T10:12:27.760Z] 10:12:27    DEBUG - Copying /builds/worker/logs/log_debug.log to /builds/worker/workspace/build/upload/logs/log_debug.log
[task 2018-09-12T10:12:27.760Z] 10:12:27    DEBUG - mkdir_p: /builds/worker/workspace/build/upload/logs Already exists.
[task 2018-09-12T10:12:27.760Z] 10:12:27    DEBUG - Copying /builds/worker/logs/log_fatal.log to /builds/worker/workspace/build/upload/logs/log_fatal.log
[task 2018-09-12T10:12:27.768Z] cleanup
[task 2018-09-12T10:12:27.768Z] + cleanup
[task 2018-09-12T10:12:27.768Z] + local rv=255
[task 2018-09-12T10:12:27.768Z] + cleanup_xvfb
[task 2018-09-12T10:12:27.768Z] pidof Xvfb
[task 2018-09-12T10:12:27.768Z] ++ pidof Xvfb
[task 2018-09-12T10:12:27.769Z] + local xvfb_pid=53
[task 2018-09-12T10:12:27.769Z] + local vnc=false
[task 2018-09-12T10:12:27.769Z] + local interactive=false
[task 2018-09-12T10:12:27.769Z] + '[' -n 53 ']'
[task 2018-09-12T10:12:27.769Z] + [[ false == false ]]
[task 2018-09-12T10:12:27.769Z] + [[ false == false ]]
[task 2018-09-12T10:12:27.769Z] + kill 53
[task 2018-09-12T10:12:27.769Z] + screen -XS xvfb quit
[task 2018-09-12T10:12:27.894Z] No screen session found.
[task 2018-09-12T10:12:27.895Z] + true
[task 2018-09-12T10:12:27.895Z] + exit 255
[taskcluster 2018-09-12 10:12:30.906Z] === Task Finished ===
[taskcluster 2018-09-12 10:12:31.733Z] Unsuccessful task run with exit code: 255 completed in 126.501 seconds
Flags: needinfo?(rbartlensky)
(In reply to Natalia Csoregi [:nataliaCs] from comment #18)
> Backed out for build bustage - java not found. 
> 
> backout:
> https://hg.mozilla.org/integration/autoland/rev/
> fca86e2acc89dd0be22f3f4a92f32e823ac395f1
> 
> failure log:
> https://treeherder.mozilla.org/logviewer.
> html#?job_id=198821787&repo=autoland&lineNumber=1142

Fixing it now.
Flags: needinfo?(rbartlensky)
Adds infer to ./mach static-analysis autotest.
Comment on attachment 9008494 [details]
Bug 1479503: Check infer in ./mach static-analysis autotest.

Nick Alexander :nalexander [he/him] has approved the revision.
Attachment #9008494 - Flags: review+
Pushed by sledru@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/aae4f349fa58
Check infer in ./mach static-analysis autotest. r=nalexander
https://hg.mozilla.org/mozilla-central/rev/aae4f349fa58
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
This change apparently made Linux desktop builds require Java, which is quite unfortunate.  Can we back this change out until we find a better way to handle that Java dependency?
Flags: needinfo?(sledru)
(In reply to Nathan Froyd [:froydnj] from comment #24)
> This change apparently made Linux desktop builds require Java, [...]

Actually it even requires Java 9 or earlier, because javah has been removed in Java 10.
yeah, let's back out this. This wasn't intended.
Flags: needinfo?(sledru)
The problem is partly that installing Java isn’t covered by "./mach
boostrap", see https://bugzilla.mozilla.org/show_bug.cgi?id=1272728.
Backed out changeset aae4f349fa58 (Bug 1479503) per developer's request on IRC.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla64 → ---
Backout by shindli@mozilla.com:
https://hg.mozilla.org/mozilla-central/rev/c5fbbf959e23
Backed out changeset aae4f349fa58 per developer's request on IRC a=backout
I see what's happening here, in moz.configure we find:

>>@depends(target, host, check_build_environment, '--help')
>>@imports('os')
>>def include_infer_autotest(target, host, build_env, help):
>>    # Do not include autotest configuration on Windows or macos
>>    # because infer is not available there
>>    if target.kernel in ['Linux'] and host.kernel in ['Linux']:
>>        path = os.path.join(build_env.topsrcdir, 'tools', 'infer', 'test', 'moz.configure')
>>        # on some builds, this path won't exists, in which case we can just
>>        # ignore the infer autotest
>>        if os.path.exists(path):
>>            return path
>>    return None
>>
>>include(include_infer_autotest)

This included tools/infer/test/moz.configure:
>>@depends(build_project)
>>def java_include(build_project):
>>    # if mobile/android is enabled then the moz.configure from ./mobile
>>    # will include java
>>    if build_project == 'mobile/android':
>>        return None
>>    return 'java.configure'
>>
>>include(java_include)

And java.configure enables the dependency. I think this was wrongfully done because of the static-analysis-autotest task on linux64 that integrated the autotest for clang-tidy and infer, but the correct approach here would be to remove this dependency an add a separate task for infer that has a dedicated mozconfig specially tailored for Android.
(In reply to Andi-Bogdan Postelnicu [:andi] from comment #30)
> I see what's happening here, in moz.configure we find:
> 
> >>@depends(target, host, check_build_environment, '--help')
> >>@imports('os')
> >>def include_infer_autotest(target, host, build_env, help):
> >>    # Do not include autotest configuration on Windows or macos
> >>    # because infer is not available there
> >>    if target.kernel in ['Linux'] and host.kernel in ['Linux']:
> >>        path = os.path.join(build_env.topsrcdir, 'tools', 'infer', 'test', 'moz.configure')
> >>        # on some builds, this path won't exists, in which case we can just
> >>        # ignore the infer autotest
> >>        if os.path.exists(path):
> >>            return path
> >>    return None
> >>
> >>include(include_infer_autotest)
> 
> This included tools/infer/test/moz.configure:
> >>@depends(build_project)
> >>def java_include(build_project):
> >>    # if mobile/android is enabled then the moz.configure from ./mobile
> >>    # will include java
> >>    if build_project == 'mobile/android':
> >>        return None
> >>    return 'java.configure'
> >>
> >>include(java_include)
> 
> And java.configure enables the dependency. I think this was wrongfully done
> because of the static-analysis-autotest task on linux64 that integrated the
> autotest for clang-tidy and infer, but the correct approach here would be to
> remove this dependency an add a separate task for infer that has a dedicated
> mozconfig specially tailored for Android.

I am not sure what the best approach might be. I might find a solution to this if I knew the answer to the following question:
Do we ever want to run autotest locally? If so the user has to enable Java. We can just let the user know when they run autotest that they do not have java installed and that they need to add `--with-java-bin` to their mozconfig or set `JAVA_HOME. Does that sound reasonable? And also what you suggested makes sense. For the task we should have a dedicated mozconfig, which makes sure that java is found.
> Do we ever want to run autotest locally? 
Nope, I don't really care about that. The best ROI is to run it at review phase.

From what we are seeing with clang-based static analysis, even developers who know about static analysis aren't running it locally... Especially as we did a great job having a < 15m feedback loop for free.
(In reply to Sylvestre Ledru [:sylvestre] from comment #32)
> Nope, I don't really care about that. The best ROI is to run it at review
> phase.
> 
> From what we are seeing with clang-based static analysis, even developers
> who know about static analysis aren't running it locally... Especially as we
> did a great job having a < 15m feedback loop for free.

The autotest subcommand tests whether clang-tidy and infer work correctly (this command is used when we introduce a new version of the these tools to check if they work as intended). This command does not analyze any of the user's files. I think you might be referring to `./mach static-analysis check/check-java`.
We definitely need to run i locally, we add new checkers to clang-tidy with test cases locally and after that it gets into infrastructure. But I think we want to run it only if we are dealing with an android build like: 
>>    if build_project == 'mobile/android':
>>        return None
So I think the fix here is simple, don't add assume that we want to run autotest for infer on a non-android build.
Now autotest does not require java to be installed, but
it will let the user know that infer is not being tested if java
is missing.
This looks nice, but please add a new target on try where we do only the autotest for android.
(In reply to Andi-Bogdan Postelnicu [:andi] from comment #36)
> This looks nice, but please add a new target on try where we do only the
> autotest for android.

What do you mean by that?
Sorry for my last comment if it wasn’t that clear, it would be nice that on infra we will test the auto test feature of infer.
(In reply to Andi-Bogdan Postelnicu [:andi] from comment #38)
> Sorry for my last comment if it wasn’t that clear, it would be nice that on
> infra we will test the auto test feature of infer.

https://tools.taskcluster.net/groups/P06fyex2S8Ob5R-QgxwExQ/tasks/WkgtizQ0TWKJa2KxPcpoPw/runs/0/logs/public%2Flogs%2Flive.log#L1718

Both clang-tidy and infer are checked now.
There are a lot of attachments, the latest one is the one that should be pushed.
Keywords: checkin-needed
Pushed by ncsoregi@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8e850a230bfc
Check infer in ./mach static-analysis autotest. r=nalexander
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/8e850a230bfc
Status: REOPENED → RESOLVED
Closed: Last year11 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
You need to log in before you can comment on or make changes to this bug.