From 2b1536e4dbc9e9178a8f6cf3fa049170811a75a1 Mon Sep 17 00:00:00 2001 From: Cole Faust Date: Fri, 18 Jun 2021 12:25:54 -0700 Subject: [PATCH] Allow disabling errorprone even when RUN_ERROR_PRONE is true Some modules use -XepDisableAllChecks to disable errorprone. However, this still causes them to be compiled twice when RUN_ERROR_PRONE is true. Allow the new enabled property to be set to false to disable errorprone entirely, so that those modules are only compiled once. Bug: 190944875 Test: New unit tests Change-Id: Ie68695db762fffcaf11bbbcb0509c4fcab73f5c5 --- java/base.go | 13 ++++++---- java/java_test.go | 66 ++++++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 73 insertions(+), 6 deletions(-) diff --git a/java/base.go b/java/base.go index 1daa10876..a7cc58e33 100644 --- a/java/base.go +++ b/java/base.go @@ -156,9 +156,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. + // This property can be in 3 states. When set to true, errorprone will + // be run during the regular build. When set to false, errorprone will + // never be run. When unset, errorprone will be run when the RUN_ERROR_PRONE + // environment variable is true. Setting this to false will improve build + // performance more than adding -XepDisableAllChecks in javacflags. Enabled *bool } @@ -706,7 +708,8 @@ 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() || Bool(j.properties.Errorprone.Enabled) { + epEnabled := j.properties.Errorprone.Enabled + if (ctx.Config().RunErrorProne() && epEnabled == nil) || Bool(epEnabled) { if config.ErrorProneClasspath == nil && ctx.Config().TestProductVariables == nil { ctx.ModuleErrorf("cannot build with Error Prone, missing external/error_prone?") } @@ -981,7 +984,7 @@ func (j *Module) compile(ctx android.ModuleContext, aaptSrcJar android.Path) { // If error-prone is enabled, enable errorprone flags on the regular // build. flags = enableErrorproneFlags(flags) - } else if ctx.Config().RunErrorProne() { + } else if ctx.Config().RunErrorProne() && j.properties.Errorprone.Enabled == nil { // 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 diff --git a/java/java_test.go b/java/java_test.go index 78d9ab4c7..0f9965d03 100644 --- a/java/java_test.go +++ b/java/java_test.go @@ -1409,7 +1409,7 @@ func TestErrorproneEnabled(t *testing.T) { // 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"]) + t.Errorf("expected javacFlags to contain %q, got %q", expectedSubstring, javac.Args["javacFlags"]) } // Modules with errorprone { enabled: true } will include errorprone checks @@ -1420,3 +1420,67 @@ func TestErrorproneEnabled(t *testing.T) { t.Errorf("expected errorprone build rule to not exist, but it did") } } + +func TestErrorproneDisabled(t *testing.T) { + bp := ` + java_library { + name: "foo", + srcs: ["a.java"], + errorprone: { + enabled: false, + }, + } + ` + ctx := android.GroupFixturePreparers( + PrepareForTestWithJavaDefaultModules, + android.FixtureMergeEnv(map[string]string{ + "RUN_ERROR_PRONE": "true", + }), + ).RunTestWithBp(t, bp) + + javac := ctx.ModuleForTests("foo", "android_common").Description("javac") + + // Test that the errorprone plugins are not passed to javac, like they would + // be if enabled was true. + expectedSubstring := "-Xplugin:ErrorProne" + if strings.Contains(javac.Args["javacFlags"], expectedSubstring) { + t.Errorf("expected javacFlags to not contain %q, got %q", expectedSubstring, javac.Args["javacFlags"]) + } + + // Check that no errorprone build rule is created, like there would be + // if enabled was unset and RUN_ERROR_PRONE was true. + errorprone := ctx.ModuleForTests("foo", "android_common").MaybeDescription("errorprone") + if errorprone.RuleParams.Description != "" { + t.Errorf("expected errorprone build rule to not exist, but it did") + } +} + +func TestErrorproneEnabledOnlyByEnvironmentVariable(t *testing.T) { + bp := ` + java_library { + name: "foo", + srcs: ["a.java"], + } + ` + ctx := android.GroupFixturePreparers( + PrepareForTestWithJavaDefaultModules, + android.FixtureMergeEnv(map[string]string{ + "RUN_ERROR_PRONE": "true", + }), + ).RunTestWithBp(t, bp) + + javac := ctx.ModuleForTests("foo", "android_common").Description("javac") + errorprone := ctx.ModuleForTests("foo", "android_common").Description("errorprone") + + // Check that the errorprone plugins are not passed to javac, because they + // will instead be passed to the separate errorprone compilation + expectedSubstring := "-Xplugin:ErrorProne" + if strings.Contains(javac.Args["javacFlags"], expectedSubstring) { + t.Errorf("expected javacFlags to not contain %q, got %q", expectedSubstring, javac.Args["javacFlags"]) + } + + // Check that the errorprone plugin is enabled + if !strings.Contains(errorprone.Args["javacFlags"], expectedSubstring) { + t.Errorf("expected errorprone to contain %q, got %q", expectedSubstring, javac.Args["javacFlags"]) + } +}