Repository: bazelbuild/bazel
Issue: pkg_tar fails when src file name contains the '=' character
Labels: type: bug
P2
good first issue

### Description of the problem / feature request / question:

Cannot build simple `pkg_tar` target when a file contains the `=` character, e.g. if the file is named `a=b`.

### If possible, provide a minimal example to reproduce the problem:
```
mkdir mydir
touch mydir/a=b

pkg_tar(
    name = "test-tar",
    extension = "tar.gz",
    strip_prefix = "/",
    package_dir = "/home/user",
    srcs = glob(["mydir/**/*"]),
    mode = "0777",
)
```

### Environment info
* Operating System:
Linux x64, Ubuntu 14.04

* Bazel version (output of `bazel info release`):
release 0.7.0

###  Have you found anything relevant by searching the web?

### Anything else, information or logs or outputs that would be helpful?
```
    with open(file_content, 'rb') as f:
IOError: [Errno 2] No such file or directory: 'mydir/a'
Target //prebuilt:test-tar failed to build
```

The `=` character is used as delimiter in `bazel-out/..../test-tar.args` which conflicts with the file name.

Issue comments:
Is this still an issue that needs to be corrected?

If this uses the “zipper” target underneath then yes. There’s also a
separate issue about zipper itself.

On Sat, 11 Aug 2018 at 5:13 Josh Fischer <notifications@github.com> wrote:

> Is this still an issue that needs to be corrected?
>
> —
> You are receiving this because you are subscribed to this thread.
> Reply to this email directly, view it on GitHub
> <https://github.com/bazelbuild/bazel/issues/4063#issuecomment-412244199>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/ABUIF-vVfPFNJfVw0Cx6OG52Fi-BFDaCks5uPj3HgaJpZM4QYdqk>
> .
>

I run into the same problem. I did check the `pkg_tar` rule and I see the it uses [`build_tar.py`](https://github.com/bazelbuild/bazel/blob/master/tools/build_defs/pkg/build_tar.py) script underneath. As @sghiaus mentioned, the script uses `=` as delimiter which conflicts with the file name.
it's interesting to see that the rule/script uses `=` as delimiter for [files](https://github.com/bazelbuild/bazel/blame/master/tools/build_defs/pkg/pkg.bzl#L47), [modes](https://github.com/bazelbuild/bazel/blame/master/tools/build_defs/pkg/pkg.bzl#L57), [owners](https://github.com/bazelbuild/bazel/blame/master/tools/build_defs/pkg/pkg.bzl#L59), and [owner names](https://github.com/bazelbuild/bazel/blame/master/tools/build_defs/pkg/pkg.bzl#L62), but uses colon `:` for [symlinks](https://github.com/bazelbuild/bazel/blame/master/tools/build_defs/pkg/pkg.bzl#L76). 


Pull request review comments:

Patch that resolved the issue:
diff --git a/tools/build_defs/pkg/BUILD b/tools/build_defs/pkg/BUILD
index 2f4052e713c263..8cfac27f0daffa 100644
--- a/tools/build_defs/pkg/BUILD
+++ b/tools/build_defs/pkg/BUILD
@@ -96,6 +96,7 @@ genrule(
     outs = [
         "etc/nsswitch.conf",
         "usr/titi",
+        "a=b",
     ],
     cmd = "for i in $(OUTS); do echo 1 >$$i; done",
 )
@@ -103,6 +104,7 @@ genrule(
 [pkg_tar(
     name = "test-tar-%s" % ext[1:],
     srcs = [
+        ":a=b",
         ":etc/nsswitch.conf",
         ":usr/titi",
     ],
diff --git a/tools/build_defs/pkg/build_tar.py b/tools/build_defs/pkg/build_tar.py
index 90d602d3d353a5..bad4ab6d8903d5 100644
--- a/tools/build_defs/pkg/build_tar.py
+++ b/tools/build_defs/pkg/build_tar.py
@@ -266,7 +266,7 @@ def main(unused_argv):
   mode_map = {}
   if FLAGS.modes:
     for filemode in FLAGS.modes:
-      (f, mode) = filemode.split('=', 1)
+      (f, mode) = filemode.split(':', 1)
       if f[0] == '/':
         f = f[1:]
       mode_map[f] = int(mode, 8)
@@ -277,7 +277,7 @@ def main(unused_argv):
   names_map = {}
   if FLAGS.owner_names:
     for file_owner in FLAGS.owner_names:
-      (f, owner) = file_owner.split('=', 1)
+      (f, owner) = file_owner.split(':', 1)
       (user, group) = owner.split('.', 1)
       if f[0] == '/':
         f = f[1:]
@@ -288,7 +288,7 @@ def main(unused_argv):
   ids_map = {}
   if FLAGS.owners:
     for file_owner in FLAGS.owners:
-      (f, owner) = file_owner.split('=', 1)
+      (f, owner) = file_owner.split(':', 1)
       (user, group) = owner.split('.', 1)
       if f[0] == '/':
         f = f[1:]
@@ -312,7 +312,7 @@ def file_attributes(filename):
       }
 
     for f in FLAGS.file:
-      (inf, tof) = f.split('=', 1)
+      (inf, tof) = f.split(':', 1)
       output.add_file(inf, tof, **file_attributes(tof))
     for f in FLAGS.empty_file:
       output.add_empty_file(f, **file_attributes(f))
diff --git a/tools/build_defs/pkg/build_test.sh b/tools/build_defs/pkg/build_test.sh
index 99bae8e8fcbfa6..a0837a2b8469cb 100755
--- a/tools/build_defs/pkg/build_test.sh
+++ b/tools/build_defs/pkg/build_test.sh
@@ -106,6 +106,7 @@ function get_changes() {
 
 function assert_content() {
   local listing="./
+./a=b
 ./etc/
 ./etc/nsswitch.conf
 ./usr/
@@ -113,8 +114,10 @@ function assert_content() {
 ./usr/bin/
 ./usr/bin/java -> /path/to/bin/java"
   check_eq "$listing" "$(get_tar_listing $1)"
+  check_eq "-rw-r--r--" "$(get_tar_permission $1 ./a=b)"
   check_eq "-rwxr-xr-x" "$(get_tar_permission $1 ./usr/titi)"
   check_eq "-rw-r--r--" "$(get_tar_permission $1 ./etc/nsswitch.conf)"
+  check_eq "42/24" "$(get_numeric_tar_owner $1 ./a=b)"
   check_eq "24/42" "$(get_numeric_tar_owner $1 ./etc/)"
   check_eq "24/42" "$(get_numeric_tar_owner $1 ./etc/nsswitch.conf)"
   check_eq "42/24" "$(get_numeric_tar_owner $1 ./usr/)"
@@ -129,6 +132,7 @@ function assert_content() {
 
 function test_tar() {
   local listing="./
+./a=b
 ./etc/
 ./etc/nsswitch.conf
 ./usr/
@@ -171,6 +175,7 @@ function test_deb() {
     return 0
   fi
   local listing="./
+./a=b
 ./etc/
 ./etc/nsswitch.conf
 ./usr/
@@ -180,6 +185,7 @@ function test_deb() {
   check_eq "$listing" "$(get_deb_listing test-deb.deb)"
   check_eq "-rwxr-xr-x" "$(get_deb_permission test-deb.deb ./usr/titi)"
   check_eq "-rw-r--r--" "$(get_deb_permission test-deb.deb ./etc/nsswitch.conf)"
+  check_eq "-rw-r--r--" "$(get_deb_permission test-deb.deb ./a=b)"
   get_deb_description test-deb.deb >$TEST_log
   expect_log "Description: toto"
   expect_log "Package: titi"
diff --git a/tools/build_defs/pkg/pkg.bzl b/tools/build_defs/pkg/pkg.bzl
index 0e250b261e8283..f9ca70ce176363 100644
--- a/tools/build_defs/pkg/pkg.bzl
+++ b/tools/build_defs/pkg/pkg.bzl
@@ -44,7 +44,7 @@ def _pkg_tar_impl(ctx):
                 file_inputs += run_files
 
     args += [
-        "--file=%s=%s" % (f.path, dest_path(f, data_path))
+        "--file=%s:%s" % (f.path, dest_path(f, data_path))
         for f in file_inputs
     ]
     for target, f_dest_path in ctx.attr.files.items():
@@ -52,14 +52,14 @@ def _pkg_tar_impl(ctx):
         if len(target_files) != 1:
             fail("Inputs to pkg_tar.files_map must describe exactly one file.")
         file_inputs += [target_files[0]]
-        args += ["--file=%s=%s" % (target_files[0].path, f_dest_path)]
+        args += ["--file=%s:%s" % (target_files[0].path, f_dest_path)]
     if ctx.attr.modes:
-        args += ["--modes=%s=%s" % (key, ctx.attr.modes[key]) for key in ctx.attr.modes]
+        args += ["--modes=%s:%s" % (key, ctx.attr.modes[key]) for key in ctx.attr.modes]
     if ctx.attr.owners:
-        args += ["--owners=%s=%s" % (key, ctx.attr.owners[key]) for key in ctx.attr.owners]
+        args += ["--owners=%s:%s" % (key, ctx.attr.owners[key]) for key in ctx.attr.owners]
     if ctx.attr.ownernames:
         args += [
-            "--owner_names=%s=%s" % (key, ctx.attr.ownernames[key])
+            "--owner_names=%s:%s" % (key, ctx.attr.ownernames[key])
             for key in ctx.attr.ownernames
         ]
     if ctx.attr.empty_files:


Test patch: