Revert "SERVER-45824 Use Ninja's response files instead of hard coding them into the generated file"

This reverts commit 4dfbe1988c96cc0c46999095d3f1bbfd47ed2160.

2 files changed, 191 insertions, 191 deletions

diff --git a/site_scons/site_tools/idl_tool.py b/site_scons/site_tools/idl_tool.py
index 0e9c2e17281..d452d2cc6b8 100755
--- a/site_scons/site_tools/idl_tool.py
+++ b/site_scons/site_tools/idl_tool.py
@@ -40,6 +40,12 @@ def idlc_emitter(target, source, env):
     target_source = env.File(base_file_name + "_gen.cpp")
     target_header = env.File(base_file_name + "_gen.h")
 
+    # When generating ninja we need to inform each IDL build the
+    # string it should search for to find implicit dependencies.
+    if env.get("GENERATING_NINJA", False):
+        setattr(target_source.attributes, "NINJA_EXTRA_VARS", {"msvc_deps_prefix": "import file:"})
+        setattr(target_header.attributes, "NINJA_EXTRA_VARS", {"msvc_deps_prefix": "import file:"})
+
     env.Alias("generated-sources", [target_source, target_header])
 
     return [target_source, target_header], source
diff --git a/site_scons/site_tools/ninja.py b/site_scons/site_tools/ninja.py
index b4af805a304..a428e81fad2 100644
--- a/site_scons/site_tools/ninja.py
+++ b/site_scons/site_tools/ninja.py
@@ -18,7 +18,6 @@ import os
 import importlib
 import io
 import shutil
-import shlex
 
 from threading import Lock
 from glob import glob
@@ -37,6 +36,7 @@ NINJA_CUSTOM_HANDLERS = "__NINJA_CUSTOM_HANDLERS"
 NINJA_BUILD = "NINJA_BUILD"
 NINJA_WHEREIS_MEMO = {}
 NINJA_STAT_MEMO = {}
+MEMO_LOCK = Lock()
 
 __NINJA_RULE_MAPPING = {}
 
@@ -376,38 +376,16 @@ class NinjaState:
 
         self.rules = {
             "CMD": {
-                "command": "cmd /c $env$cmd" if sys.platform == "win32" else "$cmd",
+                "command": "cmd /c $cmd" if sys.platform == "win32" else "$cmd",
                 "description": "Building $out",
             },
             # We add the deps processing variables to this below. We
-            # don't pipe these through cmd.exe on Windows because we
+            # don't pipe this through cmd.exe on Windows because we
             # use this to generate a compile_commands.json database
             # which can't use the shell command as it's compile
-            # command.
-            "CC": {
-                "command": "$env$CC @$out.rsp",
-                "description": "Compiling $out",
-                "rspfile": "$out.rsp",
-                "rspfile_content": "$rspc",
-            },
-            "CXX": {
-                "command": "$env$CXX @$out.rsp",
-                "description": "Compiling $out",
-                "rspfile": "$out.rsp",
-                "rspfile_content": "$rspc",
-            },
-            "LINK": {
-                "command": "$env$LINK @$out.rsp",
-                "description": "Linking $out",
-                "rspfile": "$out.rsp",
-                "rspfile_content": "$rspc",
-            },
-            "AR": {
-                "command": "$env$AR @$out.rsp",
-                "description": "Archiving $out",
-                "rspfile": "$out.rsp",
-                "rspfile_content": "$rspc",
-            },
+            # command. This does mean that we assume anything using
+            # CMD_W_DEPS is a straight up compile which is true today.
+            "CMD_W_DEPS": {"command": "$cmd", "description": "Building $out"},
             "SYMLINK": {
                 "command": (
                     "cmd /c mklink $out $in"
@@ -479,12 +457,11 @@ class NinjaState:
             "scons_pool": 1,
         }
 
-        for rule in ["CC", "CXX"]:
-            if env["PLATFORM"] == "win32":
-                self.rules[rule]["deps"] = "msvc"
-            else:
-                self.rules[rule]["deps"] = "gcc"
-                self.rules[rule]["depfile"] = "$out.d"
+        if env["PLATFORM"] == "win32":
+            self.rules["CMD_W_DEPS"]["deps"] = "msvc"
+        else:
+            self.rules["CMD_W_DEPS"]["deps"] = "gcc"
+            self.rules["CMD_W_DEPS"]["depfile"] = "$out.d"
 
         self.rules.update(env.get(NINJA_RULES, {}))
         self.pools.update(env.get(NINJA_POOLS, {}))
@@ -534,7 +511,7 @@ class NinjaState:
         """Check if output ends with a known generated suffix."""
         _, suffix = splitext(output)
         return suffix in self.generated_suffixes
-
+        
     def has_generated_sources(self, output):
         """
         Determine if output indicates this is a generated header file.
@@ -644,13 +621,12 @@ class NinjaState:
                 # builder and multiple phony targets that match the
                 # file names of the remaining outputs. This way any
                 # build can depend on any output from any build.
-                first_output, remaining_outputs = (
-                    build["outputs"][0],
-                    build["outputs"][1:],
-                )
+                first_output, remaining_outputs = build["outputs"][0], build["outputs"][1:]
                 if remaining_outputs:
                     ninja.build(
-                        outputs=remaining_outputs, rule="phony", implicit=first_output,
+                        outputs=remaining_outputs,
+                        rule="phony",
+                        implicit=first_output,
                     )
 
                 build["outputs"] = first_output
@@ -708,15 +684,17 @@ class NinjaState:
             variables={"cmd": "echo $SCONS_INVOCATION_W_TARGETS"},
         )
 
-        # If we ever change the name/s of the rules that include
-        # compile commands (i.e. something like CC) we will need to
-        # update this build to reflect that complete list.
+        # Note the use of CMD_W_DEPS below. CMD_W_DEPS are always
+        # compile commands in this generator. If we ever change the
+        # name/s of the rules that include compile commands
+        # (i.e. something like CC/CXX) we will need to update this
+        # build to reflect that complete list.
         ninja.build(
             "compile_commands.json",
             rule="CMD",
             pool="console",
             variables={
-                "cmd": "ninja -f {} -t compdb CC,CXX > compile_commands.json".format(
+                "cmd": "ninja -f {} -t compdb CMD_W_DEPS > compile_commands.json".format(
                     ninja_file
                 )
             },
@@ -780,106 +758,65 @@ def src_file(node):
     return get_path(node)
 
 
-def get_comstr(env, action, targets, sources):
-    """Get the un-substituted string for action."""
-    # Despite being having "list" in it's name this member is not
-    # actually a list. It's the pre-subst'd string of the command. We
-    # use it to determine if the command we're about to generate needs
-    # to use a custom Ninja rule. By default this redirects CC, CXX,
-    # AR, SHLINK, and LINK commands to their respective rules but the
-    # user can inject custom Ninja rules and tie them to commands by
-    # using their pre-subst'd string.
-    if hasattr(action, "process"):
-        return action.cmd_list
-
-    return action.genstring(targets, sources, env)
-
-
-def get_command_env(env):
-    try:
-        return env["NINJA_ENV_VAR_CACHE"]
-    except KeyError:
-        pass
-
-    # Scan the ENV looking for any keys which do not exist in
-    # os.environ or differ from it. We assume if it's a new or
-    # differing key from the process environment then it's
-    # important to pass down to commands in the Ninja file.
-    ENV = get_default_ENV(env)
-    scons_specified_env = {
-        key: value
-        for key, value in ENV.items()
-        if key not in os.environ or os.environ.get(key, None) != value
-    }
+# TODO: Make the Rules smarter. Instead of just using a "cmd" rule
+# everywhere we should be smarter about generating CC, CXX, LINK,
+# etc. rules
+def get_command(env, node, action):  # pylint: disable=too-many-branches
+    """Get the command to execute for node."""
+    if node.env:
+        sub_env = node.env
+    else:
+        sub_env = env
 
-    windows = env["PLATFORM"] == "win32"
-    command_env = ""
-    for key, value in scons_specified_env.items():
-        # Ensure that the ENV values are all strings:
-        if is_List(value):
-            # If the value is a list, then we assume it is a
-            # path list, because that's a pretty common list-like
-            # value to stick in an environment variable:
-            value = flatten_sequence(value)
-            value = joinpath(map(str, value))
-        else:
-            # If it isn't a string or a list, then we just coerce
-            # it to a string, which is the proper way to handle
-            # Dir and File instances and will produce something
-            # reasonable for just about everything else:
-            value = str(value)
-
-        if windows:
-            command_env += "set '{}={}' && ".format(key, value)
+    executor = node.get_executor()
+    if executor is not None:
+        tlist = executor.get_all_targets()
+        slist = executor.get_all_sources()
+    else:
+        if hasattr(node, "target_peers"):
+            tlist = node.target_peers
         else:
-            command_env += "{}={} ".format(key, value)
-
-    env["NINJA_ENV_VAR_CACHE"] = command_env
-    return command_env
-
-
-def gen_get_response_file_command(env, rule, tool, maxline=2048):
-    """Generate a response file command provider for rule name."""
+            tlist = [node]
+        slist = node.sources
 
-    # If win32 and working on a compile rule then we don't want to
-    # calculate an environment for this command. It's a compile
-    # command and compiledb doesn't support shell syntax on
-    # Windows. We need the shell syntax to use environment variables
-    # on Windows so we just skip this platform / rule combination to
-    # keep the compiledb working.
-    #
-    # On POSIX we can still set environment variables even for compile
-    # commands so we do so.
-    use_command_env = not (env["PLATFORM"] == "win32" and rule in ["CC", "CXX"])
+    # Retrieve the repository file for all sources
+    slist = [rfile(s) for s in slist]
 
-    def get_response_file_command(env, node, action, targets, sources, executor=None):
-        command = generate_command(
-            env, node, action, targets, sources, executor=executor
-        )
-        cmd_list = shlex.split(command)
-        tool_idx = cmd_list.index(env.subst(tool))
-        pre_cmd, rsp_content = cmd_list[:tool_idx], cmd_list[tool_idx:]
-        variables = {"rspc": rsp_content}
-        variables[rule] = pre_cmd
-        if use_command_env:
-            variables["env"] = get_command_env(env)
-        return rule, variables
+    # Get the dependencies for all targets
+    implicit = list({dep for tgt in tlist for dep in get_dependencies(tgt)})
 
-    return get_response_file_command
+    # Generate a real CommandAction
+    if isinstance(action, SCons.Action.CommandGeneratorAction):
+        # pylint: disable=protected-access
+        action = action._generate(tlist, slist, sub_env, 1, executor=executor)
 
+    rule = "CMD"
 
-def generate_command(env, node, action, targets, sources, executor=None):
     # Actions like CommandAction have a method called process that is
     # used by SCons to generate the cmd_line they need to run. So
     # check if it's a thing like CommandAction and call it if we can.
     if hasattr(action, "process"):
-        cmd_list, _, _ = action.process(targets, sources, env, executor=executor)
+        cmd_list, _, _ = action.process(tlist, slist, sub_env, executor=executor)
+
+        # Despite being having "list" in it's name this member is not
+        # actually a list. It's the pre-subst'd string of the command. We
+        # use it to determine if the command we generated needs to use a
+        # custom Ninja rule. By default this redirects CC/CXX commands to
+        # CMD_W_DEPS but the user can inject custom Ninja rules and tie
+        # them to commands by using their pre-subst'd string.
+        rule = __NINJA_RULE_MAPPING.get(action.cmd_list, "CMD")
+
         cmd = _string_from_cmd_list(cmd_list[0])
     else:
         # Anything else works with genstring, this is most commonly hit by
         # ListActions which essentially call process on all of their
         # commands and concatenate it for us.
         genstring = action.genstring(tlist, slist, sub_env)
+
+        # Detect if we have a custom rule for this
+        # "ListActionCommandAction" type thing.
+        rule = __NINJA_RULE_MAPPING.get(genstring, "CMD")
+
         if executor is not None:
             cmd = sub_env.subst(genstring, executor=executor)
         else:
@@ -907,57 +844,59 @@ def generate_command(env, node, action, targets, sources, executor=None):
         if cmd.endswith("&&"):
             cmd = cmd[0:-2].strip()
 
-    return cmd
-
-
-def get_shell_command(env, node, action, targets, sources, executor=None):
-    return (
-        "CMD",
-        {
-            "cmd": generate_command(env, node, action, targets, sources, executor=None),
-            "env": get_command_env(env),
-        },
-    )
-
-
-def get_command(env, node, action):  # pylint: disable=too-many-branches
-    """Get the command to execute for node."""
-    if node.env:
-        sub_env = node.env
-    else:
-        sub_env = env
-
-    executor = node.get_executor()
-    if executor is not None:
-        tlist = executor.get_all_targets()
-        slist = executor.get_all_sources()
-    else:
-        if hasattr(node, "target_peers"):
-            tlist = node.target_peers
-        else:
-            tlist = [node]
-        slist = node.sources
-
-    # Retrieve the repository file for all sources
-    slist = [rfile(s) for s in slist]
-
-    # Generate a real CommandAction
-    if isinstance(action, SCons.Action.CommandGeneratorAction):
-        # pylint: disable=protected-access
-        action = action._generate(tlist, slist, sub_env, 1, executor=executor)
-
-    variables = {}
+    outputs = get_outputs(node)
+    command_env = ""
+    windows = env["PLATFORM"] == "win32"
 
-    comstr = get_comstr(sub_env, action, tlist, slist)
+    # If win32 and rule == CMD_W_DEPS then we don't want to calculate
+    # an environment for this command. It's a compile command and
+    # compiledb doesn't support shell syntax on Windows. We need the
+    # shell syntax to use environment variables on Windows so we just
+    # skip this platform / rule combination to keep the compiledb
+    # working.
+    #
+    # On POSIX we can still set environment variables even for compile
+    # commands so we do so.
+    if not (windows and rule == "CMD_W_DEPS"):
+
+        # Scan the ENV looking for any keys which do not exist in
+        # os.environ or differ from it. We assume if it's a new or
+        # differing key from the process environment then it's
+        # important to pass down to commands in the Ninja file.
+        ENV = get_default_ENV(sub_env)
+        scons_specified_env = {
+            key: value
+            for key, value in ENV.items()
+            if key not in os.environ or os.environ.get(key, None) != value
+        }
 
-    provider = __NINJA_RULE_MAPPING.get(comstr, get_shell_command)
-    rule, variables = provider(sub_env, node, action, tlist, slist, executor=executor)
+        for key, value in scons_specified_env.items():
+            # Ensure that the ENV values are all strings:
+            if is_List(value):
+                # If the value is a list, then we assume it is a
+                # path list, because that's a pretty common list-like
+                # value to stick in an environment variable:
+                value = flatten_sequence(value)
+                value = joinpath(map(str, value))
+            else:
+                # If it isn't a string or a list, then we just coerce
+                # it to a string, which is the proper way to handle
+                # Dir and File instances and will produce something
+                # reasonable for just about everything else:
+                value = str(value)
+
+            if windows:
+                command_env += "set '{}={}' && ".format(key, value)
+            else:
+                command_env += "{}={} ".format(key, value)
 
-    # Get the dependencies for all targets
-    implicit = sorted(list({dep for tgt in tlist for dep in get_dependencies(tgt)}))
+    variables = {"cmd": command_env + cmd}
+    extra_vars = getattr(node.attributes, "NINJA_EXTRA_VARS", {})
+    if extra_vars:
+        variables.update(extra_vars)
 
     ninja_build = {
-        "outputs": get_outputs(node),
+        "outputs": outputs,
         "inputs": get_inputs(node),
         "implicit": implicit,
         "rule": rule,
@@ -1118,6 +1057,11 @@ def ninja_stat(_self, path):
     running in a no_exec build the file system state should not
     change. For these reasons we patch SCons.Node.FS.LocalFS.stat to
     use our eternal memoized dictionary.
+    
+    Since this is happening during the Node walk it's being run while
+    threaded, we have to protect adding to the memoized dictionary
+    with a threading.Lock otherwise many targets miss the memoization
+    due to racing.
     """
     global NINJA_STAT_MEMO
 
@@ -1129,7 +1073,9 @@ def ninja_stat(_self, path):
         except os.error:
             result = None
 
-        NINJA_STAT_MEMO[path] = result
+        with MEMO_LOCK:
+            NINJA_STAT_MEMO[path] = result
+
         return result
 
 
@@ -1181,11 +1127,71 @@ def ninja_always_serial(self, num, taskmaster):
     self.job = SCons.Job.Serial(taskmaster)
 
 
-class NinjaNoResponseFiles(SCons.Platform.TempFileMunge):
+class NinjaEternalTempFile(SCons.Platform.TempFileMunge):
     """Overwrite the __call__ method of SCons' TempFileMunge to not delete."""
 
     def __call__(self, target, source, env, for_signature):
-        return self.cmd
+        if for_signature:
+            return self.cmd
+
+        node = target[0] if SCons.Util.is_List(target) else target
+        if node is not None:
+            cmdlist = getattr(node.attributes, "tempfile_cmdlist", None)
+            if cmdlist is not None:
+                return cmdlist
+
+        cmd = super().__call__(target, source, env, for_signature)
+
+        # If TempFileMunge.__call__ returns a string it means that no
+        # response file was needed. No processing required so just
+        # return the command.
+        if isinstance(cmd, str):
+            return cmd
+
+        # Strip the removal commands from the command list.
+        #
+        # SCons' TempFileMunge class has some very strange
+        # behavior where it, as part of the command line, tries to
+        # delete the response file after executing the link
+        # command. We want to keep those response files since
+        # Ninja will keep using them over and over. The
+        # TempFileMunge class creates a cmdlist to do this, a
+        # common SCons convention for executing commands see:
+        # https://github.com/SCons/scons/blob/master/src/engine/SCons/Action.py#L949
+        #
+        # This deletion behavior is not configurable. So we wanted
+        # to remove the deletion command from the command list by
+        # simply slicing it out here. Unfortunately for some
+        # strange reason TempFileMunge doesn't make the "rm"
+        # command it's own list element. It appends it to the
+        # tempfile argument to cmd[0] (which is CC/CXX) and then
+        # adds the tempfile again as it's own element.
+        #
+        # So we just kind of skip that middle element. Since the
+        # tempfile is in the command list on it's own at the end we
+        # can cut it out entirely. This is what I would call
+        # "likely to break" in future SCons updates. Hopefully it
+        # breaks because they start doing the right thing and not
+        # weirdly splitting these arguments up. For reference a
+        # command list that we get back from the OG TempFileMunge
+        # looks like this:
+        #
+        # [
+        #     'g++',
+        #     '@/mats/tempfiles/random_string.lnk\nrm',
+        #     '/mats/tempfiles/random_string.lnk',
+        # ]
+        #
+        # Note the weird newline and rm command in the middle
+        # element and the lack of TEMPFILEPREFIX on the last
+        # element.
+        prefix = env.subst("$TEMPFILEPREFIX")
+        if not prefix:
+            prefix = "@"
+
+        new_cmdlist = [cmd[0], prefix + cmd[-1]]
+        setattr(node.attributes, "tempfile_cmdlist", new_cmdlist)
+        return new_cmdlist
 
     def _print_cmd_str(*_args, **_kwargs):
         """Disable this method"""
@@ -1222,11 +1228,6 @@ def generate(env):
     else:
         env.Append(CCFLAGS=["-MMD", "-MF", "${TARGET}.d"])
 
-    # Provide a way for custom rule authors to easily access command
-    # generation.
-    env.AddMethod(get_shell_command, "NinjaGetShellCommand")
-    env.AddMethod(gen_get_response_file_command, "NinjaGenResponseFileProvider")
-
     # Provides a way for users to handle custom FunctionActions they
     # want to translate to Ninja.
     env[NINJA_CUSTOM_HANDLERS] = {}
@@ -1251,15 +1252,8 @@ def generate(env):
     # deleted you would get a very subtly incorrect Ninja file and
     # might not catch it.
     env.AddMethod(register_custom_rule_mapping, "NinjaRuleMapping")
-    env.NinjaRuleMapping("${CCCOM}", gen_get_response_file_command(env, "CC", env["CC"]))
-    env.NinjaRuleMapping("${CXXCOM}", gen_get_response_file_command(env, "CXX", env["CXX"]))
-    env.NinjaRuleMapping(
-        "${LINKCOM}", gen_get_response_file_command(env, "LINK", env["LINK"])
-    )
-    env.NinjaRuleMapping(
-        "${SHLINKCOM}", gen_get_response_file_command(env, "LINK", env["LINK"])
-    )
-    env.NinjaRuleMapping("${ARCOM}", gen_get_response_file_command(env, "AR", env["AR"]))
+    env.NinjaRuleMapping("${CCCOM}", "CMD_W_DEPS")
+    env.NinjaRuleMapping("${CXXCOM}", "CMD_W_DEPS")
 
     # Make SCons node walk faster by preventing unnecessary work
     env.Decider("timestamp-match")
@@ -1373,7 +1367,7 @@ def generate(env):
     if not os.path.isdir(os.environ["TMPDIR"]):
         env.Execute(SCons.Defaults.Mkdir(os.environ["TMPDIR"]))
 
-    env["TEMPFILE"] = NinjaNoResponseFiles
+    env["TEMPFILE"] = NinjaEternalTempFile
 
     # Force the SConsign to be written, we benefit from SCons caching of
     # implicit dependencies and conftests. Unfortunately, we have to do this