From 75fffb14b8c74a6495d481f03586db7b505e737f Mon Sep 17 00:00:00 2001 From: Cole Faust Date: Sun, 13 Jun 2021 15:23:16 -0700 Subject: [PATCH] Add a build flag to always enable errorprone per-target Currently, errorprone is only run if the RUN_ERROR_PRONE enviornment variable is true. Add a flag that individual modules can use to always enable errorprone. In a followup cl, I plan to add another flag that will force all errorprone checks to be errors, so that modules can be confident that they're not ignoring any errorprone checks. Bug: 190944875 Test: New unit test and manually Change-Id: Iab0c81642ed22a736add054147829e91a891d179 --- java/base.go | 45 +++++++++++++++++++++++++++++++++++++-------- java/builder.go | 17 ----------------- java/java_test.go | 28 ++++++++++++++++++++++++++++ java/kotlin_test.go | 1 - 4 files changed, 65 insertions(+), 26 deletions(-) diff --git a/java/base.go b/java/base.go index 440b00478..1daa10876 100644 --- a/java/base.go +++ b/java/base.go @@ -155,6 +155,11 @@ type CommonProperties struct { // List of java_plugin modules that provide extra errorprone checks. Extra_check_modules []string + + // Whether to run errorprone on a normal build. If this is false, errorprone + // will still be run if the RUN_ERROR_PRONE environment variable is true. + // Default false. + Enabled *bool } Proto struct { @@ -701,7 +706,7 @@ func (j *Module) collectBuilderFlags(ctx android.ModuleContext, deps deps) javaB // javaVersion flag. flags.javaVersion = getJavaVersion(ctx, String(j.properties.Java_version), android.SdkContext(j)) - if ctx.Config().RunErrorProne() { + if ctx.Config().RunErrorProne() || Bool(j.properties.Errorprone.Enabled) { if config.ErrorProneClasspath == nil && ctx.Config().TestProductVariables == nil { ctx.ModuleErrorf("cannot build with Error Prone, missing external/error_prone?") } @@ -972,14 +977,23 @@ func (j *Module) compile(ctx android.ModuleContext, aaptSrcJar android.Path) { } if len(uniqueSrcFiles) > 0 || len(srcJars) > 0 { var extraJarDeps android.Paths - if ctx.Config().RunErrorProne() { - // If error-prone is enabled, add an additional rule to compile the java files into - // a separate set of classes (so that they don't overwrite the normal ones and require - // a rebuild when error-prone is turned off). - // TODO(ccross): Once we always compile with javac9 we may be able to conditionally - // enable error-prone without affecting the output class files. + if Bool(j.properties.Errorprone.Enabled) { + // If error-prone is enabled, enable errorprone flags on the regular + // build. + flags = enableErrorproneFlags(flags) + } else if ctx.Config().RunErrorProne() { + // Otherwise, if the RUN_ERROR_PRONE environment variable is set, create + // a new jar file just for compiling with the errorprone compiler to. + // This is because we don't want to cause the java files to get completely + // rebuilt every time the state of the RUN_ERROR_PRONE variable changes. + // We also don't want to run this if errorprone is enabled by default for + // this module, or else we could have duplicated errorprone messages. + errorproneFlags := enableErrorproneFlags(flags) errorprone := android.PathForModuleOut(ctx, "errorprone", jarName) - RunErrorProne(ctx, errorprone, uniqueSrcFiles, srcJars, flags) + + transformJavaToClasses(ctx, errorprone, -1, uniqueSrcFiles, srcJars, errorproneFlags, nil, + "errorprone", "errorprone") + extraJarDeps = append(extraJarDeps, errorprone) } @@ -1303,6 +1317,21 @@ func (j *Module) compile(ctx android.ModuleContext, aaptSrcJar android.Path) { j.outputFile = outputFile.WithoutRel() } +// Returns a copy of the supplied flags, but with all the errorprone-related +// fields copied to the regular build's fields. +func enableErrorproneFlags(flags javaBuilderFlags) javaBuilderFlags { + flags.processorPath = append(flags.errorProneProcessorPath, flags.processorPath...) + + if len(flags.errorProneExtraJavacFlags) > 0 { + if len(flags.javacFlags) > 0 { + flags.javacFlags += " " + flags.errorProneExtraJavacFlags + } else { + flags.javacFlags = flags.errorProneExtraJavacFlags + } + } + return flags +} + func (j *Module) compileJavaClasses(ctx android.ModuleContext, jarName string, idx int, srcFiles, srcJars android.Paths, flags javaBuilderFlags, extraJarDeps android.Paths) android.WritablePath { diff --git a/java/builder.go b/java/builder.go index cde87310f..ea011b8e1 100644 --- a/java/builder.go +++ b/java/builder.go @@ -279,23 +279,6 @@ func TransformJavaToClasses(ctx android.ModuleContext, outputFile android.Writab transformJavaToClasses(ctx, outputFile, shardIdx, srcFiles, srcJars, flags, deps, "javac", desc) } -func RunErrorProne(ctx android.ModuleContext, outputFile android.WritablePath, - srcFiles, srcJars android.Paths, flags javaBuilderFlags) { - - flags.processorPath = append(flags.errorProneProcessorPath, flags.processorPath...) - - if len(flags.errorProneExtraJavacFlags) > 0 { - if len(flags.javacFlags) > 0 { - flags.javacFlags += " " + flags.errorProneExtraJavacFlags - } else { - flags.javacFlags = flags.errorProneExtraJavacFlags - } - } - - transformJavaToClasses(ctx, outputFile, -1, srcFiles, srcJars, flags, nil, - "errorprone", "errorprone") -} - // Emits the rule to generate Xref input file (.kzip file) for the given set of source files and source jars // to compile with given set of builder flags, etc. func emitXrefRule(ctx android.ModuleContext, xrefFile android.WritablePath, idx int, diff --git a/java/java_test.go b/java/java_test.go index bd373c177..78d9ab4c7 100644 --- a/java/java_test.go +++ b/java/java_test.go @@ -1392,3 +1392,31 @@ func TestDefaultInstallable(t *testing.T) { assertDeepEquals(t, "Default installable value should be true.", proptools.BoolPtr(true), module.properties.Installable) } + +func TestErrorproneEnabled(t *testing.T) { + ctx, _ := testJava(t, ` + java_library { + name: "foo", + srcs: ["a.java"], + errorprone: { + enabled: true, + }, + } + `) + + javac := ctx.ModuleForTests("foo", "android_common").Description("javac") + + // Test that the errorprone plugins are passed to javac + expectedSubstring := "-Xplugin:ErrorProne" + if !strings.Contains(javac.Args["javacFlags"], expectedSubstring) { + t.Errorf("expected javacFlags to conain %q, got %q", expectedSubstring, javac.Args["javacFlags"]) + } + + // Modules with errorprone { enabled: true } will include errorprone checks + // in the main javac build rule. Only when RUN_ERROR_PRONE is true will + // the explicit errorprone build rule be created. + errorprone := ctx.ModuleForTests("foo", "android_common").MaybeDescription("errorprone") + if errorprone.RuleParams.Description != "" { + t.Errorf("expected errorprone build rule to not exist, but it did") + } +} diff --git a/java/kotlin_test.go b/java/kotlin_test.go index 1c146a192..fd2f3ca61 100644 --- a/java/kotlin_test.go +++ b/java/kotlin_test.go @@ -185,7 +185,6 @@ func TestKapt(t *testing.T) { buildOS := android.BuildOs.String() kapt := result.ModuleForTests("foo", "android_common").Rule("kapt") - //kotlinc := ctx.ModuleForTests("foo", "android_common").Rule("kotlinc") javac := result.ModuleForTests("foo", "android_common").Description("javac") errorprone := result.ModuleForTests("foo", "android_common").Description("errorprone")