diff options
author | Mathew Robinson <chasinglogic@gmail.com> | 2020-01-31 17:25:20 -0500 |
---|---|---|
committer | Evergreen Agent <no-reply@evergreen.mongodb.com> | 2020-01-31 22:37:45 +0000 |
commit | fc200fe8df4c2894b7a59c7b923eb6664996ac21 (patch) | |
tree | c2fa11de12a3dc7fc338e9a690ca5802a428b233 | |
parent | e1774c067b3074f9c28f284061d9de0820f942c7 (diff) | |
download | mongo-fc200fe8df4c2894b7a59c7b923eb6664996ac21.tar.gz |
Revert "SERVER-45824 Use Ninja's response files instead of hard coding them into the generated file"
This reverts commit 4dfbe1988c96cc0c46999095d3f1bbfd47ed2160.
-rw-r--r-- | SConstruct | 10 | ||||
-rw-r--r-- | etc/evergreen.yml | 2 | ||||
-rwxr-xr-x | site_scons/site_tools/idl_tool.py | 6 | ||||
-rw-r--r-- | site_scons/site_tools/ninja.py | 376 |
4 files changed, 194 insertions, 200 deletions
diff --git a/SConstruct b/SConstruct index 4ed8cbce671..98c6bb6d0df 100644 --- a/SConstruct +++ b/SConstruct @@ -3796,14 +3796,8 @@ if get_option('ninja') == 'true': description="Generating $out", deps="msvc", ) - - def get_idlc_command(env, node, action, targets, sources, executor=None): - _, variables = env.NinjaGetShellCommand(node, action, targets, sources, executor=executor) - variables["msvc_deps_prefix"] = "import file:" - return "IDLC", variables - - env.NinjaRuleMapping("$IDLCCOM", get_idlc_command) - env.NinjaRuleMapping(env["IDLCCOM"], get_idlc_command) + env.NinjaRuleMapping("$IDLCCOM", "IDLC") + env.NinjaRuleMapping(env["IDLCCOM"], "IDLC") # We can create empty files for FAKELIB in Ninja because it # does not care about content signatures. We have to diff --git a/etc/evergreen.yml b/etc/evergreen.yml index 9eae8c47cca..38356711177 100644 --- a/etc/evergreen.yml +++ b/etc/evergreen.yml @@ -3890,7 +3890,7 @@ tasks: params: working_dir: src shell: bash - script: ninja -f build.ninja + script: ninja -f new.build.ninja ## compile_all - build all scons targets ## - name: compile_all 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 |