From 79e6f6bfccbc2f7209e8e3ce1872951be64a41d5 Mon Sep 17 00:00:00 2001 From: Jaewoong Jung Date: Wed, 21 Apr 2021 14:01:55 -0700 Subject: [PATCH 1/3] Forbid bypassing updatability lint checks. Test: lint_test.go Bug: 182349282 Change-Id: Iac7c01493b449c2ddd6df6c68f8a74dfe72dfd7a --- java/Android.bp | 1 + java/lint.go | 16 ++++++++++- java/lint_test.go | 73 +++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 89 insertions(+), 1 deletion(-) create mode 100644 java/lint_test.go diff --git a/java/Android.bp b/java/Android.bp index 2a4b596ab..f6ce3c449 100644 --- a/java/Android.bp +++ b/java/Android.bp @@ -82,6 +82,7 @@ bootstrap_go_package { "java_test.go", "jdeps_test.go", "kotlin_test.go", + "lint_test.go", "platform_bootclasspath_test.go", "platform_compat_config_test.go", "plugin_test.go", diff --git a/java/lint.go b/java/lint.go index a77daa85d..0618cbf6d 100644 --- a/java/lint.go +++ b/java/lint.go @@ -26,6 +26,10 @@ import ( "android/soong/remoteexec" ) +// lint checks automatically enforced for modules that have different min_sdk_version than +// sdk_version +var updatabilityChecks = []string{"NewApi"} + type LintProperties struct { // Controls for running Android Lint on the module. Lint struct { @@ -298,7 +302,17 @@ func (l *linter) lint(ctx android.ModuleContext) { } if l.minSdkVersion != l.compileSdkVersion { - l.extraMainlineLintErrors = append(l.extraMainlineLintErrors, "NewApi") + l.extraMainlineLintErrors = append(l.extraMainlineLintErrors, updatabilityChecks...) + _, filtered := android.FilterList(l.properties.Lint.Warning_checks, updatabilityChecks) + if len(filtered) != 0 { + ctx.PropertyErrorf("lint.warning_checks", + "Can't treat %v checks as warnings if min_sdk_version is different from sdk_version.", filtered) + } + _, filtered = android.FilterList(l.properties.Lint.Disabled_checks, updatabilityChecks) + if len(filtered) != 0 { + ctx.PropertyErrorf("lint.disabled_checks", + "Can't disable %v checks if min_sdk_version is different from sdk_version.", filtered) + } } extraLintCheckModules := ctx.GetDirectDepsWithTag(extraLintCheckTag) diff --git a/java/lint_test.go b/java/lint_test.go new file mode 100644 index 000000000..c257f63ac --- /dev/null +++ b/java/lint_test.go @@ -0,0 +1,73 @@ +// Copyright 2021 Google Inc. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package java + +import ( + "testing" + + "android/soong/android" +) + +func TestJavaLintBypassUpdatableChecks(t *testing.T) { + testCases := []struct { + name string + bp string + error string + }{ + { + name: "warning_checks", + bp: ` + java_library { + name: "foo", + srcs: [ + "a.java", + ], + min_sdk_version: "29", + sdk_version: "current", + lint: { + warning_checks: ["NewApi"], + }, + } + `, + error: "lint.warning_checks: Can't treat \\[NewApi\\] checks as warnings if min_sdk_version is different from sdk_version.", + }, + { + name: "disable_checks", + bp: ` + java_library { + name: "foo", + srcs: [ + "a.java", + ], + min_sdk_version: "29", + sdk_version: "current", + lint: { + disabled_checks: ["NewApi"], + }, + } + `, + error: "lint.disabled_checks: Can't disable \\[NewApi\\] checks if min_sdk_version is different from sdk_version.", + }, + } + + for _, testCase := range testCases { + t.Run(testCase.name, func(t *testing.T) { + errorHandler := android.FixtureExpectsAtLeastOneErrorMatchingPattern(testCase.error) + android.GroupFixturePreparers(PrepareForTestWithJavaDefaultModules). + ExtendWithErrorHandler(errorHandler). + RunTestWithBp(t, testCase.bp) + }) + } +} From 11623b6052b0254f4e59680ba078aaeefd065752 Mon Sep 17 00:00:00 2001 From: Jaewoong Jung Date: Wed, 21 Apr 2021 14:16:57 -0700 Subject: [PATCH 2/3] Move Java lint tests to lint_test.go Test: lint_test.go Bug: 182349282 Change-Id: I30a46c8f704e66cd04541c78d3f22a140d3284ef --- java/java_test.go | 101 --------------------------------------------- java/lint_test.go | 102 ++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 102 insertions(+), 101 deletions(-) diff --git a/java/java_test.go b/java/java_test.go index e7ea4ef54..71bd4573f 100644 --- a/java/java_test.go +++ b/java/java_test.go @@ -1203,107 +1203,6 @@ func TestIncludeSrcs(t *testing.T) { } } -func TestJavaLint(t *testing.T) { - ctx, _ := testJavaWithFS(t, ` - java_library { - name: "foo", - srcs: [ - "a.java", - "b.java", - "c.java", - ], - min_sdk_version: "29", - sdk_version: "system_current", - } - `, map[string][]byte{ - "lint-baseline.xml": nil, - }) - - foo := ctx.ModuleForTests("foo", "android_common") - - sboxProto := android.RuleBuilderSboxProtoForTests(t, foo.Output("lint.sbox.textproto")) - if !strings.Contains(*sboxProto.Commands[0].Command, "--baseline lint-baseline.xml") { - t.Error("did not pass --baseline flag") - } -} - -func TestJavaLintWithoutBaseline(t *testing.T) { - ctx, _ := testJavaWithFS(t, ` - java_library { - name: "foo", - srcs: [ - "a.java", - "b.java", - "c.java", - ], - min_sdk_version: "29", - sdk_version: "system_current", - } - `, map[string][]byte{}) - - foo := ctx.ModuleForTests("foo", "android_common") - - sboxProto := android.RuleBuilderSboxProtoForTests(t, foo.Output("lint.sbox.textproto")) - if strings.Contains(*sboxProto.Commands[0].Command, "--baseline") { - t.Error("passed --baseline flag for non existent file") - } -} - -func TestJavaLintRequiresCustomLintFileToExist(t *testing.T) { - android.GroupFixturePreparers( - PrepareForTestWithJavaDefaultModules, - android.PrepareForTestDisallowNonExistentPaths, - ).ExtendWithErrorHandler(android.FixtureExpectsAllErrorsToMatchAPattern([]string{`source path "mybaseline.xml" does not exist`})). - RunTestWithBp(t, ` - java_library { - name: "foo", - srcs: [ - ], - min_sdk_version: "29", - sdk_version: "system_current", - lint: { - baseline_filename: "mybaseline.xml", - }, - } - `) -} - -func TestJavaLintUsesCorrectBpConfig(t *testing.T) { - ctx, _ := testJavaWithFS(t, ` - java_library { - name: "foo", - srcs: [ - "a.java", - "b.java", - "c.java", - ], - min_sdk_version: "29", - sdk_version: "system_current", - lint: { - error_checks: ["SomeCheck"], - baseline_filename: "mybaseline.xml", - }, - } - `, map[string][]byte{ - "mybaseline.xml": nil, - }) - - foo := ctx.ModuleForTests("foo", "android_common") - - sboxProto := android.RuleBuilderSboxProtoForTests(t, foo.Output("lint.sbox.textproto")) - if !strings.Contains(*sboxProto.Commands[0].Command, "--baseline mybaseline.xml") { - t.Error("did not use the correct file for baseline") - } - - if !strings.Contains(*sboxProto.Commands[0].Command, "--error_check NewApi") { - t.Error("should check NewApi errors") - } - - if !strings.Contains(*sboxProto.Commands[0].Command, "--error_check SomeCheck") { - t.Error("should combine NewApi errors with SomeCheck errors") - } -} - func TestGeneratedSources(t *testing.T) { ctx, _ := testJavaWithFS(t, ` java_library { diff --git a/java/lint_test.go b/java/lint_test.go index c257f63ac..eae9faf58 100644 --- a/java/lint_test.go +++ b/java/lint_test.go @@ -15,11 +15,113 @@ package java import ( + "strings" "testing" "android/soong/android" ) +func TestJavaLint(t *testing.T) { + ctx, _ := testJavaWithFS(t, ` + java_library { + name: "foo", + srcs: [ + "a.java", + "b.java", + "c.java", + ], + min_sdk_version: "29", + sdk_version: "system_current", + } + `, map[string][]byte{ + "lint-baseline.xml": nil, + }) + + foo := ctx.ModuleForTests("foo", "android_common") + + sboxProto := android.RuleBuilderSboxProtoForTests(t, foo.Output("lint.sbox.textproto")) + if !strings.Contains(*sboxProto.Commands[0].Command, "--baseline lint-baseline.xml") { + t.Error("did not pass --baseline flag") + } +} + +func TestJavaLintWithoutBaseline(t *testing.T) { + ctx, _ := testJavaWithFS(t, ` + java_library { + name: "foo", + srcs: [ + "a.java", + "b.java", + "c.java", + ], + min_sdk_version: "29", + sdk_version: "system_current", + } + `, map[string][]byte{}) + + foo := ctx.ModuleForTests("foo", "android_common") + + sboxProto := android.RuleBuilderSboxProtoForTests(t, foo.Output("lint.sbox.textproto")) + if strings.Contains(*sboxProto.Commands[0].Command, "--baseline") { + t.Error("passed --baseline flag for non existent file") + } +} + +func TestJavaLintRequiresCustomLintFileToExist(t *testing.T) { + android.GroupFixturePreparers( + PrepareForTestWithJavaDefaultModules, + android.PrepareForTestDisallowNonExistentPaths, + ).ExtendWithErrorHandler(android.FixtureExpectsAllErrorsToMatchAPattern([]string{`source path "mybaseline.xml" does not exist`})). + RunTestWithBp(t, ` + java_library { + name: "foo", + srcs: [ + ], + min_sdk_version: "29", + sdk_version: "system_current", + lint: { + baseline_filename: "mybaseline.xml", + }, + } + `) +} + +func TestJavaLintUsesCorrectBpConfig(t *testing.T) { + ctx, _ := testJavaWithFS(t, ` + java_library { + name: "foo", + srcs: [ + "a.java", + "b.java", + "c.java", + ], + min_sdk_version: "29", + sdk_version: "system_current", + lint: { + error_checks: ["SomeCheck"], + baseline_filename: "mybaseline.xml", + }, + } + `, map[string][]byte{ + "mybaseline.xml": nil, + }) + + foo := ctx.ModuleForTests("foo", "android_common") + + sboxProto := android.RuleBuilderSboxProtoForTests(t, foo.Output("lint.sbox.textproto")) + if !strings.Contains(*sboxProto.Commands[0].Command, "--baseline mybaseline.xml") { + t.Error("did not use the correct file for baseline") + } + + if !strings.Contains(*sboxProto.Commands[0].Command, "--error_check NewApi") { + t.Error("should check NewApi errors") + } + + if !strings.Contains(*sboxProto.Commands[0].Command, "--error_check SomeCheck") { + t.Error("should combine NewApi errors with SomeCheck errors") + } +} + func TestJavaLintBypassUpdatableChecks(t *testing.T) { testCases := []struct { name string From 48de883834c7a20cc3634563591b96b545d3ac69 Mon Sep 17 00:00:00 2001 From: Jaewoong Jung Date: Wed, 21 Apr 2021 16:17:25 -0700 Subject: [PATCH 3/3] Add lint.strict_updatability_linting The flag prevents developers from skipping updatability lint checks with baseline files Test: lint_test.go & manual Bug: 182349282 Change-Id: Ia74a2b83c7ef686124128bdf16f7b85a919d9e8d --- java/lint.go | 10 ++++++++++ java/lint_test.go | 29 +++++++++++++++++++++++++++++ 2 files changed, 39 insertions(+) diff --git a/java/lint.go b/java/lint.go index 0618cbf6d..862c9b4d9 100644 --- a/java/lint.go +++ b/java/lint.go @@ -57,6 +57,9 @@ type LintProperties struct { // Name of the file that lint uses as the baseline. Defaults to "lint-baseline.xml". Baseline_filename *string + + // If true, baselining updatability lint checks (e.g. NewApi) is prohibited. Defaults to false. + Strict_updatability_linting *bool } } @@ -257,6 +260,13 @@ func (l *linter) writeLintProjectXML(ctx android.ModuleContext, rule *android.Ru cmd.FlagForEachArg("--error_check ", l.properties.Lint.Error_checks) cmd.FlagForEachArg("--fatal_check ", l.properties.Lint.Fatal_checks) + if BoolDefault(l.properties.Lint.Strict_updatability_linting, false) { + if baselinePath := l.getBaselineFilepath(ctx); baselinePath.Valid() { + cmd.FlagWithInput("--baseline ", baselinePath.Path()) + cmd.FlagForEachArg("--disallowed_issues ", updatabilityChecks) + } + } + return lintPaths{ projectXML: projectXMLPath, configXML: configXMLPath, diff --git a/java/lint_test.go b/java/lint_test.go index eae9faf58..a253df979 100644 --- a/java/lint_test.go +++ b/java/lint_test.go @@ -173,3 +173,32 @@ func TestJavaLintBypassUpdatableChecks(t *testing.T) { }) } } + +func TestJavaLintStrictUpdatabilityLinting(t *testing.T) { + bp := ` + java_library { + name: "foo", + srcs: [ + "a.java", + ], + min_sdk_version: "29", + sdk_version: "current", + lint: { + strict_updatability_linting: true, + }, + } + ` + fs := android.MockFS{ + "lint-baseline.xml": nil, + } + + result := android.GroupFixturePreparers(PrepareForTestWithJavaDefaultModules, fs.AddToFixture()). + RunTestWithBp(t, bp) + + foo := result.ModuleForTests("foo", "android_common") + sboxProto := android.RuleBuilderSboxProtoForTests(t, foo.Output("lint.sbox.textproto")) + if !strings.Contains(*sboxProto.Commands[0].Command, + "--baseline lint-baseline.xml --disallowed_issues NewApi") { + t.Error("did not restrict baselining NewApi") + } +}