diff --git a/CHANGES.txt b/CHANGES.txt index 3ff8c30a6a..28785e12f8 100755 --- a/CHANGES.txt +++ b/CHANGES.txt @@ -135,6 +135,10 @@ RELEASE VERSION/DATE TO BE FILLED IN LATER Note that these are called for every build command run by SCons. It could have considerable performance impact if not used carefully. to connect to the server during start up. + - Ninja: added option "--skip-ninja-regen" to enable skipping regeneration of the ninja file + if scons can determine the ninja file doesnot need to be regenerated, which will also + skip restarting the scons daemon. Note this option is could result in incorrect rebuilds + if scons Glob or scons generated files are used in ninja build target's command lines. - Ninja: Added new alias "shutdown-ninja-scons-daemon" to allow ninja to shutdown the daemon. Also added cleanup to test framework to kill ninja scons daemons and clean ip daemon logs. NOTE: Test for this requires python psutil module. It will be skipped if not present. diff --git a/RELEASE.txt b/RELEASE.txt index a8c1d48b35..abecf390d4 100755 --- a/RELEASE.txt +++ b/RELEASE.txt @@ -107,7 +107,10 @@ CHANGED/ENHANCED EXISTING FUNCTIONALITY require delayed expansion to be enabled which is currently not supported and is typically not enabled by default on the host system. The batch files may also require environment variables that are not included by default in the msvc environment. - +- Ninja: added option "--skip-ninja-regen" to enable skipping regeneration of the ninja file + if scons can determine the ninja file doesnot need to be regenerated, which will also + skip restarting the scons daemon. Note this option is could result in incorrect rebuilds + if scons Glob or scons generated files are used in ninja build target's command lines. FIXES ----- diff --git a/SCons/Script/SConsOptions.py b/SCons/Script/SConsOptions.py index e2631fb85a..0dff6be669 100644 --- a/SCons/Script/SConsOptions.py +++ b/SCons/Script/SConsOptions.py @@ -147,11 +147,12 @@ def __getattr__(self, attr): 'warn', # TODO: Remove these once we update the AddOption() API to allow setting - # added flag as setable. - # Requested setable flag in : https://github.com/SCons/scons/issues/3983 + # added flag as settable. + # Requested settable flag in : https://github.com/SCons/scons/issues/3983 # From experimental ninja 'disable_execute_ninja', - 'disable_ninja' + 'disable_ninja', + 'skip_ninja_regen' ] def set_option(self, name, value): diff --git a/SCons/Tool/ninja/NinjaState.py b/SCons/Tool/ninja/NinjaState.py index 553d8cc7d3..5e7c28919b 100644 --- a/SCons/Tool/ninja/NinjaState.py +++ b/SCons/Tool/ninja/NinjaState.py @@ -29,6 +29,8 @@ import tempfile import shutil import sys +import random +import filecmp from os.path import splitext from tempfile import NamedTemporaryFile import ninja @@ -42,7 +44,7 @@ NINJA_CUSTOM_HANDLERS, NINJA_DEFAULT_TARGETS from .Rules import _install_action_function, _mkdir_action_function, _lib_symlink_action_function, _copy_action_function from .Utils import get_path, alias_to_ninja_build, generate_depfile, ninja_noop, get_order_only, \ - get_outputs, get_inputs, get_dependencies, get_rule, get_command_env, to_escaped_list + get_outputs, get_inputs, get_dependencies, get_rule, get_command_env, to_escaped_list, ninja_sorted_build from .Methods import get_command @@ -82,7 +84,26 @@ def __init__(self, env, ninja_file, ninja_syntax): # to make the SCONS_INVOCATION variable properly quoted for things # like CCFLAGS scons_escape = env.get("ESCAPE", lambda x: x) - scons_daemon_port = int(env.get('NINJA_SCONS_DAEMON_PORT',-1)) + + # The daemon port should be the same across runs, unless explicitly set + # or if the portfile is deleted. This ensures the ninja file is deterministic + # across regen's if nothings changed. The construction var should take preference, + # then portfile is next, and then otherwise create a new random port to persist in + # use. + scons_daemon_port = None + os.makedirs(get_path(self.env.get("NINJA_DIR")), exist_ok=True) + scons_daemon_port_file = str(pathlib.Path(get_path(self.env.get("NINJA_DIR"))) / "scons_daemon_portfile") + + if env.get('NINJA_SCONS_DAEMON_PORT') is not None: + scons_daemon_port = int(env.get('NINJA_SCONS_DAEMON_PORT')) + elif os.path.exists(scons_daemon_port_file): + with open(scons_daemon_port_file) as f: + scons_daemon_port = int(f.read()) + else: + scons_daemon_port = random.randint(10000, 60000) + + with open(scons_daemon_port_file, 'w') as f: + f.write(str(scons_daemon_port)) # if SCons was invoked from python, we expect the first arg to be the scons.py # script, otherwise scons was invoked from the scons script @@ -387,13 +408,13 @@ def generate(self): ninja.variable("builddir", get_path(self.env.Dir(self.env['NINJA_DIR']).path)) - for pool_name, size in self.pools.items(): + for pool_name, size in sorted(self.pools.items()): ninja.pool(pool_name, min(self.env.get('NINJA_MAX_JOBS', size), size)) - for var, val in self.variables.items(): + for var, val in sorted(self.variables.items()): ninja.variable(var, val) - for rule, kwargs in self.rules.items(): + for rule, kwargs in sorted(self.rules.items()): if self.env.get('NINJA_MAX_JOBS') is not None and 'pool' not in kwargs: kwargs['pool'] = 'local_pool' ninja.rule(rule, **kwargs) @@ -535,8 +556,9 @@ def check_generated_source_deps(build): ) if remaining_outputs: - ninja.build( - outputs=sorted(remaining_outputs), rule="phony", implicit=first_output, + ninja_sorted_build( + ninja, + outputs=remaining_outputs, rule="phony", implicit=first_output, ) build["outputs"] = first_output @@ -554,12 +576,18 @@ def check_generated_source_deps(build): if "inputs" in build: build["inputs"].sort() - ninja.build(**build) + ninja_sorted_build( + ninja, + **build + ) scons_daemon_dirty = str(pathlib.Path(get_path(self.env.get("NINJA_DIR"))) / "scons_daemon_dirty") for template_builder in template_builders: template_builder["implicit"] += [scons_daemon_dirty] - ninja.build(**template_builder) + ninja_sorted_build( + ninja, + **template_builder + ) # We have to glob the SCons files here to teach the ninja file # how to regenerate itself. We'll never see ourselves in the @@ -569,17 +597,19 @@ def check_generated_source_deps(build): ninja_file_path = self.env.File(self.ninja_file).path regenerate_deps = to_escaped_list(self.env, self.env['NINJA_REGENERATE_DEPS']) - ninja.build( - ninja_file_path, + ninja_sorted_build( + ninja, + outputs=ninja_file_path, rule="REGENERATE", implicit=regenerate_deps, variables={ - "self": ninja_file_path, + "self": ninja_file_path } ) - ninja.build( - regenerate_deps, + ninja_sorted_build( + ninja, + outputs=regenerate_deps, rule="phony", variables={ "self": ninja_file_path, @@ -590,8 +620,9 @@ def check_generated_source_deps(build): # 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. - ninja.build( - "compile_commands.json", + ninja_sorted_build( + ninja, + outputs="compile_commands.json", rule="CMD", pool="console", implicit=[str(self.ninja_file)], @@ -607,12 +638,14 @@ def check_generated_source_deps(build): }, ) - ninja.build( - "compiledb", rule="phony", implicit=["compile_commands.json"], + ninja_sorted_build( + ninja, + outputs="compiledb", rule="phony", implicit=["compile_commands.json"], ) - ninja.build( - ["run_ninja_scons_daemon_phony", scons_daemon_dirty], + ninja_sorted_build( + ninja, + outputs=["run_ninja_scons_daemon_phony", scons_daemon_dirty], rule="SCONS_DAEMON", ) @@ -631,39 +664,45 @@ def check_generated_source_deps(build): if len(all_targets) == 0: all_targets = ["phony_default"] - ninja.build( + ninja_sorted_build( + ninja, outputs=all_targets, rule="phony", ) ninja.default([self.ninja_syntax.escape_path(path) for path in sorted(all_targets)]) - daemon_dir = pathlib.Path(tempfile.gettempdir()) / ('scons_daemon_' + str(hashlib.md5(str(get_path(self.env["NINJA_DIR"])).encode()).hexdigest())) - pidfile = None - if os.path.exists(scons_daemon_dirty): - pidfile = scons_daemon_dirty - elif os.path.exists(daemon_dir / 'pidfile'): - pidfile = daemon_dir / 'pidfile' - - if pidfile: - with open(pidfile) as f: - pid = int(f.readline()) - try: - os.kill(pid, signal.SIGINT) - except OSError: - pass + with NamedTemporaryFile(delete=False, mode='w') as temp_ninja_file: + temp_ninja_file.write(content.getvalue()) + + if self.env.GetOption('skip_ninja_regen') and os.path.exists(ninja_file_path) and filecmp.cmp(temp_ninja_file.name, ninja_file_path): + os.unlink(temp_ninja_file.name) + else: + + daemon_dir = pathlib.Path(tempfile.gettempdir()) / ('scons_daemon_' + str(hashlib.md5(str(get_path(self.env["NINJA_DIR"])).encode()).hexdigest())) + pidfile = None + if os.path.exists(scons_daemon_dirty): + pidfile = scons_daemon_dirty + elif os.path.exists(daemon_dir / 'pidfile'): + pidfile = daemon_dir / 'pidfile' + + if pidfile: + with open(pidfile) as f: + pid = int(f.readline()) + try: + os.kill(pid, signal.SIGINT) + except OSError: + pass # wait for the server process to fully killed # TODO: update wait_for_process_to_die() to handle timeout and then catch exception # here and do something smart. wait_for_process_to_die(pid) - if os.path.exists(scons_daemon_dirty): - os.unlink(scons_daemon_dirty) + if os.path.exists(scons_daemon_dirty): + os.unlink(scons_daemon_dirty) - with NamedTemporaryFile(delete=False, mode='w') as temp_ninja_file: - temp_ninja_file.write(content.getvalue()) - shutil.move(temp_ninja_file.name, ninja_file_path) + shutil.move(temp_ninja_file.name, ninja_file_path) self.__generated = True diff --git a/SCons/Tool/ninja/Utils.py b/SCons/Tool/ninja/Utils.py index b2c3cd3005..583301ee47 100644 --- a/SCons/Tool/ninja/Utils.py +++ b/SCons/Tool/ninja/Utils.py @@ -23,6 +23,7 @@ import os import shutil from os.path import join as joinpath +from collections import OrderedDict import SCons from SCons.Action import get_default_ENV, _string_from_cmd_list @@ -52,6 +53,15 @@ def ninja_add_command_line_options(): help='Disable ninja generation and build with scons even if tool is loaded. '+ 'Also used by ninja to build targets which only scons can build.') + AddOption('--skip-ninja-regen', + dest='skip_ninja_regen', + metavar='BOOL', + action="store_true", + default=False, + help='Allow scons to skip regeneration of the ninja file and restarting of the daemon. ' + + 'Care should be taken in cases where Glob is in use or SCons generated files are used in ' + + 'command lines.') + def is_valid_dependent_node(node): """ @@ -126,7 +136,7 @@ def get_dependencies(node, skip_sources=False): get_path(src_file(child)) for child in filter_ninja_nodes(node.children()) if child not in node.sources - ] + ] return [get_path(src_file(child)) for child in filter_ninja_nodes(node.children())] @@ -261,6 +271,19 @@ def ninja_noop(*_args, **_kwargs): """ return None +def ninja_recursive_sorted_dict(build): + sorted_dict = OrderedDict() + for key, val in sorted(build.items()): + if isinstance(val, dict): + sorted_dict[key] = ninja_recursive_sorted_dict(val) + else: + sorted_dict[key] = val + return sorted_dict + + +def ninja_sorted_build(ninja, **build): + sorted_dict = ninja_recursive_sorted_dict(build) + ninja.build(**sorted_dict) def get_command_env(env, target, source): """ diff --git a/SCons/Tool/ninja/__init__.py b/SCons/Tool/ninja/__init__.py index 7f1b9e8457..f0bdee5d3b 100644 --- a/SCons/Tool/ninja/__init__.py +++ b/SCons/Tool/ninja/__init__.py @@ -26,7 +26,7 @@ import importlib import os -import random +import traceback import subprocess import sys @@ -66,7 +66,12 @@ def ninja_builder(env, target, source): print("Generating:", str(target[0])) generated_build_ninja = target[0].get_abspath() - NINJA_STATE.generate() + try: + NINJA_STATE.generate() + except Exception: + raise SCons.Errors.BuildError( + errstr=f"ERROR: an exception occurred while generating the ninja file:\n{traceback.format_exc()}", + node=target) if env["PLATFORM"] == "win32": # TODO: Is this necessary as you set env variable in the ninja build file per target? @@ -202,7 +207,6 @@ def generate(env): env["NINJA_ALIAS_NAME"] = env.get("NINJA_ALIAS_NAME", "generate-ninja") env['NINJA_DIR'] = env.Dir(env.get("NINJA_DIR", '#/.ninja')) env["NINJA_SCONS_DAEMON_KEEP_ALIVE"] = env.get("NINJA_SCONS_DAEMON_KEEP_ALIVE", 180000) - env["NINJA_SCONS_DAEMON_PORT"] = env.get('NINJA_SCONS_DAEMON_PORT', random.randint(10000, 60000)) if GetOption("disable_ninja"): env.SConsignFile(os.path.join(str(env['NINJA_DIR']), '.ninja.sconsign')) diff --git a/test/ninja/ninja_file_deterministic.py b/test/ninja/ninja_file_deterministic.py new file mode 100644 index 0000000000..9832f226ac --- /dev/null +++ b/test/ninja/ninja_file_deterministic.py @@ -0,0 +1,110 @@ +#!/usr/bin/env python +# +# Copyright The SCons Foundation +# +# Permission is hereby granted, free of charge, to any person obtaining +# a copy of this software and associated documentation files (the +# "Software"), to deal in the Software without restriction, including +# without limitation the rights to use, copy, modify, merge, publish, +# distribute, sublicense, and/or sell copies of the Software, and to +# permit persons to whom the Software is furnished to do so, subject to +# the following conditions: +# +# The above copyright notice and this permission notice shall be included +# in all copies or substantial portions of the Software. +# +# THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY +# KIND, EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE +# WARRANTIES OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND +# NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE +# LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION +# OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION +# WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. +# + +import os +import shutil +import filecmp + +import TestSCons +from TestCmd import IS_WINDOWS + +test = TestSCons.TestSCons() + +try: + import ninja +except ImportError: + test.skip_test("Could not find module in python") + +_python_ = TestSCons._python_ +_exe = TestSCons._exe + +ninja_bin = os.path.abspath(os.path.join( + ninja.BIN_DIR, + 'ninja' + _exe)) + +test.dir_fixture('ninja-fixture') + +test.file_fixture('ninja_test_sconscripts/sconstruct_ninja_determinism', 'SConstruct') + +# generate simple build +test.run(stdout=None) +test.must_contain_all_lines(test.stdout(), ['Generating: build.ninja']) +test.must_contain_all(test.stdout(), 'Executing:') +test.must_contain_all(test.stdout(), 'ninja%(_exe)s -f' % locals()) +test.must_exist([test.workpath('out1.txt'), test.workpath('out2.txt')]) +shutil.copyfile(test.workpath('build.ninja'), test.workpath('build.ninja.orig')) + +ninja_file_mtime = os.path.getmtime(test.workpath('build.ninja')) + +# generate same build again +test.run(stdout=None) +test.must_contain_all_lines(test.stdout(), ['Generating: build.ninja', 'ninja: no work to do.']) +test.must_contain_all(test.stdout(), 'Executing:') +test.must_contain_all(test.stdout(), 'ninja%(_exe)s -f' % locals()) +test.must_exist([test.workpath('out1.txt'), test.workpath('out2.txt')]) + +if os.path.getmtime(test.workpath('build.ninja')) != ninja_file_mtime: + test.fail_test(message="build.ninja file has been updated (mtime changed) and should not have been") + +# make sure the ninja file was deterministic +if not filecmp.cmp(test.workpath('build.ninja'), test.workpath('build.ninja.orig')): + test.fail_test() + +# clean build and ninja files +os.unlink(test.workpath('build.ninja.orig')) +test.run(arguments='-c', stdout=None) + +# only generate the ninja file +test.run(arguments='--disable-execute-ninja', stdout=None) +test.must_contain_all_lines(test.stdout(), ['Generating: build.ninja']) +test.must_not_exist([test.workpath('out1.txt'), test.workpath('out2.txt')]) + +# run ninja independently +program = test.workpath('run_ninja_env.bat') if IS_WINDOWS else ninja_bin +test.run(program=program, stdout=None) +test.must_exist([test.workpath('out1.txt'), test.workpath('out2.txt')]) +shutil.copyfile(test.workpath('build.ninja'), test.workpath('build.ninja.orig')) + +# only generate the ninja file again +test.run(arguments='--disable-execute-ninja', stdout=None) +test.must_contain_all_lines(test.stdout(), ['Generating: build.ninja']) +test.must_exist([test.workpath('out1.txt'), test.workpath('out2.txt')]) + +# run ninja independently again +program = test.workpath('run_ninja_env.bat') if IS_WINDOWS else ninja_bin +test.run(program=program, stdout=None) +test.must_contain_all_lines(test.stdout(), ['ninja: no work to do.']) +test.must_exist([test.workpath('out1.txt'), test.workpath('out2.txt')]) + +# make sure the ninja file was deterministic +if not filecmp.cmp(test.workpath('build.ninja'), test.workpath('build.ninja.orig')): + test.fail_test() + +test.pass_test() + +# Local Variables: +# tab-width:4 +# indent-tabs-mode:nil +# End: +# vim: set expandtab tabstop=4 shiftwidth=4: diff --git a/test/ninja/ninja_test_sconscripts/sconstruct_ninja_determinism b/test/ninja/ninja_test_sconscripts/sconstruct_ninja_determinism new file mode 100644 index 0000000000..3d3eecbb31 --- /dev/null +++ b/test/ninja/ninja_test_sconscripts/sconstruct_ninja_determinism @@ -0,0 +1,19 @@ +import random + +SetOption('experimental', 'ninja') +SetOption('skip_ninja_regen', True) +DefaultEnvironment(tools=[]) + +env = Environment(tools=[]) + +env.Tool('ninja') + +# make the dependency list vary in order. Ninja tool should sort them to be deterministic. +for i in range(1, 10): + node = env.Command(f'out{i}.txt', 'foo.c', 'echo test > $TARGET') + deps = list(range(1, i)) + random.shuffle(deps) + for j in deps: + if j == i: + continue + env.Depends(node, f'out{j}.txt')