Add support for Buck invocations with isolated daemon/cache/outputs

Summary: This diff proposes some changes to allow Buck to run in an "isolated" mode - where outputs, intermediate files etc. are not written to the usual `buck-out` directory. Instead, they are written to a directory whose name is a user-supplied prefix plus `-buck-out` that sits next to where the traditional `buck-out` would go (i.e. at the repository root). The change also means a separate `buckd` will be launched that is tied to this isolated output directory. The motivation for the change is to support Nuclide Language Services (and potentially other clients too) that want to use Buck to harvest information about the source tree and build process, but do not want to conflict with user-initiated build/test commands. A key principle I've tried to follow with the change is to not interfere with "normal" Buck usage in any way. So this diff should leave all existing Buck invocations and usage unaffected. Reviewed By: bobyangyf shipit-source-id: b68b8f211f
jasonnam · Jun 12, 2019 · c85b760 · c85b760
1 parent 117a350
commit c85b760
Show file tree

Hide file tree

Showing 8 changed files with 216 additions and 15 deletions.
diff --git a/programs/buck.py b/programs/buck.py
@@ -225,9 +225,20 @@ def get_repo(p):
                         java_version_status_queue, java_path, required_java_version
                     )
 
-                    # If 'kill' is the second argument, shut down the buckd
-                    # process
-                    if argv[1:] == ["kill"]:
+                    def has_kill_argument():
+                        if argv[1:] == ["kill"]:
+                            return True
+                        if (
+                            len(argv) > 3
+                            and argv[3:] == ["kill"]
+                            and argv[1] == "--isolation_prefix"
+                        ):
+                            return True
+                        return False
+
+                    # If 'kill' is the second argument, or immediately follows the
+                    # isolation prefix argument, shut down the buckd process
+                    if has_kill_argument():
                         buck_repo.kill_buckd()
                         return ExitCode.SUCCESS
                     return buck_repo.launch_buck(build_id, java_path, argv)

diff --git a/programs/buck_project.py b/programs/buck_project.py
@@ -57,7 +57,20 @@ def makedirs(path):
 class BuckProject:
     def __init__(self, root):
         self.root = root
-        self._buck_out = os.path.join(root, "buck-out")
+        try:
+            isolated_pos = sys.argv.index("--isolation_prefix")
+            if isolated_pos < len(sys.argv):
+                self.prefix = sys.argv[isolated_pos + 1]
+            else:
+                self.prefix = ""
+        except ValueError:
+            self.prefix = ""
+
+        self._buck_out_dirname = "buck-out"
+        if len(self.prefix) > 0:
+            self._buck_out_dirname = self.prefix + "-" + self._buck_out_dirname
+
+        self._buck_out = os.path.join(self.root, self._buck_out_dirname)
         buck_out_tmp = os.path.join(self._buck_out, "tmp")
         makedirs(buck_out_tmp)
         self._buck_out_log = os.path.join(self._buck_out, "log")
@@ -67,7 +80,7 @@ def __init__(self, root):
         # Only created if buckd is used.
         self.buckd_tmp_dir = None
 
-        self.buckd_dir = os.path.join(root, ".buckd")
+        self.buckd_dir = os.path.join(root, self.prefix + ".buckd")
         self.buckd_version_file = os.path.join(self.buckd_dir, "buckd.version")
         self.buckd_pid_file = os.path.join(self.buckd_dir, "pid")
         self.buckd_stdout = os.path.join(self.buckd_dir, "stdout")
@@ -93,7 +106,7 @@ def get_buckd_transport_address(self):
         if os.name == "nt":
             return "local:buckd_{0}".format(self.get_root_hash())
         else:
-            return "local:.buckd/sock"
+            return "local:{0}.buckd/sock".format(self.prefix)
 
     def get_running_buckd_version(self):
         return get_file_contents_if_exists(self.buckd_version_file)
@@ -119,6 +132,9 @@ def get_buckd_stderr(self):
     def get_buck_out_log_dir(self):
         return self._buck_out_log
 
+    def get_buck_out_relative_dir(self):
+        return self._buck_out_dirname
+
     def clean_up_buckd(self):
         with Tracing("BuckProject.clean_up_buckd"):
             if os.path.exists(self.buckd_dir):

diff --git a/programs/buck_tool.py b/programs/buck_tool.py
@@ -135,6 +135,7 @@ def execve(self):
 
 
 class BuckStatusReporter(object):
+
     """ Add custom logic to log Buck completion statuses or errors including
     critical ones like JVM crashes and OOMs. This object is fully mutable with
     all fields optional which get populated on the go. Only safe operations
@@ -305,18 +306,39 @@ def _environ_for_buck(self):
                 pass
         return env
 
+    def _handle_isolation_args(self, args):
+        try:
+            pos = args.index("--isolation_prefix")
+            # Allow for the argument to --isolation_prefix
+            if (pos + 1) < len(args):
+                new_args = args[:pos] + args[pos + 2 :]
+                new_args.append("--config")
+                new_args.append(
+                    "buck.base_buck_out_dir={0}".format(
+                        self._buck_project.get_buck_out_relative_dir()
+                    )
+                )
+                return new_args
+            else:
+                return args  # buck will error out on unrecognized option
+        except ValueError:
+            return args
+
     def _add_args(self, argv, args):
         """
         Add new arguments to the end of arguments string
         But before optional arguments to test runner ("--")
         """
+        if len(args) == 0:
+            return self._handle_isolation_args(argv)
+
         try:
             pos = argv.index("--")
         except ValueError:
             # "--" not found, just add to the end of the list
             pos = len(argv)
 
-        return argv[:pos] + args + argv[pos:]
+        return self._handle_isolation_args(argv[:pos] + args + argv[pos:])
 
     def _add_args_from_env(self, argv):
         """
@@ -330,8 +352,6 @@ def _add_args_from_env(self, argv):
         if os.environ.get("BUCK_CACHE_READONLY") == "1":
             args.append("-c")
             args.append("cache.http_mode=readonly")
-        if len(args) == 0:
-            return argv
         return self._add_args(argv, args)
 
     def _run_with_nailgun(self, java_path, argv, env):
@@ -797,6 +817,9 @@ def _get_java_args(self, version_uid, extra_default_options=None):
                     self._get_buck_version_timestamp()
                 ),
                 "-Dbuck.binary_hash={0}".format(self._get_buck_binary_hash()),
+                "-Dbuck.base_buck_out_dir={0}".format(
+                    self._buck_project.get_buck_out_relative_dir()
+                ),
             ]
 
             if "BUCK_DEFAULT_FILESYSTEM" not in os.environ and (

diff --git a/src/com/facebook/buck/io/filesystem/impl/BUCK b/src/com/facebook/buck/io/filesystem/impl/BUCK
@@ -18,6 +18,7 @@ java_library(
         "//src/com/facebook/buck/io:io",
         "//src/com/facebook/buck/io/file:file",
         "//src/com/facebook/buck/io/windowsfs:windowsfs",
+        "//src/com/facebook/buck/util:constants",
         "//src/com/facebook/buck/util:process_executor",
         "//src/com/facebook/buck/util:util",
         "//src/com/facebook/buck/util/config:config",

diff --git a/src/com/facebook/buck/io/filesystem/impl/DefaultProjectFilesystemFactory.java b/src/com/facebook/buck/io/filesystem/impl/DefaultProjectFilesystemFactory.java
@@ -24,6 +24,7 @@
 import com.facebook.buck.io.filesystem.ProjectFilesystemFactory;
 import com.facebook.buck.io.filesystem.RecursiveFileMatcher;
 import com.facebook.buck.io.windowsfs.WindowsFS;
+import com.facebook.buck.util.BuckConstant;
 import com.facebook.buck.util.config.Config;
 import com.facebook.buck.util.environment.Platform;
 import com.google.common.annotations.VisibleForTesting;
@@ -139,20 +140,46 @@ private static void addPathMatcherRelativeToRepo(
     }
   }
 
+  private static Optional<String> getConfiguredBuckOut(BuckPaths defaultBuckPaths, Config config) {
+    // You currently cannot truly configure the BuckOut directory today.
+    // The use of ConfiguredBuckOut and project.buck_out here is IMHO
+    // confusingly "partial" support for this feature.
+    //
+    // Language Services chose to hack around this limitation so we could run
+    // Buck in an isolated, separate BuckOut. But that requires
+    // us to ensure any ConfiguredBuckOut is a relative path underneath our
+    // top-level BuckOut (which happily enough, FBCode already does for their current
+    // use: "buck-out/dev"). We enforce that relativity here in a disgusting way.
+    Optional<String> configuredBuckOut = config.get("project", "buck_out");
+    if (configuredBuckOut.isPresent()) {
+      String value = configuredBuckOut.get();
+      String buckOut = defaultBuckPaths.getBuckOut().toString();
+      if (value.startsWith(BuckConstant.DEFAULT_BUCK_OUT_DIR_NAME)
+          && buckOut != BuckConstant.DEFAULT_BUCK_OUT_DIR_NAME) {
+        configuredBuckOut =
+            Optional.of(value.replace(BuckConstant.DEFAULT_BUCK_OUT_DIR_NAME, buckOut));
+      }
+    }
+
+    return configuredBuckOut;
+  }
+
   private static BuckPaths getConfiguredBuckPaths(
       Path rootPath, Config config, Optional<EmbeddedCellBuckOutInfo> embeddedCellBuckOutInfo) {
     BuckPaths buckPaths = BuckPaths.createDefaultBuckPaths(rootPath);
-    Optional<String> configuredBuckOut = config.getValue("project", "buck_out");
     if (embeddedCellBuckOutInfo.isPresent()) {
       Path cellBuckOut = embeddedCellBuckOutInfo.get().getCellBuckOut();
       buckPaths =
           buckPaths
               .withConfiguredBuckOut(rootPath.relativize(cellBuckOut))
               .withBuckOut(rootPath.relativize(cellBuckOut));
-    } else if (configuredBuckOut.isPresent()) {
-      buckPaths =
-          buckPaths.withConfiguredBuckOut(
-              rootPath.getFileSystem().getPath(configuredBuckOut.get()));
+    } else {
+      Optional<String> configuredBuckOut = getConfiguredBuckOut(buckPaths, config);
+      if (configuredBuckOut.isPresent()) {
+        buckPaths =
+            buckPaths.withConfiguredBuckOut(
+                rootPath.getFileSystem().getPath(configuredBuckOut.get()));
+      }
     }
     return buckPaths;
   }

diff --git a/src/com/facebook/buck/util/BuckConstant.java b/src/com/facebook/buck/util/BuckConstant.java
@@ -23,7 +23,9 @@ public class BuckConstant {
   public static final String BUCK_LOG_FILE_NAME = "buck.log";
   public static final String BUCK_MACHINE_LOG_FILE_NAME = "buck-machine-log";
   public static final String DIST_BUILD_TRACE_FILE_NAME = "dist-build.trace";
-  private static final Path BUCK_OUTPUT_PATH_DEFAULT = Paths.get("buck-out");
+  public static final String DEFAULT_BUCK_OUT_DIR_NAME = "buck-out";
+  private static final Path BUCK_OUTPUT_PATH_DEFAULT =
+      Paths.get(System.getProperty("buck.base_buck_out_dir", DEFAULT_BUCK_OUT_DIR_NAME));
 
   public static final String DIST_BUILD_SLAVE_TOPLEVEL_LOG_DIR_NAME_TEMPLATE =
       "dist-build-slave-%s";

diff --git a/test/com/facebook/buck/cli/endtoend/BaseDirEndToEndTest.java b/test/com/facebook/buck/cli/endtoend/BaseDirEndToEndTest.java
@@ -0,0 +1,120 @@
+/*
+ * Copyright 2018-present Facebook, Inc.
+ *
+ * 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.
+ */
+
+package com.facebook.buck.cli.endtoend;
+
+import com.facebook.buck.testutil.ProcessResult;
+import com.facebook.buck.testutil.endtoend.EndToEndEnvironment;
+import com.facebook.buck.testutil.endtoend.EndToEndRunner;
+import com.facebook.buck.testutil.endtoend.EndToEndTestDescriptor;
+import com.facebook.buck.testutil.endtoend.EndToEndWorkspace;
+import com.facebook.buck.testutil.endtoend.Environment;
+import com.facebook.buck.testutil.endtoend.EnvironmentFor;
+import com.google.common.collect.ImmutableMap;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.Paths;
+import org.junit.Assert;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+
+@RunWith(EndToEndRunner.class)
+public class BaseDirEndToEndTest {
+
+  @Environment
+  public static EndToEndEnvironment getBaseDirSetEnvironment() {
+    return new EndToEndEnvironment()
+        .withCommand("build")
+        .addLocalConfigSet(
+            ImmutableMap.of("parser", ImmutableMap.of("default_build_file_syntax", "SKYLARK")))
+        .addLocalConfigSet(
+            ImmutableMap.of("parser", ImmutableMap.of("default_build_file_syntax", "PYTHON_DSL")));
+  }
+
+  @EnvironmentFor(testNames = {"usingBaseDirShouldNotTouchBuckOutDir"})
+  public static EndToEndEnvironment setSuccessfulTargetPath() {
+    return getBaseDirSetEnvironment()
+        .addTemplates("cxx")
+        .withTargets("simple_successful_helloworld");
+  }
+
+  @Test
+  public void usingBaseDirShouldNotTouchBuckOutDir(
+      EndToEndTestDescriptor test, EndToEndWorkspace workspace) throws Throwable {
+    for (String template : test.getTemplateSet()) {
+      workspace.addPremadeTemplate(template);
+    }
+
+    ProcessResult result =
+        workspace.runBuckCommand(
+            "--isolation_prefix",
+            "mybase",
+            "build",
+            "//simple_successful_helloworld:simple_successful_helloworld");
+    result.assertSuccess();
+
+    Path output =
+        workspace
+            .getDestPath()
+            .resolve(
+                Paths.get(
+                    "mybase-buck-out",
+                    "gen",
+                    "simple_successful_helloworld",
+                    "simple_successful_helloworld"));
+    Assert.assertTrue(Files.exists(output));
+
+    Path usualOutput =
+        workspace
+            .getDestPath()
+            .resolve(
+                Paths.get(
+                    "buck-out",
+                    "gen",
+                    "simple_successful_helloworld",
+                    "simple_successful_helloworld"));
+    Assert.assertFalse(Files.exists(usualOutput));
+
+    workspace.deleteRecursivelyIfExists(output.toString());
+
+    result =
+        workspace.runBuckCommand(
+            "build", "//simple_successful_helloworld:simple_successful_helloworld");
+    result.assertSuccess();
+
+    output =
+        workspace
+            .getDestPath()
+            .resolve(
+                Paths.get(
+                    "mybase-buck-out",
+                    "gen",
+                    "simple_successful_helloworld",
+                    "simple_successful_helloworld"));
+    Assert.assertFalse(Files.exists(output));
+
+    usualOutput =
+        workspace
+            .getDestPath()
+            .resolve(
+                Paths.get(
+                    "buck-out",
+                    "gen",
+                    "simple_successful_helloworld",
+                    "simple_successful_helloworld"));
+    Assert.assertTrue(Files.exists(usualOutput));
+  }
+}
diff --git a/windows_failures.txt b/windows_failures.txt
@@ -1,3 +1,4 @@
+!com.facebook.buck.cli.endtoend.BaseDirEndToEndTest
 !com.facebook.buck.cli.DaemonIntegrationTest
 !com.facebook.buck.cxx.ArchiveStepIntegrationTest#thatGeneratedArchivesAreDeterministic
 !com.facebook.buck.cxx.CxxBinaryDescriptionTest#binaryShouldLinkOwnRequiredLibraries