Added option to allow scons to determine if it should skip ninja regeneration.

8 files changed, 241 insertions, 46 deletions

diff --git a/CHANGES.txt b/CHANGES.txt
index 03692d6..4541168 100755
--- a/CHANGES.txt
+++ b/CHANGES.txt
@@ -116,6 +116,8 @@ 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 to skip ninja regeneration if scons can determine the ninja file does
+      not need to be regenerated.
 
   From Mats Wichmann:
     - Tweak the way default site_scons paths on Windows are expressed to
diff --git a/RELEASE.txt b/RELEASE.txt
index cfa8afc..0dae549 100755
--- a/RELEASE.txt
+++ b/RELEASE.txt
@@ -88,6 +88,8 @@ 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 to skip ninja regeneration if scons can determine the ninja file does
+  not need to be regenerated.
 
 FIXES
 -----
diff --git a/SCons/Script/SConsOptions.py b/SCons/Script/SConsOptions.py
index e2631fb..0210b79 100644
--- a/SCons/Script/SConsOptions.py
+++ b/SCons/Script/SConsOptions.py
@@ -151,7 +151,8 @@ class SConsValues(optparse.Values):
         # Requested setable 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 c168c7f..c75c966 100644
--- a/SCons/Tool/ninja/NinjaState.py
+++ b/SCons/Tool/ninja/NinjaState.py
@@ -29,6 +29,8 @@ import signal
 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 @@ from .Globals import COMMAND_TYPES, NINJA_RULES, NINJA_POOLS, \
     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 @@ class NinjaState:
         # 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
@@ -381,13 +402,13 @@ class NinjaState:
 
         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)
@@ -529,8 +550,9 @@ class NinjaState:
                 )
 
                 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
@@ -548,12 +570,18 @@ class NinjaState:
             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
@@ -563,17 +591,19 @@ class NinjaState:
         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,
@@ -584,8 +614,9 @@ class NinjaState:
             # 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)],
@@ -601,12 +632,14 @@ class NinjaState:
                 },
             )
 
-            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",
         )
 
@@ -620,39 +653,45 @@ class NinjaState:
         
         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 b2c3cd3..dec6d2c 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 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 04a7abb..ad75978 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,8 +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 as e:
+        raise SCons.Errors.BuildError(
+            errstr=f"ERROR: an excetion 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?
         # this is not great, its doesn't consider specific
@@ -194,7 +198,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 0000000..2ac5e1a
--- /dev/null
+++ b/test/ninja/ninja_file_deterministic.py
@@ -0,0 +1,105 @@
+#!/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'))
+
+# 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')])
+
+# 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 0000000..6bd7c26
--- /dev/null
+++ b/test/ninja/ninja_test_sconscripts/sconstruct_ninja_determinism
@@ -0,0 +1,20 @@
+import SCons
+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')
+