From cf6311268445d1f15e3d727ec5372751acf4cec2 Mon Sep 17 00:00:00 2001 From: Matt Mackay Date: Mon, 30 May 2022 11:55:33 -0400 Subject: [PATCH] fix: workspace import pathing on py_library --- py/private/py_library.bzl | 35 ++++++++- py/tests/import-pathing/BUILD.bazel | 10 +++ py/tests/import-pathing/tests.bzl | 117 ++++++++++++++++++++++++++++ 3 files changed, 159 insertions(+), 3 deletions(-) create mode 100644 py/tests/import-pathing/BUILD.bazel create mode 100644 py/tests/import-pathing/tests.bzl diff --git a/py/private/py_library.bzl b/py/private/py_library.bzl index d78fd1d5..3fc5b0ff 100644 --- a/py/private/py_library.bzl +++ b/py/private/py_library.bzl @@ -15,16 +15,45 @@ def _make_srcs_depset(ctx): ], ) -def _make_import_path(workspace, base, imp): +def _make_import_path(label, workspace, base, imp): + if imp.startswith("/"): + fail( + "Import path '{imp}' on target {target} is invalid. Absolute paths are not supported.".format( + imp = imp, + target = str(label), + ), + ) + + base_segments = base.split("/") + path_segments = imp.split("/") + + relative_segments = 0 + for segment in path_segments: + if segment == "..": + relative_segments += 1 + else: + break + + # Check if the relative segments that the import path starts with match the number of segments in the base path + # that would break use out of the workspace root. + # The +1 is base_segments would take the path to the root, then one more to escape. + if relative_segments == (len(base_segments) + 1): + fail( + "Import path '{imp}' on target {target} is invalid. Import paths must not escape the workspace root".format( + imp = imp, + target = str(label), + ), + ) + if imp.startswith(".."): - return paths.normalize(paths.join(workspace, *base.split("/")[0:-len(imp.split("/"))])) + return paths.normalize(paths.join(workspace, *(base_segments[0:-relative_segments] + path_segments[relative_segments:]))) else: return paths.normalize(paths.join(workspace, base, imp)) def _make_imports_depset(ctx): base = paths.dirname(ctx.build_file_path) import_paths = [ - _make_import_path(ctx.workspace_name, base, im) + _make_import_path(ctx.label, ctx.workspace_name, base, im) for im in ctx.attr.imports ] diff --git a/py/tests/import-pathing/BUILD.bazel b/py/tests/import-pathing/BUILD.bazel new file mode 100644 index 00000000..6036eec7 --- /dev/null +++ b/py/tests/import-pathing/BUILD.bazel @@ -0,0 +1,10 @@ +load(":tests.bzl", "py_library_import_pathing_test_suite") + +# This is used in the py_library import pathing tests +py_library( + name = "__native_rule_import_list_for_test", + imports = ["baz"], + tags = ["manual"], +) + +py_library_import_pathing_test_suite() diff --git a/py/tests/import-pathing/tests.bzl b/py/tests/import-pathing/tests.bzl new file mode 100644 index 00000000..d7f2f590 --- /dev/null +++ b/py/tests/import-pathing/tests.bzl @@ -0,0 +1,117 @@ +load("@bazel_skylib//lib:unittest.bzl", "analysistest", "asserts", "unittest") +load("//py/private:py_library.bzl", _py_library = "py_library", py_library = "py_library_utils") + +def _ctx_with_imports(imports, deps = []): + return struct( + attr = struct( + deps = deps, + imports = imports, + ), + build_file_path = "foo/bar/BUILD.bazel", + workspace_name = "aspect_rules_py", + label = Label("//foo/bar:baz"), + ) + +def _can_resolve_path_in_workspace_test_impl(ctx): + env = unittest.begin(ctx) + + # The import path is the current package + fake_ctx = _ctx_with_imports(["."]) + imports = py_library.make_imports_depset(fake_ctx).to_list() + asserts.equals(env, "aspect_rules_py/foo/bar", imports[0]) + + # The import path is the parent package + fake_ctx = _ctx_with_imports([".."]) + imports = py_library.make_imports_depset(fake_ctx).to_list() + asserts.equals(env, "aspect_rules_py/foo", imports[0]) + + # The import path is both the current and parent package + fake_ctx = _ctx_with_imports([".", ".."]) + imports = py_library.make_imports_depset(fake_ctx).to_list() + asserts.equals(env, "aspect_rules_py/foo/bar", imports[0]) + asserts.equals(env, "aspect_rules_py/foo", imports[1]) + + # The import path is the current package, but with trailing / + fake_ctx = _ctx_with_imports(["./"]) + imports = py_library.make_imports_depset(fake_ctx).to_list() + asserts.equals(env, "aspect_rules_py/foo/bar", imports[0]) + + # The import path has some other child path in it + fake_ctx = _ctx_with_imports(["./baz"]) + imports = py_library.make_imports_depset(fake_ctx).to_list() + asserts.equals(env, "aspect_rules_py/foo/bar/baz", imports[0]) + + # The import path is at the root + fake_ctx = _ctx_with_imports(["../.."]) + imports = py_library.make_imports_depset(fake_ctx).to_list() + asserts.equals(env, "aspect_rules_py", imports[0]) + + # The relative import path is longer than the path from the root + fake_ctx = _ctx_with_imports(["../some/python/library/imp/path"]) + imports = py_library.make_imports_depset(fake_ctx).to_list() + asserts.equals(env, "aspect_rules_py/foo/some/python/library/imp/path", imports[0]) + + # Transitive imports are included in depset + fake_ctx = _ctx_with_imports([".."], [ctx.attr.import_dep]) + imports = py_library.make_imports_depset(fake_ctx).to_list() + asserts.equals(env, "aspect_rules_py/py/tests/import-pathing/baz", imports[0]) + asserts.equals(env, "aspect_rules_py/foo", imports[1]) + + return unittest.end(env) + +_can_resolve_path_in_workspace_test = unittest.make( + _can_resolve_path_in_workspace_test_impl, + attrs = { + "import_dep": attr.label( + default = "//py/tests/import-pathing:__native_rule_import_list_for_test", + ), + }, +) + +def _fails_on_imp_path_that_breaks_workspace_root_imp(ctx): + env = analysistest.begin(ctx) + asserts.expect_failure(env, "Import paths must not escape the workspace root") + return analysistest.end(env) + +_fails_on_imp_path_that_breaks_workspace_root_test = analysistest.make( + _fails_on_imp_path_that_breaks_workspace_root_imp, + expect_failure = True, +) + +def _fails_on_absolute_imp_impl(ctx): + env = analysistest.begin(ctx) + asserts.expect_failure(env, "Absolute paths are not supported") + return analysistest.end(env) + +_fails_on_absolute_imp_test = analysistest.make( + _fails_on_absolute_imp_impl, + expect_failure = True, +) + +def py_library_import_pathing_test_suite(): + unittest.suite( + "py_library_import_pathing_test_suite", + _can_resolve_path_in_workspace_test, + ) + + _py_library( + name = "__imports_break_out_of_root_lib", + imports = ["../../../.."], + tags = ["manual"], + ) + + _fails_on_imp_path_that_breaks_workspace_root_test( + name = "imp_path_that_breaks_workspace_root", + target_under_test = ":__imports_break_out_of_root_lib", + ) + + _py_library( + name = "__imp_path_is_absolute_lib", + imports = ["/foo"], + tags = ["manual"], + ) + + _fails_on_absolute_imp_test( + name = "imp_path_can_not_be_absolute", + target_under_test = ":__imp_path_is_absolute_lib", + )