diff --git a/pkg/private/pkg_files.bzl b/pkg/private/pkg_files.bzl index b80997d7..31e7ae3c 100644 --- a/pkg/private/pkg_files.bzl +++ b/pkg/private/pkg_files.bzl @@ -144,6 +144,12 @@ def _check_dest(content_map, dest, src, origin, allow_duplicates_with_different_ if old_entry.src == src or old_entry.origin == origin: return + # Two tree artifacts mapping to the same destination directory is allowed; + # their contents are merged by the installer and individual file conflicts + # will be caught naturally if the same file path appears twice. + if src != None and src.is_directory and old_entry.src != None and old_entry.src.is_directory: + return + # TODO(#385): This is insufficient but good enough for now. We should # compare over all the attributes too. That will detect problems where # people specify the owner in one place, but another overly broad glob diff --git a/pkg/rpm_pfg.bzl b/pkg/rpm_pfg.bzl index 9533ad08..fe916f0c 100644 --- a/pkg/rpm_pfg.bzl +++ b/pkg/rpm_pfg.bzl @@ -205,11 +205,6 @@ def _make_rpm_filename(rpm_name, version, architecture, package_name = None, rel def _process_files(pfi, origin_label, grouping_label, file_base, rpm_ctx, debuginfo_type): for dest, src in pfi.dest_src_map.items(): metadata = _package_contents_metadata(origin_label, grouping_label) - if dest in rpm_ctx.dest_check_map: - _conflicting_contents_error(dest, metadata, rpm_ctx.dest_check_map[dest]) - else: - rpm_ctx.dest_check_map[dest] = metadata - abs_dest = _make_absolute_if_not_already_or_is_macro(dest) if src.is_directory: # Set aside TreeArtifact information for external processing @@ -224,6 +219,11 @@ def _process_files(pfi, origin_label, grouping_label, file_base, rpm_ctx, debugi "tags": file_base, }) else: + if dest in rpm_ctx.dest_check_map: + _conflicting_contents_error(dest, metadata, rpm_ctx.dest_check_map[dest]) + else: + rpm_ctx.dest_check_map[dest] = metadata + # Files are well-known. Take care of them right here. rpm_ctx.rpm_files_list.append(_FILE_MODE_STANZA_FMT.format(file_base, abs_dest)) diff --git a/tests/mappings/mappings_test.bzl b/tests/mappings/mappings_test.bzl index f663c526..ec357793 100644 --- a/tests/mappings/mappings_test.bzl +++ b/tests/mappings/mappings_test.bzl @@ -41,6 +41,7 @@ load( "fake_artifact", "generic_base_case_test", "generic_negative_test", + "write_content_manifest", ) ########## @@ -985,6 +986,50 @@ def _strip_prefix_test_impl(ctx): strip_prefix_test = unittest.make(_strip_prefix_test_impl) +def _test_pkg_files_tree_dest_merge(): + # Regression test: two tree artifacts mapping to the same destination should + # be allowed. The individual file paths within each tree are disjoint, so + # the merge is valid; actual file conflicts are caught at install time. + directory( + name = "directory_a", + filenames = ["a.h"], + tags = ["manual"], + ) + directory( + name = "directory_b", + filenames = ["b/b.h"], + tags = ["manual"], + ) + pkg_files( + name = "directory_files_a", + srcs = [":directory_a"], + renames = {":directory_a": "include"}, + tags = ["manual"], + ) + pkg_files( + name = "directory_files_b", + srcs = [":directory_b"], + renames = {":directory_b": "include"}, + tags = ["manual"], + ) + pkg_filegroup( + name = "directory_files", + srcs = [ + ":directory_files_a", + ":directory_files_b", + ], + tags = ["manual"], + ) + write_content_manifest( + name = "directory_manifest", + srcs = [":directory_files"], + tags = ["manual"], + ) + generic_base_case_test( + name = "pf_tree_dest_merge", + target_under_test = ":directory_manifest", + ) + # buildifier: disable=unnamed-macro def mappings_analysis_tests(): """Declare mappings.bzl analysis tests""" @@ -999,6 +1044,7 @@ def mappings_analysis_tests(): _test_pkg_filegroup(name = "pfg_tests") _test_pkg_files_package_variables() _test_pkg_filegroup_package_variables() + _test_pkg_files_tree_dest_merge() native.test_suite( name = "pkg_files_analysis_tests", @@ -1040,6 +1086,8 @@ def mappings_analysis_tests(): # Tests for package_variables in prefix ":pf_with_package_variables", ":pfg_with_package_variables", + # Regression tests + ":pf_tree_dest_merge", ], ) diff --git a/tests/rpm/analysis_tests.bzl b/tests/rpm/analysis_tests.bzl index fd3317fb..0babbb64 100644 --- a/tests/rpm/analysis_tests.bzl +++ b/tests/rpm/analysis_tests.bzl @@ -24,7 +24,7 @@ load( ) load("//pkg:providers.bzl", "PackageVariablesInfo") load("//pkg:rpm.bzl", "pkg_rpm") -load("//tests/util:defs.bzl", "generic_base_case_test", "generic_negative_test") +load("//tests/util:defs.bzl", "directory", "generic_base_case_test", "generic_negative_test") def _declare_pkg_rpm(name, srcs_ungrouped, tags = None, **kwargs): pfg_name = "{}_pfg".format(name) @@ -302,6 +302,43 @@ def _test_naming(name): ], ) +def _test_rpm_tree_dest_merge(name): + # Regression test: two tree artifacts mapping to the same RPM destination + # should not raise a conflict error. + directory( + name = "{}_tree_a".format(name), + filenames = ["foo.h"], + tags = ["manual"], + ) + directory( + name = "{}_tree_b".format(name), + filenames = ["bar/bar.h"], + tags = ["manual"], + ) + pkg_files( + name = "{}_pf_a".format(name), + srcs = [":{}_tree_a".format(name)], + renames = {":{}_tree_a".format(name): "include"}, + tags = ["manual"], + ) + pkg_files( + name = "{}_pf_b".format(name), + srcs = [":{}_tree_b".format(name)], + renames = {":{}_tree_b".format(name): "include"}, + tags = ["manual"], + ) + _declare_pkg_rpm( + name = name + "_rpm", + srcs_ungrouped = [ + ":{}_pf_a".format(name), + ":{}_pf_b".format(name), + ], + ) + generic_base_case_test( + name = name, + target_under_test = ":" + name + "_rpm", + ) + def analysis_tests(name): # Need to test: # @@ -309,10 +346,12 @@ def analysis_tests(name): # _test_conflicting_inputs(name = name + "_conflicting_inputs") _test_naming(name = name + "_naming") + _test_rpm_tree_dest_merge(name = name + "_tree_dest_merge") native.test_suite( name = name, tests = [ name + "_conflicting_inputs", name + "_naming", + name + "_tree_dest_merge", ], )