Windows, Bazel client: pass Unix root as JVM flag

The Bazel client will pass the
--host_jvm_args=-Dbazel.windows_unix_root=<path>
flag to the server (computed from $BAZEL_SH), and
the server will no longer shell out to cygpath to
compute this value.

Fixes #2983

Change-Id: Iacc2e2eb70eacafdf7bbcad68d375ba9eadc6ee1
PiperOrigin-RevId: 158830675

src/main/cpp/startup_options.cc

@@ -23,7 +23,6 @@
 #include "src/main/cpp/util/errors.h"
 #include "src/main/cpp/util/exit_code.h"
 #include "src/main/cpp/util/file.h"
-#include "src/main/cpp/util/file_platform.h"
 #include "src/main/cpp/util/numbers.h"
 #include "src/main/cpp/util/strings.h"
 #include "src/main/cpp/workspace_layout.h"
@@ -64,6 +63,14 @@ StartupOptions::StartupOptions(const string &product_name,
     output_root = workspace_layout->GetOutputRoot();
   }
 
+#if defined(COMPILER_MSVC) || defined(__CYGWIN__)
+  string windows_unix_root = WindowsUnixRoot(blaze::GetEnv("BAZEL_SH"));
+  if (!windows_unix_root.empty()) {
+    host_jvm_args.push_back(string("-Dbazel.windows_unix_root=") +
+                            windows_unix_root);
+  }
+#endif  // defined(COMPILER_MSVC) || defined(__CYGWIN__)
+
   const string product_name_lower = GetLowercaseProductName();
   output_user_root = blaze_util::JoinPath(
       output_root, "_" + product_name_lower + "_" + GetUserName());
@@ -432,4 +439,32 @@ blaze_exit_code::ExitCode StartupOptions::AddJVMArguments(
   return blaze_exit_code::SUCCESS;
 }
 
+#if defined(COMPILER_MSVC) || defined(__CYGWIN__)
+// Extract the Windows path of "/" from $BAZEL_SH.
+// $BAZEL_SH usually has the form `<prefix>/usr/bin/bash.exe` or
+// `<prefix>/bin/bash.exe`, and this method returns that `<prefix>` part.
+// If $BAZEL_SH doesn't end with "usr/bin/bash.exe" or "bin/bash.exe" then this
+// method returns an empty string.
+string StartupOptions::WindowsUnixRoot(const string &bazel_sh) {
+  if (bazel_sh.empty()) {
+    return string();
+  }
+  std::pair<string, string> split = blaze_util::SplitPath(bazel_sh);
+  if (blaze_util::AsLower(split.second) != "bash.exe") {
+    return string();
+  }
+  split = blaze_util::SplitPath(split.first);
+  if (blaze_util::AsLower(split.second) != "bin") {
+    return string();
+  }
+
+  std::pair<string, string> split2 = blaze_util::SplitPath(split.first);
+  if (blaze_util::AsLower(split2.second) == "usr") {
+    return split2.first;
+  } else {
+    return split.first;
+  }
+}
+#endif  // defined(COMPILER_MSVC) || defined(__CYGWIN__)
+
 }  // namespace blaze

src/main/cpp/startup_options.h

@@ -230,6 +230,10 @@ class StartupOptions {
  private:
   std::string host_javabase;
   std::string default_host_javabase;
+
+#if defined(COMPILER_MSVC) || defined(__CYGWIN__)
+  static std::string WindowsUnixRoot(const std::string &bazel_sh);
+#endif
 };
 
 }  // namespace blaze

src/main/java/com/google/devtools/build/lib/windows/WindowsFileSystem.java

@@ -27,13 +27,11 @@
 import java.io.File;
 import java.io.FileNotFoundException;
 import java.io.IOException;
-import java.io.InputStream;
-import java.io.InputStreamReader;
 import java.nio.file.Files;
 import java.nio.file.LinkOption;
 import java.nio.file.attribute.DosFileAttributes;
 import java.util.Arrays;
-import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicReference;
 import java.util.regex.Matcher;
 import java.util.regex.Pattern;
 import javax.annotation.Nullable;
@@ -128,32 +126,25 @@ private static Path getCachedChildPathInternalImpl(
         // this heuristic for the valid drives. It's possible that the user has a directory "/a"
         // but no "A:\" drive, so in that case we should prepend the MSYS root.
 
-        // TODO(laszlocsomor): get rid of this heuristic and translate paths using /etc/mtab.
-        // Figure out how to handle non-top-level mount points (e.g. "/usr/bin" is mounted to
-        // "/bin"), which is problematic because Paths are created segment by segment, so
-        // individual Path objects don't know they are parts of a mount point path.
-        // Another challenge is figuring out how and when to read /etc/mtab. A simple approach is
-        // to do it in determineUnixRoot, but then we won't pick up mount changes during the
-        // lifetime of the Bazel server process. A correct approach would be to establish a
-        // Skyframe FileValue-dependency on it, but it's unclear how this class could request or
-        // retrieve Skyframe-computed data.
-        //
-        // Or wait until https://github.com/bazelbuild/bazel/issues/2107 is fixed and forget about
-        // Unix paths altogether. :)
-
         if (WindowsPath.isWindowsVolumeName(child)) {
           child = WindowsPath.getDriveLetter((WindowsPath) parent, child) + ":";
         } else {
-          Preconditions.checkNotNull(
-              UNIX_ROOT,
-              "Could not determine Unix path root or it is not an absolute Windows path. Set the "
-                  + "\"%s\" JVM argument, or export the \"%s\" environment variable for the MSYS "
-                  + "bash and have /usr/bin/cygpath installed. Parent is \"%s\", name is \"%s\".",
-              WINDOWS_UNIX_ROOT_JVM_ARG,
-              BAZEL_SH_ENV_VAR,
-              parent,
-              child);
-          parent = parent.getRelative(UNIX_ROOT);
+          if (UNIX_ROOT.get() == null) {
+            String jvmFlag = "bazel.windows_unix_root";
+            PathFragment value = determineUnixRoot(jvmFlag);
+            if (value == null) {
+              throw new IllegalStateException(
+                  String.format(
+                      "\"%1$s\" JVM flag is not set. Use the --host_jvm_args flag or export the "
+                          + "BAZEL_SH environment variable. For example "
+                          + "\"--host_jvm_args=-D%1$s=c:/tools/msys64\" or "
+                          + "\"set BAZEL_SH=c:/tools/msys64/usr/bin/bash.exe\". "
+                          + "parent=(%2$s) name=(%3$s)",
+                      jvmFlag, parent, child));
+            }
+            UNIX_ROOT.set(value);
+          }
+          parent = parent.getRelative(UNIX_ROOT.get());
         }
       }
 
@@ -305,7 +296,7 @@ private static char getDriveLetter(WindowsPath parent, String name) {
 
     @VisibleForTesting
     @Override
-    protected void applyToChildren(Predicate<Path> function) {
+    protected synchronized void applyToChildren(Predicate<Path> function) {
       super.applyToChildren(function);
     }
   }
@@ -315,13 +306,7 @@ static PathFactory getPathFactoryForTesting(Function<String, String> mockResolve
     return WindowsPathFactory.createForTesting(mockResolver);
   }
 
-  private static final String WINDOWS_UNIX_ROOT_JVM_ARG = "bazel.windows_unix_root";
-  private static final String BAZEL_SH_ENV_VAR = "BAZEL_SH";
-
-  // Absolute Windows path specifying the root of absolute Unix paths.
-  // This is typically the MSYS installation root, e.g. C:\\tools\\msys64
-  private static final PathFragment UNIX_ROOT =
-      determineUnixRoot(WINDOWS_UNIX_ROOT_JVM_ARG, BAZEL_SH_ENV_VAR);
+  private static final AtomicReference<PathFragment> UNIX_ROOT = new AtomicReference<>(null);
 
   public static final LinkOption[] NO_OPTIONS = new LinkOption[0];
   public static final LinkOption[] NO_FOLLOW = new LinkOption[] {LinkOption.NOFOLLOW_LINKS};
@@ -491,55 +476,21 @@ private static DosFileAttributes getAttribs(File file, boolean followSymlinks)
         file.toPath(), DosFileAttributes.class, symlinkOpts(followSymlinks));
   }
 
-  private static PathFragment determineUnixRoot(String jvmArgName, String bazelShEnvVar) {
-    // Get the path from a JVM argument, if specified.
+  private static PathFragment determineUnixRoot(String jvmArgName) {
+    // Get the path from a JVM flag, if specified.
     String path = System.getProperty(jvmArgName);
-
-    if (path == null || path.isEmpty()) {
-      path = "";
-
-      // Fall back to executing cygpath.
-      String bash = System.getenv(bazelShEnvVar);
-      Process process = null;
-      try {
-        process = Runtime.getRuntime().exec("cmd.exe /C " + bash + " -c \"/usr/bin/cygpath -m /\"");
-
-        // Wait 3 seconds max, that should be enough to run this command.
-        process.waitFor(3, TimeUnit.SECONDS);
-
-        if (process.exitValue() == 0) {
-          path = readAll(process.getInputStream());
-        } else {
-          System.err.print(
-              String.format(
-                  "ERROR: %s (exit code: %d)%n",
-                  readAll(process.getErrorStream()), process.exitValue()));
-        }
-      } catch (InterruptedException | IOException e) {
-        // Silently ignore failure. Either MSYS is installed at a different location, or not
-        // installed at all, or some error occurred. We can't do anything anymore but throw an
-        // exception if someone tries to create a Path from an absolute Unix path.
-        return null;
-      }
+    if (path == null) {
+      return null;
     }
 
     path = path.trim();
-    PathFragment result = PathFragment.create(path);
-    if (path.isEmpty() || result.getDriveLetter() == '\0' || !result.isAbsolute()) {
+    if (path.isEmpty()) {
       return null;
-    } else {
-      return result;
     }
-  }
 
-  private static String readAll(InputStream s) throws IOException {
-    String result = "";
-    int len;
-    char[] buf = new char[4096];
-    try (InputStreamReader r = new InputStreamReader(s)) {
-      while ((len = r.read(buf)) > 0) {
-        result += new String(buf, 0, len);
-      }
+    PathFragment result = PathFragment.create(path);
+    if (result.getDriveLetter() == '\0' || !result.isAbsolute()) {
+      return null;
     }
     return result;
   }

src/test/py/bazel/BUILD

@@ -32,3 +32,26 @@ py_test(
     srcs = ["bazel_clean_test.py"],
     deps = [":test_base"],
 )
+
+py_test(
+    name = "bazel_windows_test",
+    size = "medium",
+    srcs = select({
+        "//src:windows": ["bazel_windows_test.py"],
+        "//src:windows_msvc": ["bazel_windows_test.py"],
+        "//src:windows_msys": ["bazel_windows_test.py"],
+        "//conditions:default": ["empty_test.py"],
+    }),
+    main = select({
+        "//src:windows": "bazel_windows_test.py",
+        "//src:windows_msvc": "bazel_windows_test.py",
+        "//src:windows_msys": "bazel_windows_test.py",
+        "//conditions:default": "empty_test.py",
+    }),
+    deps = select({
+        "//src:windows": [":test_base"],
+        "//src:windows_msvc": [":test_base"],
+        "//src:windows_msys": [":test_base"],
+        "//conditions:default": [],
+    }),
+)

src/test/py/bazel/bazel_windows_test.py

@@ -0,0 +1,50 @@
+# 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.
+
+import unittest
+from src.test.py.bazel import test_base
+
+
+class BazelWindowsTest(test_base.TestBase):
+
+  def testWindowsUnixRoot(self):
+    self.ScratchFile('WORKSPACE')
+    self.ScratchFile('foo/BUILD', ['cc_binary(name="x", srcs=["x.cc"])'])
+    self.ScratchFile('foo/x.cc', [
+        '#include <stdio.h>', 'int main(int, char**) {'
+        '  printf("hello\\n");', '  return 0;', '}'
+    ])
+
+    exit_code, _, stderr = self.RunBazel(
+        ['--batch', 'build', '//foo:x', '--cpu=x64_windows_msys'],
+        env_remove={'BAZEL_SH'})
+    self.assertEqual(exit_code, 2)
+    self.assertIn('\'BAZEL_SH\' environment variable is not set',
+                  '\n'.join(stderr))
+
+    exit_code, _, stderr = self.RunBazel([
+        '--batch', '--host_jvm_args=-Dbazel.windows_unix_root=', 'build',
+        '//foo:x', '--cpu=x64_windows_msys'
+    ])
+    self.assertEqual(exit_code, 37)
+    self.assertIn('"bazel.windows_unix_root" JVM flag is not set',
+                  '\n'.join(stderr))
+
+    exit_code, _, _ = self.RunBazel(
+        ['--batch', 'build', '//foo:x', '--cpu=x64_windows_msys'])
+    self.assertEqual(exit_code, 0)
+
+
+if __name__ == '__main__':
+  unittest.main()

src/test/py/bazel/empty_test.py

@@ -0,0 +1,27 @@
+# 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.
+#
+# Empty test for platforms that don't need to run a particular test.
+
+import unittest
+
+
+class EmptyTest(unittest.TestCase):
+
+  def testNothing(self):
+    pass
+
+
+if __name__ == '__main__':
+  unittest.main()

src/test/py/bazel/test_base.py

@@ -117,7 +117,9 @@ def ScratchDir(self, path):
     if not path:
       return
     abspath = self.Path(path)
-    if os.path.exists(abspath) and not os.path.isdir(abspath):
+    if os.path.exists(abspath):
+      if os.path.isdir(abspath):
+        return
       raise IOError('"%s" (%s) exists and is not a directory' % (path, abspath))
     os.makedirs(abspath)
 
@@ -145,11 +147,13 @@ def ScratchFile(self, path, lines=None):
           f.write(l)
           f.write('\n')
 
-  def RunBazel(self, args):
+  def RunBazel(self, args, env_remove=None):
     """Runs "bazel <args>", waits for it to exit.
 
     Args:
       args: [string]; flags to pass to bazel (e.g. ['--batch', 'build', '//x'])
+      env_remove: set(string); optional; environment variables to NOT pass to
+        Bazel
     Returns:
       (int, [string], [string]) tuple: exit code, stdout lines, stderr lines
     """
@@ -163,7 +167,7 @@ def RunBazel(self, args):
             stdout=stdout,
             stderr=stderr,
             cwd=self._tests_root,
-            env=self._BazelEnvMap())
+            env=self._BazelEnvMap(env_remove))
         exit_code = proc.wait()
 
     with open(self._stdout, 'r') as f:
@@ -172,7 +176,7 @@ def RunBazel(self, args):
       stderr = [l.strip() for l in f.readlines()]
     return exit_code, stdout, stderr
 
-  def _BazelEnvMap(self):
+  def _BazelEnvMap(self, env_remove=None):
     """Returns the environment variable map to run Bazel."""
     if TestBase.IsWindows():
       result = []
@@ -190,12 +194,13 @@ def _Visit(result, _, names):
             names.pop()
 
         os.path.walk('c:\\program files\\java\\', _Visit, result)
+
       env = {
           'SYSTEMROOT': TestBase.GetEnv('SYSTEMROOT'),
           # TODO(laszlocsomor): Let Bazel pass BAZEL_SH and JAVA_HOME to tests
           # and use those here instead of hardcoding paths.
           'JAVA_HOME': 'c:\\program files\\java\\' + sorted(result)[-1],
-          'BAZEL_SH': 'c:\\tools\\msys64\\usr\\bin\\bash.exe'
+          'BAZEL_SH': 'c:\\tools\\msys64\\usr\\bin\\bash.exe',
       }
     else:
       env = {'HOME': os.path.join(self._temp, 'home')}
@@ -206,6 +211,9 @@ def _Visit(result, _, names):
     # that by checking for TEST_TMPDIR.
     env['TEST_TMPDIR'] = TestBase.GetEnv('TEST_TMPDIR')
     env['TMP'] = self._temp
+    if env_remove:
+      for e in env_remove:
+        del env[e]
     return env
 
   @staticmethod