Fix incremental compilation with derived files (bazelbuild#700)

For incremental compilation we pass an output file map to compile actions. Since we have 2 actions with the derived files feature, they were fighting to write the same set of files. In this case we now pass a separate output file map to the derived files action that only contains the swiftmodules, and one that only contains the objects to the standard compile.

In addition, the derived files action uses a separate staging directory, as the various `swiftdeps` and `priors` files conflict.

Finally, we only set `incremental_inputs` for the swift module producing action.

swift/internal/compiling.bzl

@@ -1011,7 +1011,7 @@ def _output_object_or_file_map_configurator(prerequisites, args):
 def _output_swiftmodule_or_file_map_configurator(prerequisites, args):
     """Adds the output file map or single object file to the command line."""
     return _output_or_file_map(
-        output_file_map = prerequisites.output_file_map,
+        output_file_map = prerequisites.derived_files_output_file_map,
         outputs = [prerequisites.swiftmodule_file],
         args = args,
     )
@@ -2295,21 +2295,29 @@ def _declare_compile_outputs(
         )]
         other_outputs = []
         output_file_map = None
+        derived_files_output_file_map = None
     else:
+        split_derived_file_generation = is_feature_enabled(
+            feature_configuration = feature_configuration,
+            feature_name = SWIFT_FEATURE_SPLIT_DERIVED_FILES_GENERATION,
+        )
+
         # Otherwise, we need to create an output map that lists the individual
         # object files so that we can pass them all to the archive action.
         output_info = _declare_multiple_outputs_and_write_output_file_map(
             actions = actions,
             embeds_bc = embeds_bc,
             emits_bc = emits_bc,
             emits_partial_modules = output_nature.emits_partial_modules,
+            split_derived_file_generation = split_derived_file_generation,
             srcs = srcs,
             target_name = target_name,
         )
         object_files = output_info.object_files
         ast_files = output_info.ast_files
         other_outputs = output_info.other_outputs
         output_file_map = output_info.output_file_map
+        derived_files_output_file_map = output_info.derived_files_output_file_map
 
     # Configure index-while-building if requested. IDEs and other indexing tools
     # can enable this feature on the command line during a build and then access
@@ -2336,6 +2344,7 @@ def _declare_compile_outputs(
         indexstore_directory = indexstore_directory,
         object_files = object_files,
         output_file_map = output_file_map,
+        derived_files_output_file_map = derived_files_output_file_map,
         swiftdoc_file = swiftdoc_file,
         swiftinterface_file = swiftinterface_file,
         swiftmodule_file = swiftmodule_file,
@@ -2348,6 +2357,7 @@ def _declare_multiple_outputs_and_write_output_file_map(
         embeds_bc,
         emits_bc,
         emits_partial_modules,
+        split_derived_file_generation,
         srcs,
         target_name):
     """Declares low-level outputs and writes the output map for a compilation.
@@ -2361,6 +2371,8 @@ def _declare_multiple_outputs_and_write_output_file_map(
         emits_partial_modules: `True` if the compilation action is expected to
             emit partial `.swiftmodule` files (i.e., one `.swiftmodule` file per
             source file, as in a non-WMO compilation).
+        split_derived_file_generation: Whether objects and modules are produced
+            by separate actions.
         srcs: The list of source files that will be compiled.
         target_name: The name (excluding package path) of the target being
             built.
@@ -2377,15 +2389,28 @@ def _declare_multiple_outputs_and_write_output_file_map(
         *   `output_file_map`: A `File` that represents the output file map that
             was written and that should be passed as an input to the compilation
             action via the `-output-file-map` flag.
+        *   `derived_files_output_file_map`: A `File` that represents the
+            output file map that should be passed to derived file generation
+            actions instead of the default `output_file_map` that is used for
+            producing objects only.
     """
     output_map_file = derived_files.swiftc_output_file_map(
         actions = actions,
         target_name = target_name,
     )
 
+    if split_derived_file_generation:
+        derived_files_output_map_file = derived_files.swiftc_derived_output_file_map(
+            actions = actions,
+            target_name = target_name,
+        )
+    else:
+        derived_files_output_map_file = None
+
     # The output map data, which is keyed by source path and will be written to
-    # `output_map_file`.
+    # `output_map_file` and `derived_files_output_map_file`.
     output_map = {}
+    derived_files_output_map = {}
 
     # Object files that will be used to build the archive.
     output_objs = []
@@ -2438,7 +2463,11 @@ def _declare_multiple_outputs_and_write_output_file_map(
                 src = src,
             )
             other_outputs.append(partial_module)
-            src_output_map["swiftmodule"] = partial_module.path
+
+            if split_derived_file_generation:
+                derived_files_output_map[src.path] = {"swiftmodule": partial_module.path}
+            else:
+                src_output_map["swiftmodule"] = partial_module.path
 
         output_map[src.path] = struct(**src_output_map)
 
@@ -2447,11 +2476,18 @@ def _declare_multiple_outputs_and_write_output_file_map(
         output = output_map_file,
     )
 
+    if split_derived_file_generation:
+        actions.write(
+            content = struct(**derived_files_output_map).to_json(),
+            output = derived_files_output_map_file,
+        )
+
     return struct(
         ast_files = ast_files,
         object_files = output_objs,
         other_outputs = other_outputs,
         output_file_map = output_map_file,
+        derived_files_output_file_map = derived_files_output_map_file,
     )
 
 def _declare_validated_generated_header(actions, generated_header_name):

swift/internal/derived_files.bzl

@@ -234,6 +234,24 @@ def _swiftc_output_file_map(actions, target_name):
     """
     return actions.declare_file("{}.output_file_map.json".format(target_name))
 
+def _swiftc_derived_output_file_map(actions, target_name):
+    """Declares a file for the output file map for a swiftmodule only action.
+
+    This JSON-formatted output map file allows us to supply our own paths and
+    filenames for the intermediate artifacts produced by multiple frontend
+    invocations, rather than using the temporary defaults.
+
+    Args:
+        actions: The context's actions object.
+        target_name: The name of the target being built.
+
+    Returns:
+        The declared `File`.
+    """
+    return actions.declare_file(
+        "{}.derived_output_file_map.json".format(target_name),
+    )
+
 def _swiftdoc(actions, module_name):
     """Declares a file for the Swift doc file created by a compilation rule.
 
@@ -340,6 +358,7 @@ derived_files = struct(
     reexport_modules_src = _reexport_modules_src,
     static_archive = _static_archive,
     swiftc_output_file_map = _swiftc_output_file_map,
+    swiftc_derived_output_file_map = _swiftc_derived_output_file_map,
     swiftdoc = _swiftdoc,
     swiftinterface = _swiftinterface,
     swiftmodule = _swiftmodule,

test/split_derived_files_tests.bzl

@@ -105,6 +105,11 @@ def split_derived_files_test_suite(name = "split_derived_files"):
         expected_argv = [
             "-emit-module-path",
             "-emit-object",
+            "-enable-batch-mode",
+            "simple.output_file_map.json",
+        ],
+        not_expected_argv = [
+            "simple.derived_output_file_map.json",
         ],
         mnemonic = "SwiftCompile",
         tags = [name],
@@ -150,10 +155,13 @@ def split_derived_files_test_suite(name = "split_derived_files"):
         name = "{}_object_only".format(name),
         expected_argv = [
             "-emit-object",
+            "-enable-batch-mode",
+            "simple.output_file_map.json",
         ],
         mnemonic = "SwiftCompile",
         not_expected_argv = [
             "-emit-module-path",
+            "simple.derived_output_file_map.json",
         ],
         tags = [name],
         target_under_test = "@build_bazel_rules_swift//test/fixtures/debug_settings:simple",
@@ -163,10 +171,13 @@ def split_derived_files_test_suite(name = "split_derived_files"):
         name = "{}_swiftmodule_only".format(name),
         expected_argv = [
             "-emit-module-path",
+            "-enable-batch-mode",
+            "simple.derived_output_file_map.json",
         ],
         mnemonic = "SwiftDeriveFiles",
         not_expected_argv = [
             "-emit-object",
+            "simple.output_file_map.json",
         ],
         tags = [name],
         target_under_test = "@build_bazel_rules_swift//test/fixtures/debug_settings:simple",

tools/worker/output_file_map.cc

@@ -27,15 +27,26 @@ namespace {
 // Returns the given path transformed to point to the incremental storage area.
 // For example, "bazel-out/config/{genfiles,bin}/path" becomes
 // "bazel-out/config/{genfiles,bin}/_swift_incremental/path".
-static std::string MakeIncrementalOutputPath(std::string path) {
+// When split compiling we need different directories, as the various swiftdeps
+// and priors files conflict.
+static std::string MakeIncrementalOutputPath(std::string path,
+                                             bool is_derived) {
   auto bin_index = path.find("/bin/");
   if (bin_index != std::string::npos) {
-    path.replace(bin_index, 5, "/bin/_swift_incremental/");
+    if (is_derived) {
+      path.replace(bin_index, 5, "/bin/_swift_incremental_derived/");
+    } else {
+      path.replace(bin_index, 5, "/bin/_swift_incremental/");
+    }
     return path;
   }
   auto genfiles_index = path.find("/genfiles/");
   if (genfiles_index != std::string::npos) {
-    path.replace(genfiles_index, 10, "/genfiles/_swift_incremental/");
+    if (is_derived) {
+      path.replace(genfiles_index, 10, "/genfiles/_swift_incremental_derived/");
+    } else {
+      path.replace(genfiles_index, 10, "/genfiles/_swift_incremental/");
+    }
     return path;
   }
   return path;
@@ -55,16 +66,20 @@ void OutputFileMap::WriteToPath(const std::string &path) {
 }
 
 void OutputFileMap::UpdateForIncremental(const std::string &path) {
+  bool derived =
+      path.find(".derived_output_file_map.json") != std::string::npos;
+
   nlohmann::json new_output_file_map;
   std::map<std::string, std::string> incremental_outputs;
   std::map<std::string, std::string> incremental_inputs;
+  bool emitting_module = false;
 
   // The empty string key is used to represent outputs that are for the whole
   // module, rather than for a particular source file.
   nlohmann::json module_map;
   // Derive the swiftdeps file name from the .output-file-map.json name.
   auto new_path = ReplaceExtension(path, ".swiftdeps", /*all_extensions=*/true);
-  auto swiftdeps_path = MakeIncrementalOutputPath(new_path);
+  auto swiftdeps_path = MakeIncrementalOutputPath(new_path, derived);
   module_map["swift-dependencies"] = swiftdeps_path;
   new_output_file_map[""] = module_map;
 
@@ -73,6 +88,7 @@ void OutputFileMap::UpdateForIncremental(const std::string &path) {
     auto outputs = element.value();
 
     nlohmann::json src_map;
+    std::string swiftdeps_path;
 
     // Process the outputs for the current source file.
     for (auto &output : outputs.items()) {
@@ -83,19 +99,25 @@ void OutputFileMap::UpdateForIncremental(const std::string &path) {
         // If the file kind is "object", we want to update the path to point to
         // the incremental storage area and then add a "swift-dependencies"
         // in the same location.
-        auto new_path = MakeIncrementalOutputPath(path);
+        auto new_path = MakeIncrementalOutputPath(path, derived);
         src_map[kind] = new_path;
         incremental_outputs[path] = new_path;
 
-        auto swiftdeps_path = ReplaceExtension(new_path, ".swiftdeps");
-        src_map["swift-dependencies"] = swiftdeps_path;
+        if (swiftdeps_path.empty()) {
+          swiftdeps_path = ReplaceExtension(new_path, ".swiftdeps");
+        }
       } else if (kind == "swiftdoc" || kind == "swiftinterface" ||
                  kind == "swiftmodule" || kind == "swiftsourceinfo") {
         // Module/interface outputs should be moved to the incremental storage
         // area without additional processing.
-        auto new_path = MakeIncrementalOutputPath(path);
+        auto new_path = MakeIncrementalOutputPath(path, derived);
         src_map[kind] = new_path;
         incremental_outputs[path] = new_path;
+        emitting_module = true;
+
+        if (swiftdeps_path.empty()) {
+          swiftdeps_path = ReplaceExtension(new_path, ".swiftdeps");
+        }
       } else if (kind == "swift-dependencies") {
         // If there was already a "swift-dependencies" entry present, ignore it.
         // (This shouldn't happen because the build rules won't do this, but
@@ -109,23 +131,34 @@ void OutputFileMap::UpdateForIncremental(const std::string &path) {
       }
     }
 
+    // When split compiling both output_file_maps need src level swiftdeps
+    if (!swiftdeps_path.empty()) {
+      src_map["swift-dependencies"] = swiftdeps_path;
+    }
+
     new_output_file_map[src] = src_map;
   }
 
-  auto swiftmodule_path =
-      ReplaceExtension(path, ".swiftmodule", /*all_extensions=*/true);
-  auto copied_swiftmodule_path = MakeIncrementalOutputPath(swiftmodule_path);
-  incremental_inputs[swiftmodule_path] = copied_swiftmodule_path;
-
-  auto swiftdoc_path =
-      ReplaceExtension(path, ".swiftdoc", /*all_extensions=*/true);
-  auto copied_swiftdoc_path = MakeIncrementalOutputPath(swiftdoc_path);
-  incremental_inputs[swiftdoc_path] = copied_swiftdoc_path;
-
-  auto swiftsourceinfo_path =
-      ReplaceExtension(path, ".swiftsourceinfo", /*all_extensions=*/true);
-  auto copied_swiftsourceinfo_path = MakeIncrementalOutputPath(swiftsourceinfo_path);
-  incremental_inputs[swiftsourceinfo_path] = copied_swiftsourceinfo_path;
+  // If we don't generate a swiftmodule, don't try to copy those files
+  if (emitting_module) {
+    auto swiftmodule_path =
+        ReplaceExtension(path, ".swiftmodule", /*all_extensions=*/true);
+    auto copied_swiftmodule_path =
+        MakeIncrementalOutputPath(swiftmodule_path, derived);
+    incremental_inputs[swiftmodule_path] = copied_swiftmodule_path;
+
+    auto swiftdoc_path =
+        ReplaceExtension(path, ".swiftdoc", /*all_extensions=*/true);
+    auto copied_swiftdoc_path =
+        MakeIncrementalOutputPath(swiftdoc_path, derived);
+    incremental_inputs[swiftdoc_path] = copied_swiftdoc_path;
+
+    auto swiftsourceinfo_path =
+        ReplaceExtension(path, ".swiftsourceinfo", /*all_extensions=*/true);
+    auto copied_swiftsourceinfo_path =
+        MakeIncrementalOutputPath(swiftsourceinfo_path, derived);
+    incremental_inputs[swiftsourceinfo_path] = copied_swiftsourceinfo_path;
+  }
 
   json_ = new_output_file_map;
   incremental_outputs_ = incremental_outputs;