Include classpath, sourcepath, and extra javac options in the Java indexer setup
Closed, ResolvedPublic

Description

A Java CompilationUnit can have a kythe.proto.JavaDetails message attached to it. If such a message is attached, the indexer should use its fields to populate the corresponding settings when setting up the Java compiler instance—including sourcepath, classpath, bootclasspath, and extra flags.

I think the indexer may be doing some of this now, but bootclasspath in particular seems to be missing.

fromberger created this task.Via WebMar 2 2017, 8:25 PM
fromberger assigned this task to jrtom.
fromberger added a project: Restricted Project.
fromberger added a subscriber: fromberger.
Herald added a subscriber: Core Team. · View Herald TranscriptVia HeraldMar 2 2017, 8:25 PM
jrtom added a comment.Via WebMar 15 2017, 7:47 PM

A couple of questions below.

fromberger@ and I found the place [1] where the change needs to happen; basically we need to extract the JavaDetails message from the CompilationUnit, which happens to be present in the one call site [2] of this method, so ~no plumbing required (yay!).

Then I made the mistake of looking around and trying to make sense of things, such as:

  • do we actually need the isAnalsysis parameter?
  • what's with the commented out code in the call site?

I also plan to clean up the code (locally) in other respects, but those are the two things that I'd like some more-informed input on.

IsLocalAnalysis
Inside optionsFromCompilationUnit(), there's a boolean parameter "isLocalAnalysis"; appendJREJarsToClasspath() is only called when that parameter is false. The documentation states, in part:
"Adding jre jars to classpath for local analysis done by {@link com.google.devtools.kythe.platform.java.local.LocalJavacAnalysisDriver} will cause the analysis to fail."

Two things I noticed about that:
(1) There is no class called LocalJavacAnalysisDriver in this project. There is one in the original internal code, but at a different path (and the corresponding method that it calls doesn't have this parameter).
(2) All the call sites to optionsFromCompilationUnit() eventually pass in false for isLocalAnalysis.

So as far as I can tell, that parameter can and should be removed, unless we expect to actually be doing a Local Analysis at some point in Kythe.

Dead code

// Use Xlint options in the compilation unit, instead of adding Xlint:all,
    // so that it's possible to turn off lint checks.
    // options = JavacOptionsUtils.useAllWarnings(options);

Any reason to keep this around? If not, we can kill the method it's referring to, as it's not currently called anywhere.

[1] appendJREJarsToClasspath(): java/com/google/devtools/kythe/platform/java/JavacOptionsUtils.java (line 109)
[2] optionsFromCompilationUnit(): java/com/google/devtools/kythe/platform/java/JavaCompilationDetails.java (line 207)

fromberger added a comment.Via WebMar 16 2017, 7:25 AM

Your analysis seems sound to me. I think we probably culted that switch.

jrtom closed this task as "Resolved".Via WebMar 31 2017, 4:34 PM

D1474 resolves this task.

Add Comment