From b09da7e863392794d3c839319b5301db92b905d9 Mon Sep 17 00:00:00 2001 From: Cole Faust Date: Wed, 18 May 2022 10:57:33 -0700 Subject: [PATCH] Add imports property to py_library rules This is to avoid having it hardcoded in a fork of the py_library rule. Most import attributes should just be set to ".", but our previous solution always hardcoded it to ".." instead, for ndkstubgen. ndkstubgen uses pkg_path: "ndkstubgen", i.e., it set pkg_path to the name of the folder that contained the Android.bp file. In this specific scenario, imports = ".." works. Recreate that behavior here as well, because we don't handle pkg_path properly yet. Fixes: 233081071 Test: build/bazel/ci/bp2build.sh Change-Id: Ib5e6a8edf428c74d4b5947f0ec53a2151001367a --- bp2build/build_conversion_test.go | 2 + bp2build/python_library_conversion_test.go | 72 +++++++++++++++++++++- bp2build/testing.go | 6 +- python/library.go | 36 ++++++++++- 4 files changed, 109 insertions(+), 7 deletions(-) diff --git a/bp2build/build_conversion_test.go b/bp2build/build_conversion_test.go index 0f3ca79b5..0f6e13942 100644 --- a/bp2build/build_conversion_test.go +++ b/bp2build/build_conversion_test.go @@ -1296,6 +1296,7 @@ python_library { "//conditions:default": [], })`, "srcs_version": `"PY3"`, + "imports": `["."]`, }), }, }, @@ -1321,6 +1322,7 @@ python_library { ":reqd", ]`, "srcs_version": `"PY3"`, + "imports": `["."]`, }), }, }, diff --git a/bp2build/python_library_conversion_test.go b/bp2build/python_library_conversion_test.go index 356d52e8c..66c2290ab 100644 --- a/bp2build/python_library_conversion_test.go +++ b/bp2build/python_library_conversion_test.go @@ -2,6 +2,7 @@ package bp2build import ( "fmt" + "strings" "testing" "android/soong/android" @@ -16,6 +17,8 @@ type pythonLibBp2BuildTestCase struct { filesystem map[string]string blueprint string expectedBazelTargets []testBazelTarget + dir string + expectedError error } func convertPythonLibTestCaseToBp2build_Host(tc pythonLibBp2BuildTestCase) bp2buildTestCase { @@ -34,11 +37,19 @@ func convertPythonLibTestCaseToBp2build(tc pythonLibBp2BuildTestCase) bp2buildTe for _, t := range tc.expectedBazelTargets { bp2BuildTargets = append(bp2BuildTargets, makeBazelTarget(t.typ, t.name, t.attrs)) } + // Copy the filesystem so that we can change stuff in it later without it + // affecting the original pythonLibBp2BuildTestCase + filesystemCopy := make(map[string]string) + for k, v := range tc.filesystem { + filesystemCopy[k] = v + } return bp2buildTestCase{ description: tc.description, - filesystem: tc.filesystem, + filesystem: filesystemCopy, blueprint: tc.blueprint, expectedBazelTargets: bp2BuildTargets, + dir: tc.dir, + expectedErr: tc.expectedError, } } @@ -47,6 +58,11 @@ func runPythonLibraryTestCase(t *testing.T, tc pythonLibBp2BuildTestCase) { testCase := convertPythonLibTestCaseToBp2build(tc) testCase.description = fmt.Sprintf(testCase.description, "python_library") testCase.blueprint = fmt.Sprintf(testCase.blueprint, "python_library") + for name, contents := range testCase.filesystem { + if strings.HasSuffix(name, "Android.bp") { + testCase.filesystem[name] = fmt.Sprintf(contents, "python_library") + } + } testCase.moduleTypeUnderTest = "python_library" testCase.moduleTypeUnderTestFactory = python.PythonLibraryFactory @@ -58,6 +74,11 @@ func runPythonLibraryHostTestCase(t *testing.T, tc pythonLibBp2BuildTestCase) { testCase := convertPythonLibTestCaseToBp2build_Host(tc) testCase.description = fmt.Sprintf(testCase.description, "python_library_host") testCase.blueprint = fmt.Sprintf(testCase.blueprint, "python_library_host") + for name, contents := range testCase.filesystem { + if strings.HasSuffix(name, "Android.bp") { + testCase.filesystem[name] = fmt.Sprintf(contents, "python_library_host") + } + } testCase.moduleTypeUnderTest = "python_library_host" testCase.moduleTypeUnderTestFactory = python.PythonLibraryHostFactory runBp2BuildTestCase(t, func(ctx android.RegistrationContext) { @@ -109,6 +130,7 @@ func TestSimplePythonLib(t *testing.T) { "b/d.py", ]`, "srcs_version": `"PY3"`, + "imports": `["."]`, }, }, }, @@ -136,6 +158,7 @@ func TestSimplePythonLib(t *testing.T) { attrs: attrNameToString{ "srcs": `["a.py"]`, "srcs_version": `"PY2"`, + "imports": `["."]`, }, }, }, @@ -163,6 +186,7 @@ func TestSimplePythonLib(t *testing.T) { attrs: attrNameToString{ "srcs": `["a.py"]`, "srcs_version": `"PY3"`, + "imports": `["."]`, }, }, }, @@ -189,11 +213,54 @@ func TestSimplePythonLib(t *testing.T) { typ: "py_library", name: "foo", attrs: attrNameToString{ - "srcs": `["a.py"]`, + "srcs": `["a.py"]`, + "imports": `["."]`, }, }, }, }, + { + description: "%s: pkg_path in a subdirectory of the same name converts correctly", + dir: "mylib/subpackage", + filesystem: map[string]string{ + "mylib/subpackage/a.py": "", + "mylib/subpackage/Android.bp": `%s { + name: "foo", + srcs: ["a.py"], + pkg_path: "mylib/subpackage", + + bazel_module: { bp2build_available: true }, + }`, + }, + blueprint: `%s {name: "bar"}`, + expectedBazelTargets: []testBazelTarget{ + { + // srcs_version is PY2ANDPY3 by default. + typ: "py_library", + name: "foo", + attrs: attrNameToString{ + "srcs": `["a.py"]`, + "imports": `["../.."]`, + "srcs_version": `"PY3"`, + }, + }, + }, + }, + { + description: "%s: pkg_path in a subdirectory of a different name fails", + dir: "mylib/subpackage", + filesystem: map[string]string{ + "mylib/subpackage/a.py": "", + "mylib/subpackage/Android.bp": `%s { + name: "foo", + srcs: ["a.py"], + pkg_path: "mylib/subpackage2", + bazel_module: { bp2build_available: true }, + }`, + }, + blueprint: `%s {name: "bar"}`, + expectedError: fmt.Errorf("Currently, bp2build only supports pkg_paths that are the same as the folders the Android.bp file is in."), + }, } for _, tc := range testCases { @@ -232,6 +299,7 @@ func TestPythonArchVariance(t *testing.T) { "//conditions:default": [], })`, "srcs_version": `"PY3"`, + "imports": `["."]`, }, }, }, diff --git a/bp2build/testing.go b/bp2build/testing.go index 029ba4938..93414956e 100644 --- a/bp2build/testing.go +++ b/bp2build/testing.go @@ -119,8 +119,8 @@ func runBp2BuildTestCase(t *testing.T, registerModuleTypes func(ctx android.Regi return } - errs := append(parseErrs, resolveDepsErrs...) - if tc.expectedErr != nil && checkError(t, errs, tc.expectedErr) { + parseAndResolveErrs := append(parseErrs, resolveDepsErrs...) + if tc.expectedErr != nil && checkError(t, parseAndResolveErrs, tc.expectedErr) { return } @@ -135,7 +135,7 @@ func runBp2BuildTestCase(t *testing.T, registerModuleTypes func(ctx android.Regi if checkError(t, errs, tc.expectedErr) { return } else { - t.Errorf("Expected error: %q, got: %q", tc.expectedErr, errs) + t.Errorf("Expected error: %q, got: %q and %q", tc.expectedErr, errs, parseAndResolveErrs) } } else { android.FailIfErrored(t, errs) diff --git a/python/library.go b/python/library.go index d026c1323..3a2759174 100644 --- a/python/library.go +++ b/python/library.go @@ -17,6 +17,9 @@ package python // This file contains the module types for building Python library. import ( + "path/filepath" + "strings" + "android/soong/android" "android/soong/bazel" @@ -43,6 +46,7 @@ func PythonLibraryHostFactory() android.Module { type bazelPythonLibraryAttributes struct { Srcs bazel.LabelListAttribute Deps bazel.LabelListAttribute + Imports bazel.StringListAttribute Srcs_version *string } @@ -64,16 +68,44 @@ func pythonLibBp2Build(ctx android.TopDownMutatorContext, m *Module) { // do nothing, since python_version defaults to PY2ANDPY3 } + // Bazel normally requires `import path.from.top.of.tree` statements in + // python code, but with soong you can directly import modules from libraries. + // Add "imports" attributes to the bazel library so it matches soong's behavior. + imports := "." + if m.properties.Pkg_path != nil { + // TODO(b/215119317) This is a hack to handle the fact that we don't convert + // pkg_path properly right now. If the folder structure that contains this + // Android.bp file matches pkg_path, we can set imports to an appropriate + // number of ../..s to emulate moving the files under a pkg_path folder. + pkg_path := filepath.Clean(*m.properties.Pkg_path) + if strings.HasPrefix(pkg_path, "/") { + ctx.ModuleErrorf("pkg_path cannot start with a /: %s", pkg_path) + return + } + + if !strings.HasSuffix(ctx.ModuleDir(), "/"+pkg_path) && ctx.ModuleDir() != pkg_path { + ctx.ModuleErrorf("Currently, bp2build only supports pkg_paths that are the same as the folders the Android.bp file is in. pkg_path: %s, module directory: %s", pkg_path, ctx.ModuleDir()) + return + } + numFolders := strings.Count(pkg_path, "/") + 1 + dots := make([]string, numFolders) + for i := 0; i < numFolders; i++ { + dots[i] = ".." + } + imports = strings.Join(dots, "/") + } + baseAttrs := m.makeArchVariantBaseAttributes(ctx) attrs := &bazelPythonLibraryAttributes{ Srcs: baseAttrs.Srcs, Deps: baseAttrs.Deps, Srcs_version: python_version, + Imports: bazel.MakeStringListAttribute([]string{imports}), } props := bazel.BazelTargetModuleProperties{ - Rule_class: "py_library", - Bzl_load_location: "//build/bazel/rules/python:library.bzl", + // Use the native py_library rule. + Rule_class: "py_library", } ctx.CreateBazelTargetModule(props, android.CommonAttributes{