sandbox should create regular empty files, not symlink to /dev/null.

Adds a test based on our Python rules that makes sure that this actually
fixes the issue. Thanks to @duggelz for the suggestion. It seems like
our Python rules are the only place that actually provides an
EmptyFilesSupplier to Runfiles, so there's probably no simpler way to
test this behavior in an integration test.

Fix #1458. Fix #2394.

--
PiperOrigin-RevId: 148656193
MOS_MIGRATED_REVID=148656193

7 files changed, 119 insertions, 28 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/sandbox/DarwinSandboxedStrategy.java b/src/main/java/com/google/devtools/build/lib/sandbox/DarwinSandboxedStrategy.java
index 44a05f055c..8d464ea171 100644
--- a/src/main/java/com/google/devtools/build/lib/sandbox/DarwinSandboxedStrategy.java
+++ b/src/main/java/com/google/devtools/build/lib/sandbox/DarwinSandboxedStrategy.java
@@ -292,13 +292,20 @@ public class DarwinSandboxedStrategy extends SandboxStrategy {
     }
   }
 
-  private ImmutableMap<PathFragment, Path> finalizeLinks(Map<PathFragment, Path> unfinalized)
+  private Map<PathFragment, Path> finalizeLinks(Map<PathFragment, Path> unfinalized)
       throws IOException {
-    ImmutableMap.Builder<PathFragment, Path> finalizedLinks = new ImmutableMap.Builder<>();
+    HashMap<PathFragment, Path> finalizedLinks = new HashMap<>();
     for (Map.Entry<PathFragment, Path> mount : unfinalized.entrySet()) {
       PathFragment target = mount.getKey();
       Path source = mount.getValue();
 
+      // If the source is null, the target is supposed to be an empty file. In this case we don't
+      // have to deal with finalizing the link.
+      if (source == null) {
+        finalizedLinks.put(target, source);
+        continue;
+      }
+
       FileStatus stat = source.statNullable(Symlinks.NOFOLLOW);
 
       if (stat != null && stat.isDirectory()) {
@@ -311,14 +318,11 @@ public class DarwinSandboxedStrategy extends SandboxStrategy {
         finalizeLinksPath(finalizedLinks, target, source, stat);
       }
     }
-    return finalizedLinks.build();
+    return finalizedLinks;
   }
 
   private void finalizeLinksPath(
-      ImmutableMap.Builder<PathFragment, Path> finalizedMounts,
-      PathFragment target,
-      Path source,
-      FileStatus stat)
+      Map<PathFragment, Path> finalizedMounts, PathFragment target, Path source, FileStatus stat)
       throws IOException {
     // The source must exist.
     Preconditions.checkArgument(stat != null, "%s does not exist", source.toString());
diff --git a/src/main/java/com/google/devtools/build/lib/sandbox/HardlinkedExecRoot.java b/src/main/java/com/google/devtools/build/lib/sandbox/HardlinkedExecRoot.java
index 7ddb401bfd..5bf7c4fd92 100644
--- a/src/main/java/com/google/devtools/build/lib/sandbox/HardlinkedExecRoot.java
+++ b/src/main/java/com/google/devtools/build/lib/sandbox/HardlinkedExecRoot.java
@@ -58,11 +58,11 @@ public class HardlinkedExecRoot implements SandboxExecRoot {
       if (errWriter != null) {
         errWriter.printf("createdir: %s\n", createDir.getPathString());
       }
-      FileSystemUtils.createDirectoryAndParents(createDir);
+      FileSystemUtils.createDirectoryAndParentsWithCache(createdDirs, createDir);
     }
 
     // Link all the inputs.
-    linkInputs(inputs);
+    linkInputs(inputs, createdDirs);
   }
 
   private void createDirectoriesForOutputs(Collection<PathFragment> outputs, Set<Path> createdDirs)
@@ -89,7 +89,8 @@ public class HardlinkedExecRoot implements SandboxExecRoot {
    * names (by following solib symlinks back) to modify the paths to the shared libraries in
    * cc_binaries.
    */
-  private void linkInputs(Map<PathFragment, Path> inputs) throws IOException {
+  private void linkInputs(Map<PathFragment, Path> inputs, Set<Path> createdDirs)
+      throws IOException {
     // Create directory for input files.
     Path inputsDir = sandboxPath.getRelative("inputs");
     if (!inputsDir.exists()) {
@@ -97,6 +98,16 @@ public class HardlinkedExecRoot implements SandboxExecRoot {
     }
 
     for (ImmutableMap.Entry<PathFragment, Path> entry : inputs.entrySet()) {
+      Path targetName = sandboxExecRoot.getRelative(entry.getKey());
+      FileSystemUtils.createDirectoryAndParentsWithCache(
+          createdDirs, targetName.getParentDirectory());
+
+      // The target is supposed to be an empty file.
+      if (entry.getValue() == null) {
+        FileSystemUtils.createEmptyFile(targetName);
+        continue;
+      }
+
       // Hardlink, resolve symlink here instead in finalizeLinks.
       Path target = entry.getValue().resolveSymbolicLinks();
       Path hardlinkName =
@@ -115,12 +126,10 @@ public class HardlinkedExecRoot implements SandboxExecRoot {
       }
 
       // symlink
-      Path symlinkName = sandboxExecRoot.getRelative(entry.getKey());
       if (errWriter != null) {
-        errWriter.printf("symlink: %s -> %s\n", symlinkName, hardlinkName);
+        errWriter.printf("symlink: %s -> %s\n", targetName, hardlinkName);
       }
-      FileSystemUtils.createDirectoryAndParents(symlinkName.getParentDirectory());
-      symlinkName.createSymbolicLink(hardlinkName);
+      targetName.createSymbolicLink(hardlinkName);
     }
   }
 
diff --git a/src/main/java/com/google/devtools/build/lib/sandbox/SpawnHelpers.java b/src/main/java/com/google/devtools/build/lib/sandbox/SpawnHelpers.java
index 8e3c26f93d..12b3fdc3e6 100644
--- a/src/main/java/com/google/devtools/build/lib/sandbox/SpawnHelpers.java
+++ b/src/main/java/com/google/devtools/build/lib/sandbox/SpawnHelpers.java
@@ -127,7 +127,7 @@ public final class SpawnHelpers {
       Path source;
       switch (fields.length) {
         case 1:
-          source = fs.getPath("/dev/null");
+          source = null;
           break;
         case 2:
           source = fs.getPath(fields[1]);
@@ -152,12 +152,12 @@ public final class SpawnHelpers {
       }
       for (Map.Entry<PathFragment, Artifact> mapping : rootAndMappings.getValue().entrySet()) {
         Artifact sourceArtifact = mapping.getValue();
-        PathFragment source =
-            (sourceArtifact != null) ? sourceArtifact.getExecPath() : new PathFragment("/dev/null");
+        Path source =
+            (sourceArtifact != null) ? execRoot.getRelative(sourceArtifact.getExecPath()) : null;
 
         Preconditions.checkArgument(!mapping.getKey().isAbsolute());
         PathFragment target = root.getRelative(mapping.getKey());
-        mounts.put(target, execRoot.getRelative(source));
+        mounts.put(target, source);
       }
     }
   }
diff --git a/src/main/java/com/google/devtools/build/lib/sandbox/SymlinkedExecRoot.java b/src/main/java/com/google/devtools/build/lib/sandbox/SymlinkedExecRoot.java
index bc60eae523..173f1cefa6 100644
--- a/src/main/java/com/google/devtools/build/lib/sandbox/SymlinkedExecRoot.java
+++ b/src/main/java/com/google/devtools/build/lib/sandbox/SymlinkedExecRoot.java
@@ -48,7 +48,7 @@ public final class SymlinkedExecRoot implements SandboxExecRoot {
     cleanFileSystem(inputs.keySet());
     FileSystemUtils.createDirectoryAndParentsWithCache(createdDirs, sandboxExecRoot);
     createParentDirectoriesForInputs(createdDirs, inputs.keySet());
-    createSymlinksForInputs(inputs);
+    createInputs(inputs);
     createWritableDirectories(createdDirs, writableDirs);
     createDirectoriesForOutputs(createdDirs, outputs);
   }
@@ -97,19 +97,25 @@ public final class SymlinkedExecRoot implements SandboxExecRoot {
     }
   }
 
-  private void createSymlinksForInputs(Map<PathFragment, Path> inputs) throws IOException {
+  private void createInputs(Map<PathFragment, Path> inputs) throws IOException {
     // All input files are relative to the execroot.
     for (Entry<PathFragment, Path> entry : inputs.entrySet()) {
       Path key = sandboxExecRoot.getRelative(entry.getKey());
       FileStatus keyStat = key.statNullable(Symlinks.NOFOLLOW);
       if (keyStat != null) {
         if (keyStat.isSymbolicLink()
+            && entry.getValue() != null
             && key.readSymbolicLink().equals(entry.getValue().asFragment())) {
           continue;
         }
         key.delete();
       }
-      key.createSymbolicLink(entry.getValue());
+      // A null value means that we're supposed to create an empty file as the input.
+      if (entry.getValue() != null) {
+        key.createSymbolicLink(entry.getValue());
+      } else {
+        FileSystemUtils.createEmptyFile(key);
+      }
     }
   }
 
diff --git a/src/test/java/com/google/devtools/build/lib/sandbox/LinuxSandboxedStrategyTest.java b/src/test/java/com/google/devtools/build/lib/sandbox/LinuxSandboxedStrategyTest.java
index 3db6c76abd..3db7b36b54 100644
--- a/src/test/java/com/google/devtools/build/lib/sandbox/LinuxSandboxedStrategyTest.java
+++ b/src/test/java/com/google/devtools/build/lib/sandbox/LinuxSandboxedStrategyTest.java
@@ -57,13 +57,11 @@ public class LinuxSandboxedStrategyTest extends SandboxTestCase {
     SpawnHelpers.parseManifestFile(
         fileSystem, mounts, targetDir, manifestFile.getPathFile(), false, "");
 
-    assertThat(mounts)
-        .isEqualTo(
-            ImmutableMap.of(
-                new PathFragment("runfiles/x/testfile"),
-                testFile,
-                new PathFragment("runfiles/x/emptyfile"),
-                fileSystem.getPath("/dev/null")));
+    Map<PathFragment, Path> expected = new HashMap<>();
+    expected.put(new PathFragment("runfiles/x/testfile"), testFile);
+    expected.put(new PathFragment("runfiles/x/emptyfile"), null);
+
+    assertThat(mounts).isEqualTo(expected);
   }
 
   @Test
diff --git a/src/test/shell/integration/BUILD b/src/test/shell/integration/BUILD
index ac8f037d0e..d5528d7fdf 100644
--- a/src/test/shell/integration/BUILD
+++ b/src/test/shell/integration/BUILD
@@ -31,6 +31,13 @@ sh_test(
 )
 
 sh_test(
+    name = "python_test",
+    size = "medium",
+    srcs = ["python_test.sh"],
+    data = [":test-deps"],
+)
+
+sh_test(
     name = "loading_phase_tests",
     size = "large",
     srcs = ["loading_phase_tests.sh"],
diff --git a/src/test/shell/integration/python_test.sh b/src/test/shell/integration/python_test.sh
new file mode 100755
index 0000000000..66804f4b97
--- /dev/null
+++ b/src/test/shell/integration/python_test.sh
@@ -0,0 +1,67 @@
+#!/bin/bash
+#
+# Copyright 2017 The Bazel Authors. All rights reserved.
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#    http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+
+# Load the test setup defined in the parent directory
+CURRENT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
+source "${CURRENT_DIR}/../integration_test_setup.sh" \
+  || { echo "integration_test_setup.sh not found!" >&2; exit 1; }
+
+set -eu
+
+function test_python_binary_empty_files_in_runfiles_are_regular_files() {
+  mkdir -p test/mypackage
+  cat > test/BUILD <<'EOF'
+py_test(
+    name = "a",
+    srcs = [
+        "a.py",
+        "mypackage/b.py",
+    ],
+    main = "a.py"
+)
+EOF
+  cat >test/a.py <<'EOF'
+import os.path
+import sys
+
+print "This is my name: %s" % __file__
+print "This is my working directory: %s" % os.getcwd()
+os.chdir(os.path.dirname(__file__))
+print "This is my new working directory: %s" % os.getcwd()
+
+file_to_check = "mypackage/__init__.py"
+
+if not os.path.exists(file_to_check):
+  print "mypackage/__init__.py does not exist"
+  sys.exit(1)
+
+if os.path.islink(file_to_check):
+  print "mypackage/__init__.py is a symlink, expected a regular file"
+  sys.exit(1)
+
+if not os.path.isfile(file_to_check):
+  print "mypackage/__init__.py is not a regular file"
+  sys.exit(1)
+
+print "OK"
+EOF
+  touch test/mypackage/b.py
+
+  bazel test --test_output=streamed //test:a &> $TEST_log || fail "test failed"
+}
+
+run_suite "Tests for the Python rules"