From 8bbb77d9d3e32b29eccfd66312dc4d213fc24658 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9gis=20Desgroppes?= Date: Fri, 16 Jan 2026 19:33:56 +0100 Subject: [PATCH] feat(pkg_files): preserve external package paths by default MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When no explicit `strip_prefix` is specified, `pkg_files` now automatically preserves package-relative paths for cross-package and cross-repository references. This eliminates the need for manual `strip_prefix` configurations in these common scenarios. **Cross-repository example:** `@external//pkg` → `@//tests:my_files` (contains `tests/testdata/file.txt`) | `strip_prefix` | Result | |-----------------------------------|---------------------| | `strip_prefix.from_root("tests")` | `testdata/file.txt` | | `strip_prefix.from_pkg()` | `testdata/file.txt` | | `None` (earlier default behavior) | `file.txt` | | `None` (new default behavior) | testdata/file.txt` | **Cross-package example:** `//pkg_a` → `//pkg_b:my_files` (contains `pkg_b/subdir/file.txt`) | `strip_prefix` | Result | |-----------------------------------|-------------------| | `strip_prefix.from_root("pkg_b")` | `subdir/file.txt` | | `strip_prefix.from_pkg()` | `subdir/file.txt` | | `None` (earlier default behavior) | `file.txt` | | `None` (new default behavior) | `subdir/file.txt` | Backward compatibility: since direct cross-package target references now preserve package paths instead of basename only, referencing files in a local `filegroup` aggregating cross-package files would bring back basename-only paths. --- pkg/mappings.bzl | 11 +++++- tests/mappings/external_repo/pkg/test.bzl | 45 ++++++++++++++++------- tests/mappings/mappings_test.bzl | 36 ++++++++++++++++++ 3 files changed, 78 insertions(+), 14 deletions(-) diff --git a/pkg/mappings.bzl b/pkg/mappings.bzl index 0e44158ab..825c4b37c 100644 --- a/pkg/mappings.bzl +++ b/pkg/mappings.bzl @@ -227,7 +227,16 @@ def _pkg_files_impl(ctx): file_to_target[f] = src if ctx.attr.strip_prefix == _PKGFILEGROUP_STRIP_ALL: - src_dest_paths_map = {src: paths.join(ctx.attr.prefix, src.basename) for src in srcs} + src_dest_paths_map = {} + for src in srcs: + target = file_to_target[src].label + + # Preserve package path for: 1) external->main repo refs, 2) same-repo cross-package target refs (direct) + path = _path_relative_to_package(src) if ( + (target.repo_name == "" and ctx.label.repo_name != "") or + (target.repo_name == ctx.label.repo_name and target.package != ctx.label.package and target != _owner(src)) + ) else src.basename + src_dest_paths_map[src] = paths.join(ctx.attr.prefix, path) elif ctx.attr.strip_prefix.startswith("/"): # Relative to workspace/repository root src_dest_paths_map = {src: paths.join( diff --git a/tests/mappings/external_repo/pkg/test.bzl b/tests/mappings/external_repo/pkg/test.bzl index c0a771369..9404a0c76 100644 --- a/tests/mappings/external_repo/pkg/test.bzl +++ b/tests/mappings/external_repo/pkg/test.bzl @@ -52,17 +52,36 @@ pkg_files_contents_test = analysistest.make( # Called from the rules_pkg tests def test_referencing_remote_file(name): - pkg_files( - name = "{}_g".format(name), - prefix = "usr/share", - srcs = ["@//tests:loremipsum_txt"], + """Test external package file references with automatic path preservation. + + This test suite verifies that `pkg_files` automatically preserves package-relative paths when referencing files from + an external package. Nested tests should produce the same result. + + Args: + name: Name of the generated test suite, also base name for generated test targets. + """ + tests = [ # The prefix in rules_pkg. Why yes, this is knotty - strip_prefix = strip_prefix.from_root("tests"), - tags = ["manual"], - ) - - pkg_files_contents_test( - name = name, - target_under_test = ":{}_g".format(name), - expected_dests = ["usr/share/testdata/loremipsum.txt"], - ) + struct(name = "{}_strip_prefix_from_root".format(name), strip_prefix = strip_prefix.from_root("tests")), + # ... which corresponds to stripping all directory components up to the package + struct(name = "{}_strip_prefix_from_pkg".format(name), strip_prefix = strip_prefix.from_pkg()), + # ... and should also match the default behavior + struct(name = "{}_auto_strip_prefix".format(name), strip_prefix = None), + ] + + for test in tests: + pkg_files( + name = "{}_g".format(test.name), + prefix = "usr/share", + srcs = ["@//tests:loremipsum_txt"], + strip_prefix = test.strip_prefix, + tags = ["manual"], + ) + + pkg_files_contents_test( + name = test.name, + target_under_test = ":{}_g".format(test.name), + expected_dests = ["usr/share/testdata/loremipsum.txt"], + ) + + native.test_suite(name = name, tests = [":{}".format(test.name) for test in tests]) diff --git a/tests/mappings/mappings_test.bzl b/tests/mappings/mappings_test.bzl index 27a61cba8..5740030d8 100644 --- a/tests/mappings/mappings_test.bzl +++ b/tests/mappings/mappings_test.bzl @@ -140,6 +140,40 @@ def _test_pkg_files_contents(): expected_dests = ["hello.txt"], ) + # Test same-repository cross-package direct target reference: + # When referencing a target from another package, package-relative paths are preserved. + pkg_files( + name = "pf_cross_package_direct_g", + srcs = ["//tests:loremipsum_txt"], # filegroup from different package + tags = ["manual"], + ) + + pkg_files_contents_test( + name = "pf_cross_package_direct", + target_under_test = ":pf_cross_package_direct_g", + expected_dests = ["testdata/loremipsum.txt"], # package-relative path + ) + + # Test same-repository cross-package via local filegroup: + # Aggregation brings back earlier basename-only behavior if desired. + native.filegroup( + name = "loremipsum_via_filegroup", + srcs = ["//tests:loremipsum_txt"], + tags = ["manual"], + ) + + pkg_files( + name = "pf_cross_package_via_filegroup_g", + srcs = [":loremipsum_via_filegroup"], + tags = ["manual"], + ) + + pkg_files_contents_test( + name = "pf_cross_package_via_filegroup", + target_under_test = ":pf_cross_package_via_filegroup_g", + expected_dests = ["loremipsum.txt"], # basename-only path + ) + # Used in the following tests fake_artifact( name = "testdata/test_script", @@ -940,6 +974,8 @@ def mappings_analysis_tests(): # Simple tests ":pf_no_strip_prefix", ":pf_files_only", + ":pf_cross_package_direct", + ":pf_cross_package_via_filegroup", ":pf_strip_testdata_from_pkg", ":pf_strip_prefix_from_root", ":pf_attributes_mode_overlay_if_not_provided",