From 17854f5797d10fc04301703e7d0fc7adee09394d Mon Sep 17 00:00:00 2001 From: Spandan Das Date: Fri, 14 Jan 2022 21:19:14 +0000 Subject: [PATCH 1/4] Export LintDepSetsIntf from java package This interface will be used in the apex package to propagate strict_updatability_linting to transitive deps of mainline code Test: In build/soong, go test ./java Bug: 182349282 Change-Id: I30151217e843e4e9fe82db572a066918414ed3a0 --- java/lint.go | 22 +++++++++++----------- java/sdk_library.go | 8 ++++---- 2 files changed, 15 insertions(+), 15 deletions(-) diff --git a/java/lint.go b/java/lint.go index 7845c336b..e97c9c225 100644 --- a/java/lint.go +++ b/java/lint.go @@ -102,12 +102,12 @@ type lintOutputsIntf interface { lintOutputs() *lintOutputs } -type lintDepSetsIntf interface { +type LintDepSetsIntf interface { LintDepSets() LintDepSets // Methods used to propagate strict_updatability_linting values. - getStrictUpdatabilityLinting() bool - setStrictUpdatabilityLinting(bool) + GetStrictUpdatabilityLinting() bool + SetStrictUpdatabilityLinting(bool) } type LintDepSets struct { @@ -158,15 +158,15 @@ func (l *linter) LintDepSets() LintDepSets { return l.outputs.depSets } -func (l *linter) getStrictUpdatabilityLinting() bool { +func (l *linter) GetStrictUpdatabilityLinting() bool { return BoolDefault(l.properties.Lint.Strict_updatability_linting, false) } -func (l *linter) setStrictUpdatabilityLinting(strictLinting bool) { +func (l *linter) SetStrictUpdatabilityLinting(strictLinting bool) { l.properties.Lint.Strict_updatability_linting = &strictLinting } -var _ lintDepSetsIntf = (*linter)(nil) +var _ LintDepSetsIntf = (*linter)(nil) var _ lintOutputsIntf = (*linter)(nil) @@ -273,7 +273,7 @@ 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 l.getStrictUpdatabilityLinting() { + if l.GetStrictUpdatabilityLinting() { // Verify the module does not baseline issues that endanger safe updatability. if baselinePath := l.getBaselineFilepath(ctx); baselinePath.Valid() { cmd.FlagWithInput("--baseline ", baselinePath.Path()) @@ -382,7 +382,7 @@ func (l *linter) lint(ctx android.ModuleContext) { depSetsBuilder := NewLintDepSetBuilder().Direct(html, text, xml) ctx.VisitDirectDepsWithTag(staticLibTag, func(dep android.Module) { - if depLint, ok := dep.(lintDepSetsIntf); ok { + if depLint, ok := dep.(LintDepSetsIntf); ok { depSetsBuilder.Transitive(depLint.LintDepSets()) } }) @@ -660,10 +660,10 @@ func lintZip(ctx android.BuilderContext, paths android.Paths, outputPath android // Enforce the strict updatability linting to all applicable transitive dependencies. func enforceStrictUpdatabilityLintingMutator(ctx android.TopDownMutatorContext) { m := ctx.Module() - if d, ok := m.(lintDepSetsIntf); ok && d.getStrictUpdatabilityLinting() { + if d, ok := m.(LintDepSetsIntf); ok && d.GetStrictUpdatabilityLinting() { ctx.VisitDirectDepsWithTag(staticLibTag, func(d android.Module) { - if a, ok := d.(lintDepSetsIntf); ok { - a.setStrictUpdatabilityLinting(true) + if a, ok := d.(LintDepSetsIntf); ok { + a.SetStrictUpdatabilityLinting(true) } }) } diff --git a/java/sdk_library.go b/java/sdk_library.go index 6a2a7a845..820d9ffc6 100644 --- a/java/sdk_library.go +++ b/java/sdk_library.go @@ -2363,17 +2363,17 @@ func (module *SdkLibraryImport) LintDepSets() LintDepSets { } } -func (module *SdkLibraryImport) getStrictUpdatabilityLinting() bool { +func (module *SdkLibraryImport) GetStrictUpdatabilityLinting() bool { if module.implLibraryModule == nil { return false } else { - return module.implLibraryModule.getStrictUpdatabilityLinting() + return module.implLibraryModule.GetStrictUpdatabilityLinting() } } -func (module *SdkLibraryImport) setStrictUpdatabilityLinting(strictLinting bool) { +func (module *SdkLibraryImport) SetStrictUpdatabilityLinting(strictLinting bool) { if module.implLibraryModule != nil { - module.implLibraryModule.setStrictUpdatabilityLinting(strictLinting) + module.implLibraryModule.SetStrictUpdatabilityLinting(strictLinting) } } From 6677325124321fd375935ce0efa31647cb1d27d4 Mon Sep 17 00:00:00 2001 From: Spandan Das Date: Sat, 15 Jan 2022 00:23:18 +0000 Subject: [PATCH 2/4] Propagate strict_updatability_linting to transitive deps of updatable apexes Create a topdown mutator to walk the deps of updatable apexes. If a dep is lintable, set its strictUpdatabilityLinting to true Creating a new mutator after apexInfoMutator makes it easy to maintain an allowlist (e.g. override_apex does not require an entry in the allowlist, its canonical name can be retrieved using ApexVariationName()) Test: In build/soong/, go test ./apex Test: TH Bug: 182349282 Change-Id: I1b390b0e3a8fb20754ce50c6b253d68d2b3f263b --- apex/apex.go | 19 ++++++++ apex/apex_test.go | 121 ++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 140 insertions(+) diff --git a/apex/apex.go b/apex/apex.go index d12a7865b..776e829f5 100644 --- a/apex/apex.go +++ b/apex/apex.go @@ -76,6 +76,8 @@ func RegisterPostDepsMutators(ctx android.RegisterMutatorsContext) { ctx.BottomUp("apex", apexMutator).Parallel() ctx.BottomUp("apex_directly_in_any", apexDirectlyInAnyMutator).Parallel() ctx.BottomUp("apex_flattened", apexFlattenedMutator).Parallel() + // Register after apex_info mutator so that it can use ApexVariationName + ctx.TopDown("apex_strict_updatability_lint", apexStrictUpdatibilityLintMutator).Parallel() } type apexBundleProperties struct { @@ -1001,6 +1003,23 @@ func apexInfoMutator(mctx android.TopDownMutatorContext) { } } +// apexStrictUpdatibilityLintMutator propagates strict_updatability_linting to transitive deps of a mainline module +// This check is enforced for updatable modules +func apexStrictUpdatibilityLintMutator(mctx android.TopDownMutatorContext) { + if !mctx.Module().Enabled() { + return + } + if apex, ok := mctx.Module().(*apexBundle); ok && apex.Updatable() { + mctx.WalkDeps(func(child, parent android.Module) bool { + if lintable, ok := child.(java.LintDepSetsIntf); ok { + lintable.SetStrictUpdatabilityLinting(true) + } + // visit transitive deps + return true + }) + } +} + // apexUniqueVariationsMutator checks if any dependencies use unique apex variations. If so, use // unique apex variations for this module. See android/apex.go for more about unique apex variant. // TODO(jiyong): move this to android/apex.go? diff --git a/apex/apex_test.go b/apex/apex_test.go index 6d77b0623..9c6f8cbee 100644 --- a/apex/apex_test.go +++ b/apex/apex_test.go @@ -8855,6 +8855,127 @@ func ensureDoesNotContainRequiredDeps(t *testing.T, ctx *android.TestContext, mo } } +func TestApexStrictUpdtabilityLint(t *testing.T) { + bpTemplate := ` + apex { + name: "myapex", + key: "myapex.key", + java_libs: ["myjavalib"], + updatable: %v, + min_sdk_version: "29", + } + apex_key { + name: "myapex.key", + } + java_library { + name: "myjavalib", + srcs: ["MyClass.java"], + apex_available: [ "myapex" ], + lint: { + strict_updatability_linting: %v, + }, + sdk_version: "current", + min_sdk_version: "29", + } + ` + fs := android.MockFS{ + "lint-baseline.xml": nil, + } + + testCases := []struct { + testCaseName string + apexUpdatable bool + javaStrictUpdtabilityLint bool + lintFileExists bool + disallowedFlagExpected bool + }{ + { + testCaseName: "lint-baseline.xml does not exist, no disallowed flag necessary in lint cmd", + apexUpdatable: true, + javaStrictUpdtabilityLint: true, + lintFileExists: false, + disallowedFlagExpected: false, + }, + { + testCaseName: "non-updatable apex respects strict_updatability of javalib", + apexUpdatable: false, + javaStrictUpdtabilityLint: false, + lintFileExists: true, + disallowedFlagExpected: false, + }, + { + testCaseName: "non-updatable apex respects strict updatability of javalib", + apexUpdatable: false, + javaStrictUpdtabilityLint: true, + lintFileExists: true, + disallowedFlagExpected: true, + }, + { + testCaseName: "updatable apex sets strict updatability of javalib to true", + apexUpdatable: true, + javaStrictUpdtabilityLint: false, // will be set to true by mutator + lintFileExists: true, + disallowedFlagExpected: true, + }, + } + + for _, testCase := range testCases { + bp := fmt.Sprintf(bpTemplate, testCase.apexUpdatable, testCase.javaStrictUpdtabilityLint) + fixtures := []android.FixturePreparer{} + if testCase.lintFileExists { + fixtures = append(fixtures, fs.AddToFixture()) + } + + result := testApex(t, bp, fixtures...) + myjavalib := result.ModuleForTests("myjavalib", "android_common_apex29") + sboxProto := android.RuleBuilderSboxProtoForTests(t, myjavalib.Output("lint.sbox.textproto")) + disallowedFlagActual := strings.Contains(*sboxProto.Commands[0].Command, "--baseline lint-baseline.xml --disallowed_issues NewApi") + + if disallowedFlagActual != testCase.disallowedFlagExpected { + t.Errorf("Failed testcase: %v \nActual lint cmd: %v", testCase.testCaseName, *sboxProto.Commands[0].Command) + } + } +} + +// checks transtive deps of an apex coming from bootclasspath_fragment +func TestApexStrictUpdtabilityLintBcpFragmentDeps(t *testing.T) { + bp := ` + apex { + name: "myapex", + key: "myapex.key", + bootclasspath_fragments: ["mybootclasspathfragment"], + updatable: true, + min_sdk_version: "29", + } + apex_key { + name: "myapex.key", + } + bootclasspath_fragment { + name: "mybootclasspathfragment", + contents: ["myjavalib"], + apex_available: ["myapex"], + } + java_library { + name: "myjavalib", + srcs: ["MyClass.java"], + apex_available: [ "myapex" ], + sdk_version: "current", + min_sdk_version: "29", + compile_dex: true, + } + ` + fs := android.MockFS{ + "lint-baseline.xml": nil, + } + + result := testApex(t, bp, dexpreopt.FixtureSetApexBootJars("myapex:myjavalib"), fs.AddToFixture()) + myjavalib := result.ModuleForTests("myjavalib", "android_common_apex29") + sboxProto := android.RuleBuilderSboxProtoForTests(t, myjavalib.Output("lint.sbox.textproto")) + if !strings.Contains(*sboxProto.Commands[0].Command, "--baseline lint-baseline.xml --disallowed_issues NewApi") { + t.Errorf("Strict updabality lint missing in myjavalib coming from bootclasspath_fragment mybootclasspath-fragment\nActual lint cmd: %v", *sboxProto.Commands[0].Command) + } +} + func TestMain(m *testing.M) { os.Exit(m.Run()) } From 08c911f4eb0cff167069b587610182dd74c29a7b Mon Sep 17 00:00:00 2001 From: Spandan Das Date: Fri, 21 Jan 2022 22:07:26 +0000 Subject: [PATCH 3/4] Create allowlist to skip strict updatability lint check As of Jan 2022, some updatable mainline modules have (transitive) deps with NewApi in their respective lint-baseline.xml. Create an allowlist to relax this check temporarily for those mainline modules. Test: m lint-check Test: TH Bug: 182349282 Change-Id: I9ccda6fccb973e9100e31b7e559f5642289ac717 --- apex/apex.go | 23 ++++++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-) diff --git a/apex/apex.go b/apex/apex.go index 776e829f5..51fa0d614 100644 --- a/apex/apex.go +++ b/apex/apex.go @@ -1009,7 +1009,7 @@ func apexStrictUpdatibilityLintMutator(mctx android.TopDownMutatorContext) { if !mctx.Module().Enabled() { return } - if apex, ok := mctx.Module().(*apexBundle); ok && apex.Updatable() { + if apex, ok := mctx.Module().(*apexBundle); ok && apex.checkStrictUpdatabilityLinting() { mctx.WalkDeps(func(child, parent android.Module) bool { if lintable, ok := child.(java.LintDepSetsIntf); ok { lintable.SetStrictUpdatabilityLinting(true) @@ -1020,6 +1020,27 @@ func apexStrictUpdatibilityLintMutator(mctx android.TopDownMutatorContext) { } } +// TODO: b/215736885 Whittle the denylist +// Transitive deps of certain mainline modules baseline NewApi errors +// Skip these mainline modules for now +var ( + skipStrictUpdatabilityLintAllowlist = []string{ + "com.android.art", + "com.android.art.debug", + "com.android.conscrypt", + "com.android.media", + // test apexes + "test_com.android.art", + "test_com.android.conscrypt", + "test_com.android.media", + "test_jitzygote_com.android.art", + } +) + +func (a *apexBundle) checkStrictUpdatabilityLinting() bool { + return a.Updatable() && !android.InList(a.ApexVariationName(), skipStrictUpdatabilityLintAllowlist) +} + // apexUniqueVariationsMutator checks if any dependencies use unique apex variations. If so, use // unique apex variations for this module. See android/apex.go for more about unique apex variant. // TODO(jiyong): move this to android/apex.go? From d9c23abeca23bfe13c65fd4d341a4d6b2b59d5be Mon Sep 17 00:00:00 2001 From: Spandan Das Date: Thu, 10 Feb 2022 02:34:13 +0000 Subject: [PATCH 4/4] Do not propagate strict updatability linting to libcore/ propogation of this flag is determined by the location of the Android.bp where a library is defined. libcore/ libraries that are transtive deps of updatable apexes will - inherit strict_updatability_linting:true if defined in libcore root - not inherit strict_updatability_linting:true if defined in libcore subdir Note that the module can explicitly list `lint:{strict_updatability_linting: true}' in their Android.bp file, in which case they will not be allowed to baseline NewApi errors Test: m lint-check Test: TH Bug: 208656169 Bug: 182349282 Change-Id: I4c373b2960a7af16301d7f06aab448f39b937ea9 --- apex/apex.go | 8 +++++++ apex/apex_test.go | 58 +++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 66 insertions(+) diff --git a/apex/apex.go b/apex/apex.go index 51fa0d614..fc945422c 100644 --- a/apex/apex.go +++ b/apex/apex.go @@ -1011,6 +1011,14 @@ func apexStrictUpdatibilityLintMutator(mctx android.TopDownMutatorContext) { } if apex, ok := mctx.Module().(*apexBundle); ok && apex.checkStrictUpdatabilityLinting() { mctx.WalkDeps(func(child, parent android.Module) bool { + // b/208656169 Do not propagate strict updatability linting to libcore/ + // These libs are available on the classpath during compilation + // These libs are transitive deps of the sdk. See java/sdk.go:decodeSdkDep + // Only skip libraries defined in libcore root, not subdirectories + if mctx.OtherModuleDir(child) == "libcore" { + // Do not traverse transitive deps of libcore/ libs + return false + } if lintable, ok := child.(java.LintDepSetsIntf); ok { lintable.SetStrictUpdatabilityLinting(true) } diff --git a/apex/apex_test.go b/apex/apex_test.go index 9c6f8cbee..1062e758f 100644 --- a/apex/apex_test.go +++ b/apex/apex_test.go @@ -8937,6 +8937,64 @@ func TestApexStrictUpdtabilityLint(t *testing.T) { } } +func TestUpdatabilityLintSkipLibcore(t *testing.T) { + bp := ` + apex { + name: "myapex", + key: "myapex.key", + java_libs: ["myjavalib"], + updatable: true, + min_sdk_version: "29", + } + apex_key { + name: "myapex.key", + } + java_library { + name: "myjavalib", + srcs: ["MyClass.java"], + apex_available: [ "myapex" ], + sdk_version: "current", + min_sdk_version: "29", + } + ` + + testCases := []struct { + testCaseName string + moduleDirectory string + disallowedFlagExpected bool + }{ + { + testCaseName: "lintable module defined outside libcore", + moduleDirectory: "", + disallowedFlagExpected: true, + }, + { + testCaseName: "lintable module defined in libcore root directory", + moduleDirectory: "libcore/", + disallowedFlagExpected: false, + }, + { + testCaseName: "lintable module defined in libcore child directory", + moduleDirectory: "libcore/childdir/", + disallowedFlagExpected: true, + }, + } + + for _, testCase := range testCases { + lintFileCreator := android.FixtureAddTextFile(testCase.moduleDirectory+"lint-baseline.xml", "") + bpFileCreator := android.FixtureAddTextFile(testCase.moduleDirectory+"Android.bp", bp) + result := testApex(t, "", lintFileCreator, bpFileCreator) + myjavalib := result.ModuleForTests("myjavalib", "android_common_apex29") + sboxProto := android.RuleBuilderSboxProtoForTests(t, myjavalib.Output("lint.sbox.textproto")) + cmdFlags := fmt.Sprintf("--baseline %vlint-baseline.xml --disallowed_issues NewApi", testCase.moduleDirectory) + disallowedFlagActual := strings.Contains(*sboxProto.Commands[0].Command, cmdFlags) + + if disallowedFlagActual != testCase.disallowedFlagExpected { + t.Errorf("Failed testcase: %v \nActual lint cmd: %v", testCase.testCaseName, *sboxProto.Commands[0].Command) + } + } +} + // checks transtive deps of an apex coming from bootclasspath_fragment func TestApexStrictUpdtabilityLintBcpFragmentDeps(t *testing.T) { bp := `