rpm_pfg: allow multiple tree artifacts at the same destination

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

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,10 @@ 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))
 

tests/mappings/mappings_test.bzl

@@ -40,6 +40,7 @@ load(
     "fake_artifact",
     "generic_base_case_test",
     "generic_negative_test",
+    "write_content_manifest",
 )
 
 ##########
@@ -918,6 +919,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"""
@@ -930,6 +975,7 @@ def mappings_analysis_tests():
     # TODO(nacl) migrate the above to use a scheme the one used here.  At the very
     # least, the test suites should be easy to find/name.
     _test_pkg_filegroup(name = "pfg_tests")
+    _test_pkg_files_tree_dest_merge()
 
     native.test_suite(
         name = "pkg_files_analysis_tests",
@@ -968,6 +1014,8 @@ def mappings_analysis_tests():
             ":pkg_mklink_mode_overlay_if_not_provided",
             # Tests involving pkg_filegroup
             ":pfg_tests",
+            # Regression tests
+            ":pf_tree_dest_merge",
         ],
     )
 

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,17 +302,56 @@ 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:
     #
     # - Mutual exclusivity of certain options (low priority)
     #
     _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",
         ],
     )