subninja chdir: just like ninja in a subshell

[davidhubbard]

This feature lets a build.ninja depend on a submodule that has its own ninja build system. The submodule is built exactly as if it had been built independently on the command line.

rule cc
    command = gcc -o $out -Iinclude $in -Wall

build foo.exe: cc src/foo.c | submodules/bar/libbar.a

subninja build.ninja
    chdir = submodules/bar

This feature guarantees that variables defined in the parent cannot leak into the submodule, causing hard-to-fix bugs.

The parent must list a dependency on a file in the submodule for the submodule to actually be built. If there is a default in the submodule, it is ignored. The submodule build is invoked just as it would be with the specific file target on the command line.

When building in a chdir, Edge::EvaluateCommand() will prefix a command with "cd " + chdir + " && " (for posix) or "cmd /c cd " + chdir + " && " (for windows). See https://ninja-build.org/manual.html#ref_rule_command.

Other minor changes:

1. Each Node is still globally unique. State::GetNode() takes a new argument, BindingEnv* env, and BindingEnv now holds the abs_path_ of any chdir for the target.

2. Variables cannot leak across a chdir. BindingEnv, LookupVariable(), LookupRule() and LookupWithFallback() stop all lookups at a chdir boundary.

3. ManifestParser::ParseFileInclude() will update any Node that starts with the chdir path. The Node is updated to point to the new chdir BindingEnv.

4. When expanding rspfiles and depfiles, the current chdir is added to the front of any filename.

5. The reverse is done in Node::PathDecanonicalized(), which strips off the chdir.

Performance Impact

The baseline chromium build, ninja binary in depot_tools as of 2018-Oct with 12 CPUs takes 59m59.473s:

chromium/src$ rm -rf out/Release; gn gen out/Release --args="is_debug = false use_jumbo_build = true enable_nacl = false"
Done. Made 10583 targets from 1756 files in 2699ms
chromium/src$ time ninja -C out/Release chrome -l 12
ninja: Entering directory `out/Release'
[20398/20398] LINK ./chrome

real    59m59.473s
user    570m51.021s
sys     15m48.949s
chromium/src$

This patched ninja builds chromium with times of:

• 59m48.549s = 3588.549s
• 59m12.656s = 3552.656s

chromium/src$ rm -rf out/Release; gn gen out/Release --args="is_debug = false use_jumbo_build = true enable_nacl = false"
Done. Made 10583 targets from 1756 files in 2431ms
chromium/src$ time path/to/my/subninja -d stats -C out/Release chrome -l 12
ninja: Entering directory `out/Release'
[20398/20398] LINK ./chrome
metric                  count   avg (us)        total (ms)
.ninja parse            4016    1314.6          5279.6
canonicalize str        2687737 0.1             306.1
canonicalize path       4557029 0.1             439.0
lookup node             4557028 0.2             953.1
.ninja_log load         1       19.0            0.0
.ninja_deps load        1       4.0             0.0
node stat               133389  3.6             484.5
depfile load            160     12.4            2.0
StartEdge               20398   10406.7         212276.1
FinishCommand           20398   342.7           6990.0

path->node hash load 0.74 (145294 entries / 196613 buckets)

real    59m48.549s
user    571m9.050s
sys     15m45.269s
chromium/src$ rm -rf out/Release; gn gen out/Release --args="is_debug = false use_jumbo_build = true enable_nacl = false"
Done. Made 10583 targets from 1756 files in 2429ms
chromium/src$ time path/to/my/subninja -d stats -C out/Release chrome -l 12
ninja: Entering directory `out/Release'
[20398/20398] LINK ./chrome
metric                  count   avg (us)        total (ms)
.ninja parse            4016    1349.3          5418.9
canonicalize str        2687737 0.1             311.0
canonicalize path       4557029 0.1             438.7
lookup node             4557028 0.2             964.9
.ninja_log load         1       23.0            0.0
.ninja_deps load        1       4.0             0.0
node stat               133389  3.6             483.8
depfile load            160     12.7            2.0
StartEdge               20398   10630.7         216844.4
FinishCommand           20398   345.9           7056.4

path->node hash load 0.74 (145294 entries / 196613 buckets)

real    59m12.656s
user    569m45.955s
sys     15m41.942s
chromium/src$

Baseline manifest_parser_perftest from a clean git clone of https://github.com/ninja-build/ninja
reports an average of 540.0ms:

ninja$ ./manifest_parser_perftest
Creating manifest data...done.
516ms (hash: 768e41c)
556ms (hash: 768e41c)
538ms (hash: 768e41c)
544ms (hash: 768e41c)
546ms (hash: 768e41c)
min 516ms  max 556ms  avg 540.0ms
ninja$

Using this patched version, manifest_parser_perftest reports 570.0ms average:

subninja$ ./manifest_parser_perftest
544ms (hash: 768e41c)
577ms (hash: 768e41c)
575ms (hash: 768e41c)
576ms (hash: 768e41c)
578ms (hash: 768e41c)
min 544ms  max 578ms  avg 570.0ms
subninja$

[bauen1]

Take this with a grain of salt, since I don't currently use ninja, but have been looking at it as an alternative for make for a modular project that would greatly benefit from a feature such as this.

This could fix #977.

Performance might be improvable by a tiny / negligible amount by using chdir and vfork instead of posix_spawn.

A minor issue: chdir doesn't accept . or .., so:

subninja build.ninja
  chdir = a

works, but

subninja build.ninja
  chdir = ./a

doesn't, even though they should behave the same. (but not allowing .. might be a good thing)

Personally I would prefer a syntax closer to:

subproject a/build.ninja
  # possibly allow overwriting the following:
  # chdir (defaults to 'a/')
  # builddir (defaults to $chdir)

# which would be equivalent to:
subninja build.ninja
   chdir = a

Since the functionality would be quite different from what subninja does and the syntax would be close to similiar constructs (ie. full / relative path and not broken into pieces).

Allowing $builddir to be overridden would allow true out-of-tree builds if supported by the generated build script (see #977).

Technically this would also allow for true out-of-tree-objects and outside-of-source-tree builds (if supported by the ninja generator of the project), eg.

$ ls
ninja-git
build
$ ninja --file ninja-git/build.ninja --destdir build
...
$ ls build
ninja
...

(Which isn't exactly useful, unless your building a project as part of something bigger)

I would love to see this (albeit slightly modified) merged 👍

[davidhubbard]

Thanks for the feedback, @bauen1, yeah @Qix- and I discussed #977 and that was the impetus to make this PR.

The reason I made this just a "chdir" option to the existing "subninja" instead of a whole new keyword "subproject" is that I feel like this PR simply expands "subninja," and in a way that is fairly straightforward - when "chdir" is used, parent rules can depend on targets within the chdir but no variables get through from the parent to the child. When "chdir" is not used, the child .ninja file will be able to use parent variables.

It's possible I've not understood you, but I very much think this is a case of "less is more." For an out-of-tree build, if I understand what you're trying to do, you can already tell ninja to do that, without this PR:

When you generate the build.ninja you put the build.ninja in the out-of-tree build directory but it has either relative or absolute path references to the sources. I definitely do that all the time using both CMake (--build option) and Chromium's gn build system ("gen" option). No "subninja" or "chdir" needed. It's already supported.

[bauen1]

Right, I forgot that ninja build scripts are supposed to be generated. 👍

[davidhubbard]

Synced to latest, resolved merge conflicts.

[ActuallyaDeviloper]

Is this still active?
I just stumbled upon this pull request looking for this feature.

I gave this branch a spin and ran into two problems:

1. ../ doesn't work in chdir. That would be exactly my usecase because I have multiple projects next to each other (basically the opposite of a monorepo) and want them to refer to each other in the build.
2. It doesn't quite work for me on Windows 8. I changed it to cmd /c "cd xxx && yyy" for it to work and it still fails sometimes, possibly because quotes need to be escaped now. It seems to me, that the proper solution is to setup the process by directly passing it in lpCurrentDirectory to CreateProcessA. I also found this comment: https://github.com/ninja-build/ninja/blob/master/src/subprocess-win32.cc#L108

Besides, I would really like see this this feature added to ninja.

[davidhubbard]

../ doesn't work by design.

Targets can be provided by a child dir which the parent dir can then depend on. Not vice versa. Not even by pretending to be a child dir at the ../ path

Without knowing more about your build except that you wrote you have "multiple projects next to each other," for instance you're going to have a bad time if you've got circular dependencies. This Pull Request avoids circular dependencies by not allowing the child to know anything about the parent.

Your escaping question misses the simplified case where there was no chdir. Before this Pull Request, cmd /c yyy worked without any escaping. Adding "cd xxx" doesn't change that: cmd /c "cd xxx" && yyy.

Happy to help you work within this Pull Request to get your build to work, it seems doable, if you comment over at my fork of ninja - github.com/davidhubbard/ninja. I won't discuss anymore here.

[ActuallyaDeviloper]

There are no circular dependencies. I just want to compile different projects together and I don't want to copy each shared project into each projects that uses it. ../ works for everything else in ninja. I think it's highly illogical to forbid it unless it is a high burden to implement.

Regarding cmd /c, it seems it was not used before in Ninja and apparently caused trouble in the past, hence that comment. Have you actually tried it on Windows? I believe the command is interpreted as "cmd /c cd xxx" && "yyy".

[davidhubbard]

Synced to latest, resolved merge conflicts.

[davidhubbard]

Fixed windows tests in src/manifest_parser_test.cc

[tbodt]

@jhasse is this something you'd merge?

[jhasse]

Yes, eventuelly, but no guarantees. Currently there's lots of other stuff on my plate.

[davidhubbard]

Thanks @jhasse. I have decided not to worry about syncing this pull request every time it gets a conflict with head. I'm still very much interested in this pull request. No worries about if/when this gets merged.

[ActuallyaDeviloper]

A year has passed since my last comment so I took this opportunity to gave this branch another spin and see if I can make it work. I documented my findings.

Unfortunately the relative path limitation still persists in the PR and it's not clear to me why. Here I can only repeat that I still believe that relative paths should just work fine like they do everywhere else in Ninja. It's not as if Ninja otherwise was an isolated build environment in which you shouldn't be able to escape the current directory. It is very reasonable to have the generated Ninja-files in a ninja subdirectory which you have to leave with ../some_sub_module/ninja or to have multiple projects next to each other that refer to each other like ../other_project.

My second issue when I tried to use this last year was related to the use of cmd /c. I have since figured out what exactly the problem was. The issue was that the cmd interpreter requires quotation marks to be applied to the command's executable name if a POSIX path separator is used even if there are no spaces in the path (like for example a/b.exe). This limitation does not apply to CreateProcessA invokations directly.

Fortunately @davidhubbard has since done some updates to the code so that it now avoids using cmd /c on Windows.

Unfortunately it doesn't quite work for me still for a number of reasons:

1. The endCD == string::npos check in subprocess-win32.cc is incorrect. It should be !=.
2. The cmd /c to lpCurrentDirectory transformation is not quite equivalent to "ninja in a subshell" because the executable name itself is not looked up relative to the new current directory. If someone (like me) invokes a local tool in the repository as a Ninja build step, it won't be found.
3. I think it is not a save change to drop the cmd /c in general like a comment in the code currently claims. This is because there are many other reasons people might use cmd /c besides just the single cd upfront. For example if one has this command cmd /c cd "..." && A.exe && B.exe in his Ninja build, then the current code will transform it to A.exe && B.exe which will break because CreateProcessA obviously can't handle &&. It's the same story with cmd /c cd "..." && copy ... or indeed many other cmd features.
4. RelPathEnv::ApplyChdir, VirtualFileSystem::Chdir and VirtualFileSystem::Getcwd don't seem to properly handle Windows paths i.e. backslashes and drive letters.
5. When I tried it, Ninja was recompiling the subproject unnecessarily after the subproject was explicitely rebuilt on it's own. I have not investigated this problem further but I believe this might be caused me using relative paths for chdir which contain ..\.
6. And finally there is still this thing about cmd /c wanting more quotation marks compared to CreateProcessA. This no longer actually applies but I think this whole fallback encoding using a cmd /c command string only makes sense if it would also work.

In my branch I removed the relative paths checks and fixed (1) and (2). Unfortunately I believe (3) means that the whole string modification approach can't ever work quite correctly. Probably the current directory has to be passed explicitely to Subprocess::Start.

[Qix-]

Hi, a year later from the previous comment. Can we get some movement on this, perchance?

@jhasse are you in need of co-maintainer(s)?

[jhasse]

Thanks, but we rather need people fixing bugs and stuff

[Qix-]

@jhasse what's blocking this PR?

[jhasse]

Conflicting files for one. But mainly there are currently other features which I would like to merge next, see https://github.com/ninja-build/ninja/milestones

[Qix-]

Sorry for being a broken record, and I know there are other things prioritized over this, but this is blocking CMake support in another build system.

Is there something I can do to help this along, or at least get it added to a milestone?