Add support for failing on file conflicts. (bazelbuild#683)

* Add support for failing on file conflicts. See bazelbuild#682 for discussion about the appropriate action to take. * Update pkg_files.bzl --------- Co-authored-by: aiuto <[email protected]>
aiuto · Sep 16, 2023 · 2884ce3 · 2884ce3
1 parent 6dd6841
commit 2884ce3
Show file tree

Hide file tree

Showing 4 changed files with 72 additions and 27 deletions.
diff --git a/pkg/install.bzl b/pkg/install.bzl
@@ -29,6 +29,7 @@ def _pkg_install_script_impl(ctx):
     content_map = {}
     for src in ctx.attr.srcs:
         process_src(
+            ctx,
             content_map,
             files_to_run,
             src = src,

diff --git a/pkg/private/pkg_files.bzl b/pkg/private/pkg_files.bzl
@@ -61,7 +61,7 @@ _DestFile = provider(
     },
 )
 
-def _check_dest(content_map, dest, src, origin):
+def _check_dest(ctx, content_map, dest, src, origin):
     old_entry = content_map.get(dest)
     if not old_entry:
         return
@@ -73,23 +73,28 @@ def _check_dest(content_map, dest, src, origin):
     # people specify the owner in one place, but another overly broad glob
     # brings in the file with a different owner.
     if old_entry.src.path != src.path:
-        # buildifier: disable=print
-        print(
-            "Duplicate output path: <%s>, declared in %s and %s" % (
+        msg = "Duplicate output path: <%s>, declared in %s and %s\n  SRC: %s" % (
                 dest,
                 origin,
                 content_map[dest].origin,
-            ),
-            "\n  SRC:",
-            src,
-        )
+                src,
+            )
+        if ctx.attr.allow_duplicates_with_different_content:
+            # buildifier: disable=print
+            print("WARNING:", msg)
+        else:
+            # When we default to this behaviour, we should consider telling
+            # users the attribute to set to deal with this.
+            # For now though, let's not, since they've explicitly opted in.
+            fail(msg)
 
 def _merge_attributes(info, mode, user, group, uid, gid):
     if hasattr(info, "attributes"):
         attrs = info.attributes
         mode = attrs.get("mode") or mode
         user = attrs.get("user") or user
         group = attrs.get("group") or group
+
         new_uid = attrs.get("uid")
         if new_uid != None:
           uid = new_uid
@@ -98,11 +103,11 @@ def _merge_attributes(info, mode, user, group, uid, gid):
           gid = new_gid
     return (mode, user, group, uid, gid)
 
-def _process_pkg_dirs(content_map, pkg_dirs_info, origin, default_mode, default_user, default_group, default_uid, default_gid):
+def _process_pkg_dirs(ctx, content_map, pkg_dirs_info, origin, default_mode, default_user, default_group, default_uid, default_gid):
     attrs = _merge_attributes(pkg_dirs_info, default_mode, default_user, default_group, default_uid, default_gid)
     for dir in pkg_dirs_info.dirs:
         dest = dir.strip("/")
-        _check_dest(content_map, dest, None, origin)
+        _check_dest(ctx, content_map, dest, None, origin)
         content_map[dest] = _DestFile(
             src = None,
             entry_type = ENTRY_IS_DIR,
@@ -114,11 +119,11 @@ def _process_pkg_dirs(content_map, pkg_dirs_info, origin, default_mode, default_
             origin = origin,
         )
 
-def _process_pkg_files(content_map, pkg_files_info, origin, default_mode, default_user, default_group, default_uid, default_gid):
+def _process_pkg_files(ctx, content_map, pkg_files_info, origin, default_mode, default_user, default_group, default_uid, default_gid):
     attrs = _merge_attributes(pkg_files_info, default_mode, default_user, default_group, default_uid, default_gid)
     for filename, src in pkg_files_info.dest_src_map.items():
         dest = filename.strip("/")
-        _check_dest(content_map, dest, src, origin)
+        _check_dest(ctx, content_map, dest, src, origin)
         content_map[dest] = _DestFile(
             src = src,
             entry_type = ENTRY_IS_TREE if src.is_directory else ENTRY_IS_FILE,
@@ -130,10 +135,10 @@ def _process_pkg_files(content_map, pkg_files_info, origin, default_mode, defaul
             origin = origin,
         )
 
-def _process_pkg_symlink(content_map, pkg_symlink_info, origin, default_mode, default_user, default_group, default_uid, default_gid):
+def _process_pkg_symlink(ctx, content_map, pkg_symlink_info, origin, default_mode, default_user, default_group, default_uid, default_gid):
     dest = pkg_symlink_info.destination.strip("/")
     attrs = _merge_attributes(pkg_symlink_info, default_mode, default_user, default_group, default_uid, default_gid)
-    _check_dest(content_map, dest, None, origin)
+    _check_dest(ctx, content_map, dest, None, origin)
     content_map[dest] = _DestFile(
         src = None,
         entry_type = ENTRY_IS_LINK,
@@ -146,18 +151,19 @@ def _process_pkg_symlink(content_map, pkg_symlink_info, origin, default_mode, de
         link_to = pkg_symlink_info.target,
     )
 
-def _process_pkg_filegroup(content_map, pkg_filegroup_info, origin, default_mode, default_user, default_group, default_uid, default_gid):
+def _process_pkg_filegroup(ctx, content_map, pkg_filegroup_info, origin, default_mode, default_user, default_group, default_uid, default_gid):
     if hasattr(pkg_filegroup_info, "pkg_dirs"):
         for d in pkg_filegroup_info.pkg_dirs:
-            _process_pkg_dirs(content_map, d[0], d[1], default_mode, default_user, default_group, default_uid, default_gid)
+            _process_pkg_dirs(ctx, content_map, d[0], d[1], default_mode, default_user, default_group, default_uid, default_gid)
     if hasattr(pkg_filegroup_info, "pkg_files"):
         for pf in pkg_filegroup_info.pkg_files:
-            _process_pkg_files(content_map, pf[0], pf[1], default_mode, default_user, default_group, default_uid, default_gid)
+            _process_pkg_files(ctx, content_map, pf[0], pf[1], default_mode, default_user, default_group, default_uid, default_gid)
     if hasattr(pkg_filegroup_info, "pkg_symlinks"):
         for psl in pkg_filegroup_info.pkg_symlinks:
-            _process_pkg_symlink(content_map, psl[0], psl[1], default_mode, default_user, default_group, default_uid, default_gid)
+            _process_pkg_symlink(ctx, content_map, psl[0], psl[1], default_mode, default_user, default_group, default_uid, default_gid)
 
 def process_src(
+        ctx,
         content_map,
         files,
         src,
@@ -191,6 +197,7 @@ def process_src(
     found_info = False
     if PackageFilesInfo in src:
         _process_pkg_files(
+            ctx,
             content_map,
             src[PackageFilesInfo],
             origin,
@@ -203,6 +210,7 @@ def process_src(
         found_info = True
     if PackageFilegroupInfo in src:
         _process_pkg_filegroup(
+            ctx,
             content_map,
             src[PackageFilegroupInfo],
             origin,
@@ -215,6 +223,7 @@ def process_src(
         found_info = True
     if PackageSymlinkInfo in src:
         _process_pkg_symlink(
+            ctx,
             content_map,
             src[PackageSymlinkInfo],
             origin,
@@ -227,6 +236,7 @@ def process_src(
         found_info = True
     if PackageDirsInfo in src:
         _process_pkg_dirs(
+            ctx,
             content_map,
             src[PackageDirsInfo],
             origin,
@@ -261,10 +271,11 @@ def add_directory(content_map, dir_path, origin, mode = None, user = None, group
         gid = gid,
     )
 
-def add_empty_file(content_map, dest_path, origin, mode = None, user = None, group = None, uid = None, gid = None):
+def add_empty_file(ctx, content_map, dest_path, origin, mode = None, user = None, group = None, uid = None, gid = None):
     """Add a single file to the content map.
 
     Args:
+      ctx: rule context.
       content_map: The content map
       dest_path: Where to place the file in the package.
       origin: The rule instance adding this entry
@@ -273,7 +284,7 @@ def add_empty_file(content_map, dest_path, origin, mode = None, user = None, gro
       group: fallback mode to use for Package*Info elements without group
     """
     dest = dest_path.strip("/")
-    _check_dest(content_map, dest, None, origin)
+    _check_dest(ctx, content_map, dest, None, origin)
     content_map[dest] = _DestFile(
         src = None,
         entry_type = ENTRY_IS_EMPTY_FILE,
@@ -323,6 +334,7 @@ def add_label_list(
 
     for src in srcs:
         if not process_src(
+            ctx,
             content_map,
             file_deps,
             src = src,
@@ -335,6 +347,7 @@ def add_label_list(
         ):
             # Add in the files of srcs which are not pkg_* types
             add_from_default_info(
+                ctx,
                 content_map,
                 file_deps,
                 src,
@@ -349,6 +362,7 @@ def add_label_list(
             )
 
 def add_from_default_info(
+        ctx,
         content_map,
         file_deps,
         src,
@@ -363,6 +377,7 @@ def add_from_default_info(
     """Helper method to add the DefaultInfo of a target to a content_map.
 
     Args:
+      ctx: rule context.
       content_map: (r/w) The content map to update.
       file_deps: (r/w) The list of file Depsets that srcs depend on.
       src: A source object.
@@ -394,6 +409,7 @@ def add_from_default_info(
         else:
             fmode = "0755" if f == the_executable else default_mode
             add_single_file(
+                ctx,
                 content_map,
                 dest_path = d_path,
                 src = f,
@@ -409,7 +425,7 @@ def add_from_default_info(
             for rf in runfiles.files.to_list():
                 d_path = base_path + "/" + rf.short_path
                 fmode = "0755" if rf == the_executable else default_mode
-                _check_dest(content_map, d_path, rf, src.label)
+                _check_dest(ctx, content_map, d_path, rf, src.label)
                 content_map[d_path] = _DestFile(
                     src = rf,
                     entry_type = ENTRY_IS_FILE,
@@ -450,10 +466,12 @@ def get_my_executable(src):
         return ftr.executable
     return None
 
-def add_single_file(content_map, dest_path, src, origin, mode = None, user = None, group = None, uid = None, gid = None):
+
+def add_single_file(ctx, content_map, dest_path, src, origin, mode = None, user = None, group = None, uid = None, gid = None):
     """Add an single file to the content map.
 
     Args:
+      ctx: rule context.
       content_map: The content map
       dest_path: Where to place the file in the package.
       src: Source object. Must have len(src[DefaultInfo].files) == 1
@@ -463,7 +481,7 @@ def add_single_file(content_map, dest_path, src, origin, mode = None, user = Non
       group: fallback mode to use for Package*Info elements without group
     """
     dest = dest_path.strip("/")
-    _check_dest(content_map, dest, src, origin)
+    _check_dest(ctx, content_map, dest, src, origin)
     content_map[dest] = _DestFile(
         src = src,
         entry_type = ENTRY_IS_FILE,
@@ -475,10 +493,12 @@ def add_single_file(content_map, dest_path, src, origin, mode = None, user = Non
         gid = gid,
     )
 
-def add_symlink(content_map, dest_path, src, origin, mode = None, user = None, group = None, uid = None, gid = None):
+def add_symlink(ctx, content_map, dest_path, src, origin, mode = None, user = None, group = None, uid = None, gid = None):
+
     """Add a symlink to the content map.
 
     Args:
+      ctx: rule context.
       content_map: The content map
       dest_path: Where to place the file in the package.
       src: Path to link to.
@@ -488,7 +508,7 @@ def add_symlink(content_map, dest_path, src, origin, mode = None, user = None, g
       group: fallback mode to use for Package*Info elements without group
     """
     dest = dest_path.strip("/")
-    _check_dest(content_map, dest, None, origin)
+    _check_dest(ctx, content_map, dest, None, origin)
     content_map[dest] = _DestFile(
         src = None,
         link_to = src,

diff --git a/pkg/private/tar/tar.bzl b/pkg/private/tar/tar.bzl
@@ -120,6 +120,7 @@ def _pkg_tar_impl(ctx):
     # Start with all the pkg_* inputs
     for src in ctx.attr.srcs:
         if not process_src(
+            ctx,
             content_map,
             file_deps,
             src = src,
@@ -149,7 +150,12 @@ def _pkg_tar_impl(ctx):
                 if f.is_directory:
                     add_tree_artifact(content_map, dest, f, src.label)
                 else:
-                    add_single_file(content_map, dest, f, src.label)
+                    # Note: This extra remap is the bottleneck preventing this
+                    # large block from being a utility method as shown below.
+                    # Should we disallow mixing pkg_files in srcs with remap?
+                    # I am fine with that if it makes the code more readable.
+                    dest = _remap(remap_paths, d_path)
+                    add_single_file(ctx, content_map, dest, f, src.label)
 
     # TODO(aiuto): I want the code to look like this, but we don't have lambdas.
     # transform_path = lambda f: _remap(
@@ -164,6 +170,7 @@ def _pkg_tar_impl(ctx):
             fail("Each input must describe exactly one file.", attr = "files")
         file_deps.append(depset([target_files[0]]))
         add_single_file(
+            ctx,
             content_map,
             f_dest_path,
             target_files[0],
@@ -183,13 +190,14 @@ def _pkg_tar_impl(ctx):
                 "%s=%s" % (_quote(key), ctx.attr.ownernames[key]),
             )
     for empty_file in ctx.attr.empty_files:
-        add_empty_file(content_map, empty_file, ctx.label)
+        add_empty_file(ctx, content_map, empty_file, ctx.label)
     for empty_dir in ctx.attr.empty_dirs or []:
         add_directory(content_map, empty_dir, ctx.label)
     for f in ctx.files.deps:
         args.add("--tar", f.path)
     for link in ctx.attr.symlinks:
         add_symlink(
+            ctx,
             content_map,
             link,
             ctx.attr.symlinks[link],
@@ -279,6 +287,14 @@ The name may contain variables, same as [package_file_name](#package_file_name)"
             doc = "See [Common Attributes](#package_variables)",
             providers = [PackageVariablesInfo],
         ),
+        "allow_duplicates_with_different_content": attr.bool(
+            default=True,
+            doc="""If true, will allow you to reference multiple pkg_* which conflict
+(writing different content or metadata to the same destination).
+Such behaviour is always incorrect, but we provide a flag to support it in case old
+builds were accidentally doing it. Never explicitly set this to true for new code.
+"""
+        ),
         "stamp": attr.int(
             doc = """Enable file time stamping.  Possible values:
 <li>stamp = 1: Use the time of the build as the modification time of each file in the archive.

diff --git a/pkg/private/zip/zip.bzl b/pkg/private/zip/zip.bzl
@@ -142,6 +142,14 @@ The list of compressions is the same as Python's ZipFile: https://docs.python.or
             default = 0,
         ),
 
+        "allow_duplicates_with_different_content": attr.bool(
+            default=True,
+            doc="""If true, will allow you to reference multiple pkg_* which conflict
+(writing different content or metadata to the same destination).
+Such behaviour is always incorrect, but we provide a flag to support it in case old
+builds were accidentally doing it. Never explicitly set this to true for new code.
+"""
+        ),
         # Is --stamp set on the command line?
         # TODO(https://github.com/bazelbuild/rules_pkg/issues/340): Remove this.
         "private_stamp_detect": attr.bool(default = False),