added ninja tool for scons #52793
added ninja tool for scons #52793
Conversation
dmoody256
force-pushed
the
add_scons_ninja_tool
branch
from
dc0f04f
to
9790284
Compare
|
@Calinou Should I add a github action for building with ninja? |
Godot's CI is pretty busy as it is, so I would prefer replacing one of the current CI workflows to use Ninja instead of the default SCons setup. |
|
Im not that well versed with github actions so hopefully I set it up right. |
dmoody256
force-pushed
the
add_scons_ninja_tool
branch
from
d5d0d91
to
9a61435
Compare
|
okay fixed the ninja ci in |
|
I think also the optimizations for MD5-timestamp and implicit cache should be removed. In my opinion they are not worth it, they save a a very small amount of time but can lead to very painful and time consuming headaches if the user is not fully aware what is going on. With ninja solving the incremental build speed issue, the scons build should be reserved for absolute build correctness. |
I would move this to a dedicated PR so that it can be reviewed and merged independently. I do think removing these might be good, they seem to cause more harm than gain. How does using Ninja impact build time, and caching for CI? If it's faster on both scratch builds and incremental builds, it might be worth it to switch the whole CI over to using ninja, so that it goes faster. This requires having it play nice with the current SCons cache feature though. |
SConstruct
Outdated
| @@ -1,6 +1,6 @@ | |||
| #!/usr/bin/env python | |||
|
|
|||
| EnsureSConsVersion(3, 0, 0) | |||
| EnsureSConsVersion(4, 2, 0) | |||
| EnsurePythonVersion(3, 5) | |||
There was a problem hiding this comment.
Not for this PR specifically, but a note to self: SCons 4.2.0 is deprecated Python 3.5 support, so we might want to follow suit and get ready for 4.3.0 by bumping this to 3.6 (and then start porting the codebase to finally benefit from f-strings).
There was a problem hiding this comment.
done in
d0a5ed1
Sure, will do.
scons testing upstream show that there is not much difference in a clean build, on average ninja is slightly faster (~5%). However this testing is with a very basic C test project. It would be worth doing some comparisons with Godot. I'll get started on that.
The ninja tool does not currently support using scons cache, it is planned feature and I have some ideas how to go about it, but it probably has an ETA of >6 months. In the meantime, the ninja tool does support the ccache tool I PR'ed and made reference to above. Mongodb exclusively uses pure scons builds in its CI to leverage SCons cache. I think it would be advantageous to do the same for Godot. If you really want ninja in the CI for posterity, I would say setting up ccache to work in the CI along with ninja would be a good idea. |
That makes sense. I think we should do like mongodb here and not use ninja in CI, at least in this PR. We can reassess once cache support is improved and if it can be made to play nice with the current GitHub Actions cache. |
|
Some metrics: Intel(R) Xeon(R) Platinum 8124M CPU @ 3.00GHz (16 cores) command: ninja: pure scons: |
dmoody256
force-pushed
the
add_scons_ninja_tool
branch
from
3ca2a48
to
e3b3d25
Compare
removed ninja CI in |
dmoody256
force-pushed
the
add_scons_ninja_tool
branch
from
e3b3d25
to
4b79902
Compare
dmoody256
force-pushed
the
add_scons_ninja_tool
branch
from
4b79902
to
d0a5ed1
Compare
| from SCons.Script.Main import _load_site_scons_dir | ||
|
|
||
| _load_site_scons_dir(".", "misc/scons") |
There was a problem hiding this comment.
Instead of doing this way, you can pass env_base.Tool("scons_ninja", toolpath=["misc/scons"]), and this way get rid of the extraneous site_tools/ folder (so it's misc/scons/scons_ninja). misc/scons is our "site_tools" already.
|
|
||
| from SCons import __version__ as scons_raw_version | ||
|
|
||
| if env_base._get_major_minor_revision(scons_raw_version) >= (4, 2, 0) and sys.version_info >= (3, 6): |
There was a problem hiding this comment.
The python version check shouldn't be needed after #53829.
|
I rebased this branch on latest I also amended the commit message to be somewhat more explicit, as I like the Git history to give relevant details additionally to the PR discussion. I started doing some testing, my early results are a bit confusing but I'll have to test more thoroughly tomorrow:
Also, the error message when the |
|
My tests yesterday were done while trying to use the fast Here's what I did and got as result: I did not touch any file between each Full logs |
|
The above tests were done with https://github.com/akien-mga/godot/tree/add_scons_ninja_tool I tried the current commit of this PR instead without rebasing, and it does work much better: So either something changed in the |
|
So it seems that the slow rebuilds I experienced are simply due to daemon expired between each build:
But that seems quite limiting. I can see how this daemon would do its job well if using e.g. VSCode which tries to recompile the code all the time when you type, but for a workflow where you write code, then compile when you're done, or simply pull new code from Is there a way to improve incremental compilation when the daemon has expired? Systematically rebuilding half the code when there was no change is not usable. |
technically the daemon can stay up forever. I made the timeout because the daemon pid file lives in build directory. This is not great, because if you clean you build folder, then the pidfile is lost and the daemon will stay until someone manually finds it and takes it down. It definitely needs some work to make it a better behaved daemon. The main thing is that if the sconscripts change you need to restart the daemon. For scons specific jobs, ninja will not be able to determine if they need to be rebuilt, so ninja will execute those jobs, and send them to the scons daemon, which will then determine if they really need to be rebuilt. The ninja only jobs shouldn't need to be rebuilt, but maybe there is an issue there. I will take a look. Thanks for taking a closer look! |
|
@akien-mga So I have a few fixups in my local branch for some of the issues you presented, but still I am getting a case where ninja is rebuilding files it should not after regenerating the ninja file takes place, for example, making a non functional change to SConstruct. I realized there are several parts of the godot build which will re-write files just from scons execution, that is to say these files are generated outside the build graph, during sconscript reading phase. This will not work for the ninja tool, because the ninja tool subverts scons normal build processes to make sure nothing is built during the actual SCons build and only the dependency graph and command lines are generated. Pure SCons build can be smart about this generated during reading phase because it does MD5 checking and can see that even though the timestamp changed, the MD5 is the same so the file is not actually dirty. Here is just one example I am talking about: Lines 720 to 724 in 6ad0ca8 When I was working on a branch to move all build output to target directory, I ended up fixing all the cases I found where things are being generated outside of scons, because in that case too, SCons can not act on things done during SConscript reading phase. TLDR: I need make some changes to the structure of the build for it work with ninja, namely fixing cases were building is done during sconscript reading phase, should I do that in a separate PR? regardless of ninja, that practice is incorrect form and those build files should become official scons nodes. |
|
@akien-mga Also I wanted to mention that the ninja tool has merged to upstream scons. That version does not include the scons daemon, and when ninja needs scons to build something, it aggregates the targets, and kicks them all off at once, which has potential to lead to some ordering issues depending on the build structure. But in any case, while I appreciate the help getting the scons daemon feature working, there are other avenues to get ninja working for the godot scons build. Most of godot source code generation takes place in function actions, but it may be possible to outsource these to dedicated python scripts (you would have to do something like this for Meson anyways) and then just have ninja execute the python scripts as a command. |
|
Thank you for your contribution! As you noted, the state of ninja support changed over the course of this PR, with ninja support being upstreamed and mostly polished. I made a new one that uses this new version directly. So, I'll mark this as superceded by #89452. |
This is a project localized version from the scons builtin tool I have in my branch (plus some latest cherry picks from mongodb): https://github.com/dmoody256/scons/tree/ninja_scons_daemon
The origin of this tool has been used extensively in the mognodb build by all its developers over the last several years. This is a bleeding edge version featuring a scons daemon which facilitates callback building to scons. This bleeding edge version will make its way in to scons mainline probably within a few months.
Hopefully godot can start using it and we can flush out any issues so that the mainline version will work out of the box for godot.
Using the tool is simple by just passing
use_ninja=yeson the scons build command, and a ninja file will be generated.From there you can just run
ninjato build. Anytime you change a SConstruct or SCsub or generator py files, it should regenerate ninja file automatically.Note that the scons daemon is launched by ninja and is running scons in the background waiting for ninja to send build commands that only scons know how to deal with. This would be any kind of SCons function action or more complicated actions. The daemon uses the SCons environment variable NINJA_SCONS_DAEMON_KEEP_ALIVE to determine how many seconds the scons daemon should stay awake without receiving a command from ninja. The default is 1800 seconds. A new daemon will be used when the ninja file is regenerated.
Also note this requires using scons 4.2.
Also note that things built directly by ninja will not use SCons cache, but the callback nodes are still able to.
I look forward to providing support and bugfix to any issues the godot devs may find when using this tool.