Ninja generation tool - experimental version #3642

dmoody256 · 2020-05-06T17:32:58Z

Summary
This tool attempts to generically take a SCons build and generate a ninja file which is representative of the build and can be built by Ninja.

Ninja is a very fast build system, which is meant to have its build files generated by other systems. Ninja is especially good at iterative rebuilds and quick traversing a build graph to determine what needs to be rebuilt.

This PR is to introduce the ninja tool based off the ninja tool from mongodb's site tools.

This PR scope is BETA functionality of the ninja tool. It will not work well with all other tools and is primarily aimed at cross platform (win32/linux/osx) C/C++ builds. The goal of this PR is to get the tool to a point that it can be integrated in beta form.

Feature/Test List

Test Name	Test Description	Implemented
generate_and_build.py	Generate and build simple C program	X
generate_and_build_cxx.py	Generate and build simple CPP program and check header deps	X
generate_source.py	Generate source with built C program and build C program from generated source	X
multi_env.py	Build two C programs with two seperate envs with one ninja file	X
shell_commands.py	Simple shell command to take in sources and build targets	X
copy_function_command.py	SCons "Copy" function action converted to command line for ninja	X
build_libraries.py	Build and linking C shared and static libraries	X
iterative_speedup.py	Make sure Ninja is faster than SCons in single file change rebuild (Would love to see this fail one day)	X
output_orders.py	Make sure outputs are ordered correctly in ninja file
existing_targets.py	Make sure response files generate correctly even if existing targets are already built
unhandled_actions.py	Unhandled actions will reinvoke scons without ninja to build just those targets
ninja_rule.py	Create and register a custom ninja rule
command_line_targets.py	Specify Alias and targets from command to build
non_ninja_nodes.py	Make sure Scons is reinvoked on nodes which are not FS or Alias

Known Issues

Known Issue Description	Github Issue
SCons doesn't have representive job pools to translate to ninja	#3667
Ninja tool is global, affects all environments regardless of what environment its invoked in
Compilation Databases interactions between SCons and Ninja
Response files are always used even when not necessary
Targets and -j are not propagated to ninja correctly
TEMPLATE aggregation destroys order (need interactive mode)

Contributor Checklist:

I have created a new test or updated the unit tests to cover the basic use.
I have created a new test or updated the unit tests to cover ninja specific Methods.
I have updated src/CHANGES.txt (and read the README.txt in that directory)
I have updated the appropriate documentation

dmoody256 · 2020-05-07T15:56:46Z

this test shows how fast ninja can be at iterative rebuilds, and it gets exponentially faster the more files involved:

bdbaddog · 2020-05-07T17:03:11Z

@dmoody256 - yup. not news. Ninja's raw speed is hard to beat.

dmoody256 · 2020-05-07T17:17:26Z

@bdbaddog yes, and that's why I'm excited SCons can leverage that speed! Ninja is meant to be fast and generated, and SCons has great syntax (python!), most extendable and broad feature support, now with this Ninja feature, it can crank through big builds as fast as possible.

bdbaddog · 2020-05-10T03:39:59Z

Moving to 4.1.0 Project. Unlikely this will be releasable in the very near term.
I'd like to get 4.0.0 out in the nearer term.

Likely we'll push a beta with whatever of this is ready shortly after 4.0.0

dmoody256 · 2020-05-10T03:49:36Z

ok sounds good, I wrote some basic tests covering building basic C libs/applications. I still need to write some tests for the Ninja specific methods added to environment, and finishing documentation.

Overall the tool probably still needs some updates/tests regarding using other tools in conjunction with the ninja tool activated (example: other builders like Java, Fortran, etc..) since it reworks alot of SCons internals. That might not need to be handled on the PR, maybe just have a warning printed and documenting that this new feature is in a experimental phase.

acmorrow · 2020-05-13T15:15:21Z

SCons/Tool/ninja.py

+            # this check for us.
+            "SharedFlagChecker": ninja_noop,
+            # The install builder is implemented as a function action.
+            "installFunc": _install_action_function,


This is the motivation for #3573. By implementing our own handler for installFunc here, it means that customization of what Install means at the SCons layer doesn't get reflected in the generated Ninja file. It would be good to see that fixed.

acmorrow · 2020-05-13T15:26:38Z

SCons/Tool/ninja.py

+def _install_action_function(_env, node):
+    """Install files using the install or copy commands"""
+    return {
+        "outputs": get_outputs(node),
+        "rule": "INSTALL",
+        "inputs": [get_path(src_file(s)) for s in node.sources],
+        "implicit": get_dependencies(node),
+    }
+
+def _mkdir_action_function(env, node):
+    return {
+        "outputs": get_outputs(node),
+        "rule": "CMD",
+        # implicit explicitly omitted, we translate these so they can be
+        # used by anything that depends on these but commonly this is
+        # hit with a node that will depend on all of the fake
+        # srcnode's that SCons will never give us a rule for leading
+        # to an invalid ninja file.
+        "variables": {
+            # On Windows mkdir "-p" is always on
+            "cmd": "{mkdir}".format(
+                mkdir="mkdir $out & exit 0" if env["PLATFORM"] == "win32" else "mkdir -p $out",
+            ),
+        },
+    }
+
+def _copy_action_function(env, node):
+    return {
+        "outputs": get_outputs(node),
+        "inputs": [get_path(src_file(s)) for s in node.sources],
+        "rule": "CMD",
+        # implicit explicitly omitted, we translate these so they can be
+        # used by anything that depends on these but commonly this is
+        # hit with a node that will depend on all of the fake
+        # srcnode's that SCons will never give us a rule for leading
+        # to an invalid ninja file.
+        "variables": {
+            # On Windows mkdir "-p" is always on
+            "cmd": "$COPY $in $out",
+        },
+    }
+
+
+def _lib_symlink_action_function(_env, node):
+    """Create shared object symlinks if any need to be created"""
+    symlinks = getattr(getattr(node, "attributes", None), "shliblinks", None)
+
+    if not symlinks or symlinks is None:
+        return None
+
+    outputs = [link.get_dir().rel_path(linktgt) for link, linktgt in symlinks]
+    inputs = [link.get_path() for link, _ in symlinks]
+
+    return {
+        "outputs": outputs,
+        "inputs": inputs,
+        "rule": "SYMLINK",
+        "implicit": get_dependencies(node),
+    }


The more of these that can be later moved into command generators, the better, because then Ninja doesn't need to special case them and changes at the SCons layer will be auto-reflected in the Ninja layer.

If alot of the SCons function actions would be converted to command actions, I wonder the level of negative impact to performance for normal SCons builds if they need to spawn new shells more often? Would anyone or anything else prefer the command actions over function actions besides the ninja tool?

If its significant impact, maybe function actions could have command action variants? However, I don't like the idea of maintaining the same feature in two different implementations.

@dmoody256 - I did that with Install/InstallAs to check perf. And the intrinsic python logic was the same or slightly faster. (Surprisingly) even on linux.. Even for parallel builds.

Realistically, there will be almost no maintenance. What's going to change with logic to copy a file? As long as the action is well defined, a note in the sources specifying that the logic in Ninja generator has to be kept in sync if the functionality changes should suffice.

Or.. An equivalent command line action could be annotated to the python action which the ninja (or other) generator can use. At least that way the logic is all in one place
(Think python decorator).

SCons/Tool/ninja.py

acmorrow · 2020-05-13T15:28:56Z

SCons/Tool/ninja.py

+                "implicit": dependencies,
+            }
+
+        raise Exception("Unhandled list action with rule: " + results[0]["rule"])


This is currently one of the major weaknesses of the tool. It is unclear though how to address it.

Why does the handle_list_action function only check the rule of the first result for its list of results? It calls the action_to_ninja_build function for each item which may return a different ninja rule for each item in results.

I am understanding this function to

take in a list action and convert each action in the list to a ninja build dictionary

don't actually use the series of dictionary in those results, but generate a new single ninja build dictionary to represent all of them

So if we only check the first item we, are making some assumptions that the rest are of that same type?

Could instead the several ninja build dictionaries be made to depend on each other so they execute in the same order? Maybe some of the current handling are optimization cases where its better to have a single build item, but those could still be handled separately, and instead of just raising exception at the end here we generate all the ninja build items and make them depend on each other?

Could instead the several ninja build dictionaries be made to depend on each other so they execute in the same order?

This is how it used to work and it was too hard to get a ninja file that both functioned properly (had correct DAG) and that Ninja would happily parse. In the interest of shipping I changed this to the simpler solution here which doesn't break on the Mongo code base (which was our smaller subset of caring at the time).

Also a trick is that some things are ListActions that don't need to be, for instance Linking on Windows is a ListAction where the second Action is a FunctionAction which we had no need or analogy for. (here are the relevant functions)

tl;dr It can be done that way but it was more fiddly than I had time for when originally writing this tool.

SCons/Tool/ninja.py

acmorrow · 2020-05-13T15:59:48Z

SCons/Tool/ninja.py

+    # Used to determine if a build generates a source file. Ninja
+    # requires that all generated sources are added as order_only
+    # dependencies to any builds that *might* use them.
+    env["NINJA_GENERATED_SOURCE_SUFFIXES"] = [".h", ".hpp"]


Another place where the generated sources stuff needs to be abstracted out and dealt with.

acmorrow · 2020-05-13T16:00:18Z

SCons/Tool/ninja.py

+    # dependencies to any builds that *might* use them.
+    env["NINJA_GENERATED_SOURCE_SUFFIXES"] = [".h", ".hpp"]
+
+    if env["PLATFORM"] != "win32" and env.get("RANLIBCOM"):


This too belongs in a tool's customization of its interaction with the Ninja tool, and not in the Ninja tool itself.

acmorrow · 2020-05-13T16:03:43Z

SCons/Tool/ninja.py

+        try:
+            emitter = builder.emitter
+            if emitter is not None:
+                builder.emitter = SCons.Builder.ListEmitter(


Using an emitter here seems not quite right. Should this instead be a scanner?

acmorrow · 2020-05-13T16:04:58Z

SCons/Tool/ninja.py

+    # In the future we may be able to use this to actually cache the build.ninja
+    # file once we have the upstream support for referencing SConscripts as File
+    # nodes.
+    def ninja_execute(self):


I think this could re-use more of SCons:

+ + base_execute = SCons.Taskmaster.Task.execute def ninja_execute(self): global NINJA_STATE target = self.targets[0] target_name = str(target) - if target_name != ninja_file_name and "conftest" not in target_name: - NINJA_STATE.add_build(target) - else: - target.build() + if target_name == ninja_file_name or "conftest" in target_name: + base_execute(self) + else: + NINJA_STATE.add_build(target)

acmorrow · 2020-05-13T16:05:46Z

SCons/Tool/ninja.py

+    # date-ness.
+    SCons.Script.Main.BuildTask.needs_execute = lambda x: True
+
+    # We will eventually need to overwrite TempFileMunge to make it


This looks like an area that needs investigation regarding what we can do at the SCons layer to address it.

acmorrow · 2020-05-13T16:07:14Z

This is a great start. Thanks @dmoody256 for putting it together and adding tests. There are a ton of things we need to investigate, improve, or re-design to make this a truly useful and general purpose tool. I've left a ton of comments trying to highlight these places and provide some context.

acmorrow · 2020-05-13T16:22:56Z

Another thing to consider long term is whether the model and approach here could be extended to generate for other backends. Could we emit real msbuild or vcxproj this way?

dmoody256 · 2020-05-13T17:25:39Z

Another thing to consider long term is whether the model and approach here could be extended to generate for other backends. Could we emit real msbuild or vcxproj this way?

SCons can already generate the MSVC projects, I'm not sure if by "real" your implying some discrepancy with the SCons generated MSVC projects.

I think generalizing an approach to generate different builds is a good path to take up. It would flush out issues in SCons by generalizing the build graph to mesh with other build systems and maybe give insights into techniques other build systems have leveraged.

acmorrow · 2020-05-13T19:10:35Z

SCons/Tool/ninja.py

+    # the DAG which is required so that we walk every target, and therefore add
+    # it to the global NINJA_STATE, before we try to write the ninja file.
+    def ninja_file_depends_on_all(target, source, env):
+        if not any("conftest" in str(t) for t in target):


Forgot to mention: this is where #3626 becomes important.

acmorrow · 2020-05-13T19:11:08Z

Another thing to consider long term is whether the model and approach here could be extended to generate for other backends. Could we emit real msbuild or vcxproj this way?

SCons can already generate the MSVC projects, I'm not sure if by "real" your implying some discrepancy with the SCons generated MSVC projects.

I think the SCons generated MSVC projects just re-invoke SCons though?

dmoody256 · 2020-05-13T21:12:35Z

think the SCons generated MSVC projects just re-invoke SCons though?

ah yes, thats correct, SCons still issues the build commands.

acmorrow · 2020-05-14T14:44:01Z

SCons/Tool/ninja.py

+    # TODO: this is hacking into scons, preferable if there were a less intrusive way
+    # We will subvert the normal builder execute to make sure all the ninja file is dependent
+    # on all targets generated from any builders 
+    SCons_Builder_BuilderBase__execute = SCons.Builder.BuilderBase._execute


Why do it this way rather than just writing a scanner?

I am looking into the scanner method you suggested currently. I already had this implemented in my local and had been testing with it, so pushed it up before investigating the scanner route a bit more, but there are few draw backs with going through the env's list of builders.

One problem with just using the list of builders in the current environment, is it only captures the builders at the time the ninja tool was generated, so in a given SCons build, if you add builders later, they don't get emitters/scanners updated to make everything depend on the ninja file. Also the ninja tool affects SCons not just in the environment it was generated with, it affects all environments, so another reason going through the passed in environment's builder list is not reliable.

Secondly, there are other builders that will never be in the env's builder list, such as the Alias builder or the Command builder, so alternate solutions need to be done for those. There may be other builders too that I am not aware of that are never added to the builder list.

This solution, every builder will use this execute function, and this happens after the scons scripts have been read and executed so you can add builders after the ninja tool is initialized.

Ideally the instead of forcing this change directly into the SCons internal class from the tool, SCons has some new function that allows some callback to be setup, or lets you set all emitters/scanners for all builders.

acmorrow · 2020-05-27T20:33:14Z

@dmoody256 - FYI, please be advised of the following fix that landed in the MongoDB version of this: mongodb/mongo@18cbf0d#diff-e5450d4ac791a19fac9a32fac3a1b19c

Another one incoming that looks like this:

diff --git a/site_scons/site_tools/ninja_next.py b/site_scons/site_tools/ninja_next.py
index f999b746560..0ce56407439 100644
--- a/site_scons/site_tools/ninja_next.py
+++ b/site_scons/site_tools/ninja_next.py
@@ -1358,6 +1358,16 @@ def generate(env):
SCons.Node.FS.Dir.get_csig = ninja_csig(SCons.Node.FS.Dir.get_csig)
SCons.Node.Alias.Alias.get_csig = ninja_csig(SCons.Node.Alias.Alias.get_csig)

+    # Ignore CHANGED_SOURCES and CHANGED_TARGETS. We don't want those
+    # to have effect in a generation pass because the generator
+    # shouldn't generate differently depending on the current local
+    # state. Without this, when generating on Windows, if you already
+    # had a foo.obj, you would omit foo.cpp from the response file. Do the same for UNCHANGED.
+    SCons.Executor.Executor._get_changed_sources = SCons.Executor.Executor._get_sources
+    SCons.Executor.Executor._get_changed_targets = SCons.Executor.Executor._get_targets
+    SCons.Executor.Executor._get_unchanged_sources = SCons.Executor.Executor._get_sources
+    SCons.Executor.Executor._get_unchanged_targets = SCons.Executor.Executor._get_targets
+
     # Replace false action messages with nothing.
     env["PRINT_CMD_LINE_FUNC"] = ninja_noop

dmoody256 · 2020-05-27T20:46:33Z

@acmorrow thanks!

I would like to add some tests for these cases to capture this functionality. For the sorted issue, from my understanding it caused the generated ninja file to put the outputs in the wrong order to some build command that was expecting dwo to come after the .o? I would like to mock it up with some script that expects args in a certain order, but want to make sure that was the issue.

acmorrow · 2020-06-01T20:06:12Z

@dmoody256 - No, not exactly. It was more that having the .dwo file as the "real" target (as opposed to using the phony mechanism we use for mulitple outputs` appeared to completely disable ninja deps generation. The result was that ninja would not see targets as out of date when the should have been.

acmorrow · 2020-06-01T20:07:02Z

Also @dmoody256, this commit did land in the MongoDB version: mongodb/mongo@a7541c6

… vs using Glob()'s

…med files

…ed code changes

…c55859f325ce23daa90b3c1c55bc76cb

…eration # Conflicts: # SCons/Tool/ninja/Methods.py # SCons/Tool/ninja/NinjaState.py # SCons/Tool/ninja/Utils.py # SCons/Tool/ninja/__init__.py # doc/generated/functions.gen

mwichmann · 2021-07-09T16:25:05Z

SCons/Script/SConsOptions.py

@@ -39,7 +39,7 @@

 diskcheck_all = SCons.Node.FS.diskcheck_types()

-experimental_features = {'warp_speed', 'transporter'}
+experimental_features = {'warp_speed', 'transporter', 'ninja'}


are you intending to leave the two "dummy" names in now there's a real one?

Yes. Easter eggs are fun?

mwichmann · 2021-07-09T16:32:19Z

SCons/Tool/ninja/ninja.xml

+    <tool name="ninja">
+        <summary>
+            <para>
+                Sets up &b-link-Ninja; builder which generates a ninja build file, and then optionally runs ninja.


when referring to the ninja command it might be worth annotating it... unfortunately <application> doesn't render with our current settings, so probably <command>ninja</command> or add it to doc/scons.mod and use &ninja;.

mwichmann · 2021-07-09T16:33:29Z

SCons/Tool/ninja/ninja.xml

+            <para>
+                &b-Ninja; is a special builder which
+                adds a target to create a ninja build file.
+                The builder does not require any source files to be specified,


nit: trailing comma - intend period?

addressed in next push.

mwichmann · 2021-07-09T16:35:51Z

SCons/Tool/ninja/ninja.xml

+        <summary>
+            <para>
+                The list of source file suffixes which are generated by SCons build steps.
+                All source files which match these suffixes will be added to the _generated_sources alias in the output


since this is prelim we can fixup these later - annotation of filenames and the alias name

dmoody256 changed the title ~~Ninja generation tool~~ [WIP] Ninja generation tool May 6, 2020

dmoody256 force-pushed the ninja-generation branch from 5ab790f to 2588627 Compare May 6, 2020 17:40

dmoody256 changed the title ~~[WIP] Ninja generation tool~~ Ninja generation tool May 7, 2020

dmoody256 changed the title ~~Ninja generation tool~~ [WIP] Ninja generation tool May 7, 2020

dmoody256 force-pushed the ninja-generation branch 3 times, most recently from 40eb81a to 8803ceb Compare May 8, 2020 17:44

bdbaddog added the Ninja label May 10, 2020

bdbaddog mentioned this pull request May 10, 2020

[WIP] write a tool to generate build.ninja files from SCons #3536

Closed

3 tasks

acmorrow reviewed May 13, 2020

View reviewed changes

acmorrow mentioned this pull request May 13, 2020

Support running all external commands under a wrapper #3650

Open

acmorrow reviewed May 13, 2020

View reviewed changes

acmorrow reviewed May 14, 2020

View reviewed changes

dmoody256 force-pushed the ninja-generation branch 2 times, most recently from c786703 to bbec480 Compare May 14, 2020 16:28

bdbaddog and others added 9 commits June 1, 2021 16:52

update tests to use SetOption('experimental','ninja')

b20646d

fix sider complaint

f8415ba

switch to using SCons.Node.SConscriptNodes to get list of sconscripts…

3ed0272

… vs using Glob()'s

Quiet sider complaint

0a430bb

Continue refactor. Simplify __init__, and move logic to reasonably na…

31e1abf

…med files

Fix sider complaints

764ddca

Address mwichmann's comments on PR. Mostly doc and a few import relat…

208eb11

…ed code changes

updates to get working with mongodb build

a20a886

update from mongodb for limiting ninja max jobs from commit: 0b620c24…

a9bf0d4

…c55859f325ce23daa90b3c1c55bc76cb

dmoody256 force-pushed the ninja-generation branch from ceba1ff to a9bf0d4 Compare June 2, 2021 06:20

bdbaddog added 6 commits June 6, 2021 20:59

Merge remote-tracking branch 'dmoody/ninja-generation' into ninja-gen…

2a78d0b

…eration # Conflicts: # SCons/Tool/ninja/Methods.py # SCons/Tool/ninja/NinjaState.py # SCons/Tool/ninja/Utils.py # SCons/Tool/ninja/__init__.py # doc/generated/functions.gen

resolve sider complaint

76aa3ea

rename NINJA_BUILDDIR -> NINJA_DIR

c80bd2f

Rename DISABLE_AUTO_NINJA -> NINJA_DISABLE_AUTO_RUN

a262a13

incremental doc updates to indicate experimental state of ninja builder

2889a57

Updates to docs to indicate experimental status

f359308

mwichmann reviewed Jul 9, 2021

View reviewed changes

bdbaddog added 5 commits July 9, 2021 11:17

Resolve sider issue

24ec1b3

add ninja entity and wrap all ninja.build with <filename> tags

91364b3

Fix broken markup, regenerate files

20b5f24

Add info to CHANGES.txt on ninja

6c08f31

Merge branch 'master' into ninja-generation

0b866fc

bdbaddog merged commit ba08481 into SCons:master Jul 9, 2021

mwichmann changed the title ~~[WIP] Ninja generation tool - BETA version~~ Ninja generation tool - experimental version Jul 11, 2021

dmoody256 mentioned this pull request Sep 22, 2021

added ninja tool for scons godotengine/godot#52793

Closed

Ninja generation tool - experimental version #3642

Ninja generation tool - experimental version #3642

Conversation

dmoody256 commented May 6, 2020 • edited Loading

dmoody256 commented May 7, 2020

bdbaddog commented May 7, 2020

dmoody256 commented May 7, 2020

bdbaddog commented May 10, 2020

dmoody256 commented May 10, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chasinglogic Sep 28, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acmorrow commented May 13, 2020

acmorrow commented May 13, 2020

dmoody256 commented May 13, 2020

Choose a reason for hiding this comment

acmorrow commented May 13, 2020

dmoody256 commented May 13, 2020

Choose a reason for hiding this comment

dmoody256 May 14, 2020 • edited Loading

Choose a reason for hiding this comment

acmorrow commented May 27, 2020

dmoody256 commented May 27, 2020

acmorrow commented Jun 1, 2020

acmorrow commented Jun 1, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmoody256 commented May 6, 2020 •

edited

Loading

chasinglogic Sep 28, 2020 •

edited

Loading

dmoody256 May 14, 2020 •

edited

Loading