From 7bc1cf508f8d730c852736ab77470a7be9af9988 Mon Sep 17 00:00:00 2001 From: Ulya Trafimovich Date: Tue, 5 Jan 2021 15:41:55 +0000 Subject: [PATCH] Remove obsolete class loader context API and update unit tests. The removed API has been unused since https://r.android.com/1533342 (except for unit tests). Changes in the unit tests reflect the change of API in https://r.android.com/1533342: early errors caused by unknown library paths at CLC construction time have been replaced with late errors at the time of CLC validation. Bug: 132357300 Test: m nothing Change-Id: I739c7c41b6f882b7e28cdd6acd05961d754d8687 --- dexpreopt/class_loader_context.go | 46 ++---------- dexpreopt/class_loader_context_test.go | 97 ++++++++++++-------------- java/app.go | 2 +- java/java.go | 2 +- 4 files changed, 52 insertions(+), 95 deletions(-) diff --git a/dexpreopt/class_loader_context.go b/dexpreopt/class_loader_context.go index ab789aa23..532d8fc06 100644 --- a/dexpreopt/class_loader_context.go +++ b/dexpreopt/class_loader_context.go @@ -255,24 +255,13 @@ const AnySdkVersion int = android.FutureApiLevelInt // Add class loader context for the given library to the map entry for the given SDK version. func (clcMap ClassLoaderContextMap) addContext(ctx android.ModuleInstallPathContext, sdkVer int, lib string, - hostPath, installPath android.Path, strict bool, nestedClcMap ClassLoaderContextMap) error { - - // If missing dependencies are allowed, the build shouldn't fail when a is - // not found. However, this is likely to result is disabling dexpreopt, as it won't be - // possible to construct class loader context without on-host and on-device library paths. - strict = strict && !ctx.Config().AllowMissingDependencies() - - if hostPath == nil && strict { - return fmt.Errorf("unknown build path to \"%s\"", lib) - } + hostPath, installPath android.Path, nestedClcMap ClassLoaderContextMap) error { devicePath := UnknownInstallLibraryPath if installPath == nil { if android.InList(lib, CompatUsesLibs) || android.InList(lib, OptionalCompatUsesLibs) { // Assume that compatibility libraries are installed in /system/framework. installPath = android.PathForModuleInstall(ctx, "framework", lib+".jar") - } else if strict { - return fmt.Errorf("unknown install path to \"%s\"", lib) } else { // For some stub libraries the only known thing is the name of their implementation // library, but the library itself is unavailable (missing or part of a prebuilt). In @@ -310,40 +299,17 @@ func (clcMap ClassLoaderContextMap) addContext(ctx android.ModuleInstallPathCont return nil } -// Wrapper around addContext that reports errors. -func (clcMap ClassLoaderContextMap) addContextOrReportError(ctx android.ModuleInstallPathContext, sdkVer int, lib string, - hostPath, installPath android.Path, strict bool, nestedClcMap ClassLoaderContextMap) { - - err := clcMap.addContext(ctx, sdkVer, lib, hostPath, installPath, strict, nestedClcMap) - if err != nil { - ctx.ModuleErrorf(err.Error()) - } -} - -// Add class loader context. Fail on unknown build/install paths. -func (clcMap ClassLoaderContextMap) AddContext(ctx android.ModuleInstallPathContext, lib string, - hostPath, installPath android.Path) { - - clcMap.addContextOrReportError(ctx, AnySdkVersion, lib, hostPath, installPath, true, nil) -} - -// Add class loader context if the library exists. Don't fail on unknown build/install paths. -func (clcMap ClassLoaderContextMap) MaybeAddContext(ctx android.ModuleInstallPathContext, lib *string, - hostPath, installPath android.Path) { - - if lib != nil { - clcMap.addContextOrReportError(ctx, AnySdkVersion, *lib, hostPath, installPath, false, nil) - } -} - // Add class loader context for the given SDK version. Don't fail on unknown build/install paths, as // libraries with unknown paths still need to be processed by manifest_fixer (which doesn't care // about paths). For the subset of libraries that are used in dexpreopt, their build/install paths // are validated later before CLC is used (in validateClassLoaderContext). -func (clcMap ClassLoaderContextMap) AddContextForSdk(ctx android.ModuleInstallPathContext, sdkVer int, +func (clcMap ClassLoaderContextMap) AddContext(ctx android.ModuleInstallPathContext, sdkVer int, lib string, hostPath, installPath android.Path, nestedClcMap ClassLoaderContextMap) { - clcMap.addContextOrReportError(ctx, sdkVer, lib, hostPath, installPath, false, nestedClcMap) + err := clcMap.addContext(ctx, sdkVer, lib, hostPath, installPath, nestedClcMap) + if err != nil { + ctx.ModuleErrorf(err.Error()) + } } // Merge the other class loader context map into this one, do not override existing entries. diff --git a/dexpreopt/class_loader_context_test.go b/dexpreopt/class_loader_context_test.go index 6b6b1629a..86f7871a0 100644 --- a/dexpreopt/class_loader_context_test.go +++ b/dexpreopt/class_loader_context_test.go @@ -18,6 +18,7 @@ package dexpreopt // For class loader context tests involving .bp files, see TestUsesLibraries in java package. import ( + "fmt" "reflect" "strings" "testing" @@ -50,36 +51,30 @@ func TestCLC(t *testing.T) { m := make(ClassLoaderContextMap) - m.AddContext(ctx, "a", buildPath(ctx, "a"), installPath(ctx, "a")) - m.AddContext(ctx, "b", buildPath(ctx, "b"), installPath(ctx, "b")) - - // "Maybe" variant in the good case: add as usual. - c := "c" - m.MaybeAddContext(ctx, &c, buildPath(ctx, "c"), installPath(ctx, "c")) - - // "Maybe" variant in the bad case: don't add library with unknown name, keep going. - m.MaybeAddContext(ctx, nil, nil, nil) + m.AddContext(ctx, AnySdkVersion, "a", buildPath(ctx, "a"), installPath(ctx, "a"), nil) + m.AddContext(ctx, AnySdkVersion, "b", buildPath(ctx, "b"), installPath(ctx, "b"), nil) + m.AddContext(ctx, AnySdkVersion, "c", buildPath(ctx, "c"), installPath(ctx, "c"), nil) // Add some libraries with nested subcontexts. m1 := make(ClassLoaderContextMap) - m1.AddContext(ctx, "a1", buildPath(ctx, "a1"), installPath(ctx, "a1")) - m1.AddContext(ctx, "b1", buildPath(ctx, "b1"), installPath(ctx, "b1")) + m1.AddContext(ctx, AnySdkVersion, "a1", buildPath(ctx, "a1"), installPath(ctx, "a1"), nil) + m1.AddContext(ctx, AnySdkVersion, "b1", buildPath(ctx, "b1"), installPath(ctx, "b1"), nil) m2 := make(ClassLoaderContextMap) - m2.AddContext(ctx, "a2", buildPath(ctx, "a2"), installPath(ctx, "a2")) - m2.AddContext(ctx, "b2", buildPath(ctx, "b2"), installPath(ctx, "b2")) - m2.AddContextForSdk(ctx, AnySdkVersion, "c2", buildPath(ctx, "c2"), installPath(ctx, "c2"), m1) + m2.AddContext(ctx, AnySdkVersion, "a2", buildPath(ctx, "a2"), installPath(ctx, "a2"), nil) + m2.AddContext(ctx, AnySdkVersion, "b2", buildPath(ctx, "b2"), installPath(ctx, "b2"), nil) + m2.AddContext(ctx, AnySdkVersion, "c2", buildPath(ctx, "c2"), installPath(ctx, "c2"), m1) m3 := make(ClassLoaderContextMap) - m3.AddContext(ctx, "a3", buildPath(ctx, "a3"), installPath(ctx, "a3")) - m3.AddContext(ctx, "b3", buildPath(ctx, "b3"), installPath(ctx, "b3")) + m3.AddContext(ctx, AnySdkVersion, "a3", buildPath(ctx, "a3"), installPath(ctx, "a3"), nil) + m3.AddContext(ctx, AnySdkVersion, "b3", buildPath(ctx, "b3"), installPath(ctx, "b3"), nil) - m.AddContextForSdk(ctx, AnySdkVersion, "d", buildPath(ctx, "d"), installPath(ctx, "d"), m2) + m.AddContext(ctx, AnySdkVersion, "d", buildPath(ctx, "d"), installPath(ctx, "d"), m2) // When the same library is both in conditional and unconditional context, it should be removed // from conditional context. - m.AddContextForSdk(ctx, 42, "f", buildPath(ctx, "f"), installPath(ctx, "f"), nil) - m.AddContextForSdk(ctx, AnySdkVersion, "f", buildPath(ctx, "f"), installPath(ctx, "f"), nil) + m.AddContext(ctx, 42, "f", buildPath(ctx, "f"), installPath(ctx, "f"), nil) + m.AddContext(ctx, AnySdkVersion, "f", buildPath(ctx, "f"), installPath(ctx, "f"), nil) // Merge map with implicit root library that is among toplevel contexts => does nothing. m.AddContextMap(m1, "c") @@ -88,12 +83,12 @@ func TestCLC(t *testing.T) { m.AddContextMap(m3, "m_g") // Compatibility libraries with unknown install paths get default paths. - m.AddContextForSdk(ctx, 29, AndroidHidlManager, buildPath(ctx, AndroidHidlManager), nil, nil) - m.AddContextForSdk(ctx, 29, AndroidHidlBase, buildPath(ctx, AndroidHidlBase), nil, nil) + m.AddContext(ctx, 29, AndroidHidlManager, buildPath(ctx, AndroidHidlManager), nil, nil) + m.AddContext(ctx, 29, AndroidHidlBase, buildPath(ctx, AndroidHidlBase), nil, nil) // Add "android.test.mock" to conditional CLC, observe that is gets removed because it is only // needed as a compatibility library if "android.test.runner" is in CLC as well. - m.AddContextForSdk(ctx, 30, AndroidTestMock, buildPath(ctx, AndroidTestMock), nil, nil) + m.AddContext(ctx, 30, AndroidTestMock, buildPath(ctx, AndroidTestMock), nil, nil) valid, validationError := validateClassLoaderContext(m) @@ -160,31 +155,19 @@ func TestCLC(t *testing.T) { }) } -// Test that an unexpected unknown build path causes immediate error. -func TestCLCUnknownBuildPath(t *testing.T) { - ctx := testContext() - m := make(ClassLoaderContextMap) - err := m.addContext(ctx, AnySdkVersion, "a", nil, nil, true, nil) - checkError(t, err, "unknown build path to \"a\"") -} - -// Test that an unexpected unknown install path causes immediate error. -func TestCLCUnknownInstallPath(t *testing.T) { - ctx := testContext() - m := make(ClassLoaderContextMap) - err := m.addContext(ctx, AnySdkVersion, "a", buildPath(ctx, "a"), nil, true, nil) - checkError(t, err, "unknown install path to \"a\"") -} - -func TestCLCMaybeAdd(t *testing.T) { +// Test that unknown library paths cause a validation error. +func testCLCUnknownPath(t *testing.T, whichPath string) { ctx := testContext() m := make(ClassLoaderContextMap) - a := "a" - m.MaybeAddContext(ctx, &a, nil, nil) + if whichPath == "build" { + m.AddContext(ctx, AnySdkVersion, "a", nil, nil, nil) + } else { + m.AddContext(ctx, AnySdkVersion, "a", buildPath(ctx, "a"), nil, nil) + } // The library should be added to tags by the manifest_fixer. - t.Run("maybe add", func(t *testing.T) { + t.Run("uses libs", func(t *testing.T) { haveUsesLibs := m.UsesLibs() wantUsesLibs := []string{"a"} if !reflect.DeepEqual(wantUsesLibs, haveUsesLibs) { @@ -192,20 +175,28 @@ func TestCLCMaybeAdd(t *testing.T) { } }) - // But class loader context in such cases should raise an error on validation. - t.Run("validate", func(t *testing.T) { - _, err := validateClassLoaderContext(m) - checkError(t, err, "invalid build path for \"a\"") - }) + // But CLC cannot be constructed: there is a validation error. + _, err := validateClassLoaderContext(m) + checkError(t, err, fmt.Sprintf("invalid %s path for \"a\"", whichPath)) +} + +// Test that unknown build path is an error. +func TestCLCUnknownBuildPath(t *testing.T) { + testCLCUnknownPath(t, "build") +} + +// Test that unknown install path is an error. +func TestCLCUnknownInstallPath(t *testing.T) { + testCLCUnknownPath(t, "install") } // An attempt to add conditional nested subcontext should fail. func TestCLCNestedConditional(t *testing.T) { ctx := testContext() m1 := make(ClassLoaderContextMap) - m1.AddContextForSdk(ctx, 42, "a", buildPath(ctx, "a"), installPath(ctx, "a"), nil) + m1.AddContext(ctx, 42, "a", buildPath(ctx, "a"), installPath(ctx, "a"), nil) m := make(ClassLoaderContextMap) - err := m.addContext(ctx, AnySdkVersion, "b", buildPath(ctx, "b"), installPath(ctx, "b"), true, m1) + err := m.addContext(ctx, AnySdkVersion, "b", buildPath(ctx, "b"), installPath(ctx, "b"), m1) checkError(t, err, "nested class loader context shouldn't have conditional part") } @@ -214,10 +205,10 @@ func TestCLCNestedConditional(t *testing.T) { func TestCLCSdkVersionOrder(t *testing.T) { ctx := testContext() m := make(ClassLoaderContextMap) - m.AddContextForSdk(ctx, 28, "a", buildPath(ctx, "a"), installPath(ctx, "a"), nil) - m.AddContextForSdk(ctx, 29, "b", buildPath(ctx, "b"), installPath(ctx, "b"), nil) - m.AddContextForSdk(ctx, 30, "c", buildPath(ctx, "c"), installPath(ctx, "c"), nil) - m.AddContextForSdk(ctx, AnySdkVersion, "d", buildPath(ctx, "d"), installPath(ctx, "d"), nil) + m.AddContext(ctx, 28, "a", buildPath(ctx, "a"), installPath(ctx, "a"), nil) + m.AddContext(ctx, 29, "b", buildPath(ctx, "b"), installPath(ctx, "b"), nil) + m.AddContext(ctx, 30, "c", buildPath(ctx, "c"), installPath(ctx, "c"), nil) + m.AddContext(ctx, AnySdkVersion, "d", buildPath(ctx, "d"), installPath(ctx, "d"), nil) valid, validationError := validateClassLoaderContext(m) diff --git a/java/app.go b/java/app.go index 249313c35..e9597d78a 100755 --- a/java/app.go +++ b/java/app.go @@ -1221,7 +1221,7 @@ func (u *usesLibrary) classLoaderContextForUsesLibDeps(ctx android.ModuleContext if tag, ok := ctx.OtherModuleDependencyTag(m).(usesLibraryDependencyTag); ok { dep := ctx.OtherModuleName(m) if lib, ok := m.(UsesLibraryDependency); ok { - clcMap.AddContextForSdk(ctx, tag.sdkVersion, dep, + clcMap.AddContext(ctx, tag.sdkVersion, dep, lib.DexJarBuildPath(), lib.DexJarInstallPath(), lib.ClassLoaderContexts()) } else if ctx.Config().AllowMissingDependencies() { ctx.AddMissingDependencies([]string{dep}) diff --git a/java/java.go b/java/java.go index 18dd9bda4..145f6c9a5 100644 --- a/java/java.go +++ b/java/java.go @@ -3300,7 +3300,7 @@ func addCLCFromDep(ctx android.ModuleContext, depModule android.Module, } if implicitSdkLib != nil { - clcMap.AddContextForSdk(ctx, dexpreopt.AnySdkVersion, *implicitSdkLib, + clcMap.AddContext(ctx, dexpreopt.AnySdkVersion, *implicitSdkLib, dep.DexJarBuildPath(), dep.DexJarInstallPath(), dep.ClassLoaderContexts()) } else { depName := ctx.OtherModuleName(depModule)