From d30ac8a32a9ff28db781db20e6dbe08d0b04b394 Mon Sep 17 00:00:00 2001 From: Jihoon Kang Date: Mon, 9 Oct 2023 18:00:17 +0000 Subject: [PATCH 1/2] Disable full_api_surface_stubs for some java_api_library modules java_api_library modules generated from java_sdk_library get full_api_surface_stubs corresponding to the api surface that the module contributes to. However, modules generated from java_sdk_library where sdk_version is none should not depend on the full api surface stub jar. Test: m --build-from-text-stub Bug: 288624417 Change-Id: I7edda3e6a40f739e805e8719b8d366da765a6933 --- java/sdk_library.go | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/java/sdk_library.go b/java/sdk_library.go index f72c6086f..78e31023f 100644 --- a/java/sdk_library.go +++ b/java/sdk_library.go @@ -1876,6 +1876,14 @@ func (module *SdkLibrary) createApiLibrary(mctx android.DefaultableHookContext, props.Full_api_surface_stub = proptools.StringPtr(apiScope.kind.DefaultJavaLibraryName() + "_full.from-text") } + // java_sdk_library modules that set sdk_version as none does not depend on other api + // domains. Therefore, java_api_library created from such modules should not depend on + // full_api_surface_stubs but create and compile stubs by the java_api_library module + // itself. + if module.SdkVersion(mctx).Kind == android.SdkNone { + props.Full_api_surface_stub = nil + } + props.System_modules = module.deviceProperties.System_modules mctx.CreateModule(ApiLibraryFactory, &props) From 063ec003f98a1cb45ddbda607dd41563d204f85d Mon Sep 17 00:00:00 2001 From: Jihoon Kang Date: Wed, 28 Jun 2023 01:16:23 +0000 Subject: [PATCH 2/2] Add current api check as validation for from-text stub generation Currently, there is no build action ensuring that the API text files are up to date, unless a user runs `m checkapi` or `m update-api`. This means that the user must run `m update-api` after making a change that modifies api surface(s), so that the API text file reflects the local change. This adds additional layer of action to developers, and it is not guaranteed that the developer will always run `m update-api` after making an api surface-affecting changes. To prevent such mistake, this change adds droidstub-level api check as validation for from-text stub generation. With this change, the build will fail if the API text file is not up to date and the user must run `m update-api` when making api surface-affecting local changes. The validation is done by adding all droidstubs modules associated with the java_api_contributions passed to java_api_library via `api_contributions` as dependency and setting the current api timestamp files as the validations for the from-text stubs generating build rule. The full api surface libraries will not run the validations to avoid circular dependency. However, all java_sdk_library generated java_api_library modules will run validations, mapped to the droidstubs in the same api domaion. If the user sets the environment variable `DISABLE_STUB_VALIDATION=true`, validation actions are not run. Validation actions run by default. Test: m nothing --build-from-text-stub and run ninja query to verify `check_current_api.timestamp`s are listed as validation \ DISABLE_STUB_VALIDATION=true m nothing --build-from-text-stub and run ninja query to verify that validation actions are not added Bug: 288624417 Change-Id: I329e6438fe8f3ac30d8c6a971d57853ed6b0d150 --- java/core-libraries/Android.bp | 1 + java/droidstubs.go | 9 ++ java/droidstubs_test.go | 2 +- java/java.go | 43 ++++++- java/java_test.go | 216 +++++++++++++++++++++++++-------- java/sdk_library.go | 2 + 6 files changed, 220 insertions(+), 53 deletions(-) diff --git a/java/core-libraries/Android.bp b/java/core-libraries/Android.bp index 4380f4f5c..8b7387a85 100644 --- a/java/core-libraries/Android.bp +++ b/java/core-libraries/Android.bp @@ -66,6 +66,7 @@ java_api_library { libs: [ "stub-annotations", ], + enable_validation: false, } java_library { diff --git a/java/droidstubs.go b/java/droidstubs.go index 5432ce7f5..8ab2f2aca 100644 --- a/java/droidstubs.go +++ b/java/droidstubs.go @@ -171,6 +171,10 @@ type ApiStubsProvider interface { ApiStubsSrcProvider } +type currentApiTimestampProvider interface { + CurrentApiTimestamp() android.Path +} + // droidstubs passes sources files through Metalava to generate stub .java files that only contain the API to be // documented, filtering out hidden classes and methods. The resulting .java files are intended to be passed to // a droiddoc module to generate documentation. @@ -238,10 +242,15 @@ func (d *Droidstubs) StubsSrcJar() android.Path { return d.stubsSrcJar } +func (d *Droidstubs) CurrentApiTimestamp() android.Path { + return d.checkCurrentApiTimestamp +} + var metalavaMergeAnnotationsDirTag = dependencyTag{name: "metalava-merge-annotations-dir"} var metalavaMergeInclusionAnnotationsDirTag = dependencyTag{name: "metalava-merge-inclusion-annotations-dir"} var metalavaAPILevelsAnnotationsDirTag = dependencyTag{name: "metalava-api-levels-annotations-dir"} var metalavaAPILevelsModuleTag = dependencyTag{name: "metalava-api-levels-module-tag"} +var metalavaCurrentApiTimestampTag = dependencyTag{name: "metalava-current-api-timestamp-tag"} func (d *Droidstubs) DepsMutator(ctx android.BottomUpMutatorContext) { d.Javadoc.addDeps(ctx) diff --git a/java/droidstubs_test.go b/java/droidstubs_test.go index e149b9893..f86e1acef 100644 --- a/java/droidstubs_test.go +++ b/java/droidstubs_test.go @@ -392,7 +392,7 @@ func TestGeneratedApiContributionVisibilityTest(t *testing.T) { removed_api_file: "A/removed.txt", } }, - visibility: ["//a"], + visibility: ["//a", "//b"], } `, map[string][]byte{ diff --git a/java/java.go b/java/java.go index 35fd7c2b8..1cb79433f 100644 --- a/java/java.go +++ b/java/java.go @@ -1664,6 +1664,8 @@ type ApiLibrary struct { extractedSrcJar android.WritablePath // .dex of stubs, used for hiddenapi processing dexJarFile OptionalDexJarPath + + validationPaths android.Paths } type JavaApiLibraryProperties struct { @@ -1699,6 +1701,12 @@ type JavaApiLibraryProperties struct { // The jar will also be passed to metalava as a classpath to // generate compilable stubs. System_modules *string + + // If true, the module runs validation on the API signature files provided + // by the modules passed via api_contributions by checking if the files are + // in sync with the source Java files. However, the environment variable + // DISABLE_STUB_VALIDATION has precedence over this property. + Enable_validation *bool } func ApiLibraryFactory() android.Module { @@ -1787,6 +1795,12 @@ func (al *ApiLibrary) stubsFlags(ctx android.ModuleContext, cmd *android.RuleBui } } +func (al *ApiLibrary) addValidation(ctx android.ModuleContext, cmd *android.RuleBuilderCommand, validationPaths android.Paths) { + for _, validationPath := range validationPaths { + cmd.Validation(validationPath) + } +} + // This method extracts the stub class files from the stub jar file provided // from full_api_surface_stub module instead of compiling the srcjar generated from invoking metalava. // This method is used because metalava can generate compilable from-text stubs only when @@ -1823,8 +1837,28 @@ func (al *ApiLibrary) extractApiSrcs(ctx android.ModuleContext, rule *android.Ru func (al *ApiLibrary) DepsMutator(ctx android.BottomUpMutatorContext) { apiContributions := al.properties.Api_contributions + addValidations := !ctx.Config().IsEnvTrue("DISABLE_STUB_VALIDATION") && + proptools.BoolDefault(al.properties.Enable_validation, true) for _, apiContributionName := range apiContributions { ctx.AddDependency(ctx.Module(), javaApiContributionTag, apiContributionName) + + // Add the java_api_contribution module generating droidstubs module + // as dependency when validation adding conditions are met and + // the java_api_contribution module name has ".api.contribution" suffix. + // All droidstubs-generated modules possess the suffix in the name, + // but there is no such guarantee for tests. + if addValidations { + if strings.HasSuffix(apiContributionName, ".api.contribution") { + ctx.AddDependency(ctx.Module(), metalavaCurrentApiTimestampTag, strings.TrimSuffix(apiContributionName, ".api.contribution")) + } else { + ctx.ModuleErrorf("Validation is enabled for module %s but a "+ + "current timestamp provider is not found for the api "+ + "contribution %s", + ctx.ModuleName(), + apiContributionName, + ) + } + } } ctx.AddVariationDependencies(nil, libTag, al.properties.Libs...) ctx.AddVariationDependencies(nil, staticLibTag, al.properties.Static_libs...) @@ -1862,8 +1896,7 @@ func (al *ApiLibrary) GenerateAndroidBuildActions(ctx android.ModuleContext) { android.PathForModuleOut(ctx, "metalava.sbox.textproto")). SandboxInputs() - var stubsDir android.OptionalPath - stubsDir = android.OptionalPathForPath(android.PathForModuleOut(ctx, "metalava", "stubsDir")) + stubsDir := android.OptionalPathForPath(android.PathForModuleOut(ctx, "metalava", "stubsDir")) rule.Command().Text("rm -rf").Text(stubsDir.String()) rule.Command().Text("mkdir -p").Text(stubsDir.String()) @@ -1895,6 +1928,10 @@ func (al *ApiLibrary) GenerateAndroidBuildActions(ctx android.ModuleContext) { case systemModulesTag: module := dep.(SystemModulesProvider) systemModulesPaths = append(systemModulesPaths, module.HeaderJars()...) + case metalavaCurrentApiTimestampTag: + if currentApiTimestampProvider, ok := dep.(currentApiTimestampProvider); ok { + al.validationPaths = append(al.validationPaths, currentApiTimestampProvider.CurrentApiTimestamp()) + } } }) @@ -1918,6 +1955,8 @@ func (al *ApiLibrary) GenerateAndroidBuildActions(ctx android.ModuleContext) { cmd.FlagWithInput("--migrate-nullness ", previousApi) } + al.addValidation(ctx, cmd, al.validationPaths) + al.stubsSrcJar = android.PathForModuleOut(ctx, "metalava", ctx.ModuleName()+"-"+"stubs.srcjar") al.stubsJarWithoutStaticLibs = android.PathForModuleOut(ctx, "metalava", "stubs.jar") al.stubsJar = android.PathForModuleOut(ctx, ctx.ModuleName(), fmt.Sprintf("%s.jar", ctx.ModuleName())) diff --git a/java/java_test.go b/java/java_test.go index 6b8b735f7..c4fc55bc4 100644 --- a/java/java_test.go +++ b/java/java_test.go @@ -1847,9 +1847,17 @@ func TestDeviceBinaryWrapperGeneration(t *testing.T) { } func TestJavaApiContributionEmptyApiFile(t *testing.T) { - testJavaError(t, + android.GroupFixturePreparers( + prepareForJavaTest, + android.FixtureMergeEnv( + map[string]string{ + "DISABLE_STUB_VALIDATION": "true", + }, + ), + ).ExtendWithErrorHandler(android.FixtureExpectsAtLeastOneErrorMatchingPattern( "Error: foo has an empty api file.", - `java_api_contribution { + )).RunTestWithBp(t, ` + java_api_contribution { name: "foo", } java_api_library { @@ -1874,7 +1882,20 @@ func TestJavaApiLibraryAndProviderLink(t *testing.T) { api_surface: "public", } ` - ctx, _ := testJavaWithFS(t, ` + ctx := android.GroupFixturePreparers( + prepareForJavaTest, + android.FixtureMergeMockFs( + map[string][]byte{ + "a/Android.bp": []byte(provider_bp_a), + "b/Android.bp": []byte(provider_bp_b), + }, + ), + android.FixtureMergeEnv( + map[string]string{ + "DISABLE_STUB_VALIDATION": "true", + }, + ), + ).RunTestWithBp(t, ` java_api_library { name: "bar1", api_surface: "public", @@ -1886,11 +1907,7 @@ func TestJavaApiLibraryAndProviderLink(t *testing.T) { api_surface: "system", api_contributions: ["foo1", "foo2"], } - `, - map[string][]byte{ - "a/Android.bp": []byte(provider_bp_a), - "b/Android.bp": []byte(provider_bp_b), - }) + `) testcases := []struct { moduleName string @@ -1944,7 +1961,22 @@ func TestJavaApiLibraryAndDefaultsLink(t *testing.T) { api_surface: "system", } ` - ctx, _ := testJavaWithFS(t, ` + ctx := android.GroupFixturePreparers( + prepareForJavaTest, + android.FixtureMergeMockFs( + map[string][]byte{ + "a/Android.bp": []byte(provider_bp_a), + "b/Android.bp": []byte(provider_bp_b), + "c/Android.bp": []byte(provider_bp_c), + "d/Android.bp": []byte(provider_bp_d), + }, + ), + android.FixtureMergeEnv( + map[string]string{ + "DISABLE_STUB_VALIDATION": "true", + }, + ), + ).RunTestWithBp(t, ` java_defaults { name: "baz1", api_surface: "public", @@ -1975,13 +2007,7 @@ func TestJavaApiLibraryAndDefaultsLink(t *testing.T) { defaults:["baz1", "baz2"], api_contributions: ["foo4"], } - `, - map[string][]byte{ - "a/Android.bp": []byte(provider_bp_a), - "b/Android.bp": []byte(provider_bp_b), - "c/Android.bp": []byte(provider_bp_c), - "d/Android.bp": []byte(provider_bp_d), - }) + `) testcases := []struct { moduleName string @@ -2026,7 +2052,20 @@ func TestJavaApiLibraryJarGeneration(t *testing.T) { api_surface: "public", } ` - ctx, _ := testJavaWithFS(t, ` + ctx := android.GroupFixturePreparers( + prepareForJavaTest, + android.FixtureMergeMockFs( + map[string][]byte{ + "a/Android.bp": []byte(provider_bp_a), + "b/Android.bp": []byte(provider_bp_b), + }, + ), + android.FixtureMergeEnv( + map[string]string{ + "DISABLE_STUB_VALIDATION": "true", + }, + ), + ).RunTestWithBp(t, ` java_api_library { name: "bar1", api_surface: "public", @@ -2038,11 +2077,7 @@ func TestJavaApiLibraryJarGeneration(t *testing.T) { api_surface: "system", api_contributions: ["foo1", "foo2"], } - `, - map[string][]byte{ - "a/Android.bp": []byte(provider_bp_a), - "b/Android.bp": []byte(provider_bp_b), - }) + `) testcases := []struct { moduleName string @@ -2094,7 +2129,24 @@ func TestJavaApiLibraryLibsLink(t *testing.T) { } ` - ctx, _ := testJavaWithFS(t, ` + ctx := android.GroupFixturePreparers( + prepareForJavaTest, + android.FixtureMergeMockFs( + map[string][]byte{ + "a/Android.bp": []byte(provider_bp_a), + "b/Android.bp": []byte(provider_bp_b), + "c/Android.bp": []byte(lib_bp_a), + "c/Lib.java": {}, + "d/Android.bp": []byte(lib_bp_b), + "d/Lib.java": {}, + }, + ), + android.FixtureMergeEnv( + map[string]string{ + "DISABLE_STUB_VALIDATION": "true", + }, + ), + ).RunTestWithBp(t, ` java_api_library { name: "bar1", api_surface: "public", @@ -2108,15 +2160,7 @@ func TestJavaApiLibraryLibsLink(t *testing.T) { api_contributions: ["foo1", "foo2"], libs: ["lib1", "lib2", "bar1"], } - `, - map[string][]byte{ - "a/Android.bp": []byte(provider_bp_a), - "b/Android.bp": []byte(provider_bp_b), - "c/Android.bp": []byte(lib_bp_a), - "c/Lib.java": {}, - "d/Android.bp": []byte(lib_bp_b), - "d/Lib.java": {}, - }) + `) testcases := []struct { moduleName string @@ -2171,7 +2215,24 @@ func TestJavaApiLibraryStaticLibsLink(t *testing.T) { } ` - ctx, _ := testJavaWithFS(t, ` + ctx := android.GroupFixturePreparers( + prepareForJavaTest, + android.FixtureMergeMockFs( + map[string][]byte{ + "a/Android.bp": []byte(provider_bp_a), + "b/Android.bp": []byte(provider_bp_b), + "c/Android.bp": []byte(lib_bp_a), + "c/Lib.java": {}, + "d/Android.bp": []byte(lib_bp_b), + "d/Lib.java": {}, + }, + ), + android.FixtureMergeEnv( + map[string]string{ + "DISABLE_STUB_VALIDATION": "true", + }, + ), + ).RunTestWithBp(t, ` java_api_library { name: "bar1", api_surface: "public", @@ -2185,15 +2246,7 @@ func TestJavaApiLibraryStaticLibsLink(t *testing.T) { api_contributions: ["foo1", "foo2"], static_libs: ["lib1", "lib2", "bar1"], } - `, - map[string][]byte{ - "a/Android.bp": []byte(provider_bp_a), - "b/Android.bp": []byte(provider_bp_b), - "c/Android.bp": []byte(lib_bp_a), - "c/Lib.java": {}, - "d/Android.bp": []byte(lib_bp_b), - "d/Lib.java": {}, - }) + `) testcases := []struct { moduleName string @@ -2242,19 +2295,28 @@ func TestJavaApiLibraryFullApiSurfaceStub(t *testing.T) { } ` - ctx, _ := testJavaWithFS(t, ` + ctx := android.GroupFixturePreparers( + prepareForJavaTest, + android.FixtureMergeMockFs( + map[string][]byte{ + "a/Android.bp": []byte(provider_bp_a), + "b/Android.bp": []byte(provider_bp_b), + "c/Android.bp": []byte(lib_bp_a), + }, + ), + android.FixtureMergeEnv( + map[string]string{ + "DISABLE_STUB_VALIDATION": "true", + }, + ), + ).RunTestWithBp(t, ` java_api_library { name: "bar1", api_surface: "public", api_contributions: ["foo1"], full_api_surface_stub: "lib1", } - `, - map[string][]byte{ - "a/Android.bp": []byte(provider_bp_a), - "b/Android.bp": []byte(provider_bp_b), - "c/Android.bp": []byte(lib_bp_a), - }) + `) m := ctx.ModuleForTests("bar1", "android_common") manifest := m.Output("metalava.sbox.textproto") @@ -2402,7 +2464,14 @@ func TestHeadersOnly(t *testing.T) { } func TestJavaApiContributionImport(t *testing.T) { - ctx, _ := testJava(t, ` + ctx := android.GroupFixturePreparers( + prepareForJavaTest, + android.FixtureMergeEnv( + map[string]string{ + "DISABLE_STUB_VALIDATION": "true", + }, + ), + ).RunTestWithBp(t, ` java_api_library { name: "foo", api_contributions: ["bar"], @@ -2483,3 +2552,50 @@ func TestSdkLibraryProvidesSystemModulesToApiLibrary(t *testing.T) { classPathFlag := "--classpath __SBOX_SANDBOX_DIR__/out/.intermediates/bar/android_common/turbine-combined/bar.jar" android.AssertStringDoesContain(t, "command expected to contain classpath flag", manifestCommand, classPathFlag) } + +func TestApiLibraryDroidstubsDependency(t *testing.T) { + result := android.GroupFixturePreparers( + prepareForJavaTest, + PrepareForTestWithJavaSdkLibraryFiles, + FixtureWithLastReleaseApis("foo"), + android.FixtureModifyConfig(func(config android.Config) { + config.SetApiLibraries([]string{"foo"}) + }), + android.FixtureMergeMockFs( + map[string][]byte{ + "A.java": nil, + }, + ), + ).RunTestWithBp(t, ` + java_api_library { + name: "foo", + api_contributions: [ + "api-stubs-docs-non-updatable.api.contribution", + ], + enable_validation: true, + } + java_api_library { + name: "bar", + api_contributions: [ + "api-stubs-docs-non-updatable.api.contribution", + ], + enable_validation: false, + } + `) + + currentApiTimestampPath := "api-stubs-docs-non-updatable/android_common/metalava/check_current_api.timestamp" + foo := result.ModuleForTests("foo", "android_common").Module().(*ApiLibrary) + fooValidationPathsString := strings.Join(foo.validationPaths.Strings(), " ") + bar := result.ModuleForTests("bar", "android_common").Module().(*ApiLibrary) + barValidationPathsString := strings.Join(bar.validationPaths.Strings(), " ") + android.AssertStringDoesContain(t, + "Module expected to have validation", + fooValidationPathsString, + currentApiTimestampPath, + ) + android.AssertStringDoesNotContain(t, + "Module expected to not have validation", + barValidationPathsString, + currentApiTimestampPath, + ) +} diff --git a/java/sdk_library.go b/java/sdk_library.go index 78e31023f..041aeb786 100644 --- a/java/sdk_library.go +++ b/java/sdk_library.go @@ -1838,6 +1838,7 @@ func (module *SdkLibrary) createApiLibrary(mctx android.DefaultableHookContext, Static_libs []string Full_api_surface_stub *string System_modules *string + Enable_validation *bool }{} props.Name = proptools.StringPtr(module.apiLibraryModuleName(apiScope)) @@ -1885,6 +1886,7 @@ func (module *SdkLibrary) createApiLibrary(mctx android.DefaultableHookContext, } props.System_modules = module.deviceProperties.System_modules + props.Enable_validation = proptools.BoolPtr(true) mctx.CreateModule(ApiLibraryFactory, &props) }