subninja chdir: just like ninja in a subshell #1578

davidhubbard · 2019-05-22T14:38:49Z

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:

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.
Variables cannot leak across a chdir. BindingEnv, LookupVariable(), LookupRule() and LookupWithFallback() stop all lookups at a chdir boundary.
ManifestParser::ParseFileInclude() will update any Node that starts with the chdir path. The Node is updated to point to the new chdir BindingEnv.
When expanding rspfiles and depfiles, the current chdir is added to the front of any filename.
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 · 2019-06-02T16:08:12Z

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 · 2019-06-03T19:09:17Z

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 · 2019-06-04T07:52:26Z

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

davidhubbard · 2019-08-14T21:07:18Z

Synced to latest, resolved merge conflicts.

ActuallyaDeviloper · 2019-09-14T12:38:51Z

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:

../ 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.
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 · 2019-09-14T16:38:41Z

../ 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 · 2019-09-14T17:07:06Z

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 · 2019-11-07T00:50:13Z

Synced to latest, resolved merge conflicts.

davidhubbard · 2019-11-07T19:41:07Z

Fixed windows tests in src/manifest_parser_test.cc

``` 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 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. 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$ ```

tbodt · 2020-07-18T18:09:53Z

@jhasse is this something you'd merge?

jhasse · 2020-07-20T09:13:08Z

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

davidhubbard · 2020-07-20T14:15:38Z

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 · 2020-09-29T20:13:41Z

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:

The endCD == string::npos check in subprocess-win32.cc is incorrect. It should be !=.
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.
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.
RelPathEnv::ApplyChdir, VirtualFileSystem::Chdir and VirtualFileSystem::Getcwd don't seem to properly handle Windows paths i.e. backslashes and drive letters.
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 ..\.
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- · 2021-09-17T10:21:15Z

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 · 2021-09-19T16:31:27Z

Thanks, but we rather need people fixing bugs and stuff

Qix- · 2021-09-19T17:09:25Z

@jhasse what's blocking this PR?

jhasse · 2021-09-19T17:21:02Z

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- · 2021-12-27T16:06:58Z

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?

This CL changes the grammar defined in lexer.in.cc and propagates the changes through all the affected files: * src/depfile_parse.cc * src/lexer.cc * src/lexer.h * src/lexer.in.cc (of course) This adds a 'chdir' token which can be used after a 'subninja': <subninja> path <indent> <chdir> <equals> path However, this CL does not make the changes to the parser to support the 'chdir' token. That is left for a future CL. Note that ninja-build#1578 is the origin of this sequence of CLs. The github pull request does not add a CHDIR token to the lexer, and uses a string compare with "chdir". This CL corrects that and uses the lexer in the proper way. Change-Id: Iaac81cbcc387da44f7ace1298a0f7e388f414908

davidhubbard force-pushed the master branch from fe24e3a to 40b5a3d Compare August 14, 2019 21:06

rrbutani added a commit to rrbutani/ninja that referenced this pull request Oct 20, 2019

Remove the ../ check from PR ninja-build#1578

a7e3eef

davidhubbard force-pushed the master branch from 40b5a3d to b4bb918 Compare November 7, 2019 00:50

davidhubbard force-pushed the master branch from b4bb918 to efc422f Compare November 7, 2019 19:40

davidhubbard force-pushed the master branch from efc422f to 4acd673 Compare January 21, 2020 17:33

Qix- mentioned this pull request Mar 26, 2022

chdir support for cross-project subninja evmar/n2#9

Open

sbc100 mentioned this pull request Sep 8, 2022

Experimental support for building system libraries and ports with ninja. NFC emscripten-core/emscripten#17809

Merged

theoparis mentioned this pull request Oct 14, 2023

Support subninja chdir michaelforney/samurai#100

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

subninja chdir: just like ninja in a subshell #1578

subninja chdir: just like ninja in a subshell #1578

davidhubbard commented May 22, 2019

bauen1 commented Jun 2, 2019 •

edited

Loading

davidhubbard commented Jun 3, 2019

bauen1 commented Jun 4, 2019

davidhubbard commented Aug 14, 2019

ActuallyaDeviloper commented Sep 14, 2019 •

edited

Loading

davidhubbard commented Sep 14, 2019

ActuallyaDeviloper commented Sep 14, 2019

davidhubbard commented Nov 7, 2019

davidhubbard commented Nov 7, 2019

tbodt commented Jul 18, 2020

jhasse commented Jul 20, 2020

davidhubbard commented Jul 20, 2020

ActuallyaDeviloper commented Sep 29, 2020 •

edited

Loading

Qix- commented Sep 17, 2021

jhasse commented Sep 19, 2021

Qix- commented Sep 19, 2021

jhasse commented Sep 19, 2021

Qix- commented Dec 27, 2021

subninja chdir: just like ninja in a subshell #1578

Are you sure you want to change the base?

subninja chdir: just like ninja in a subshell #1578

Conversation

davidhubbard commented May 22, 2019

bauen1 commented Jun 2, 2019 • edited Loading

davidhubbard commented Jun 3, 2019

bauen1 commented Jun 4, 2019

davidhubbard commented Aug 14, 2019

ActuallyaDeviloper commented Sep 14, 2019 • edited Loading

davidhubbard commented Sep 14, 2019

ActuallyaDeviloper commented Sep 14, 2019

davidhubbard commented Nov 7, 2019

davidhubbard commented Nov 7, 2019

tbodt commented Jul 18, 2020

jhasse commented Jul 20, 2020

davidhubbard commented Jul 20, 2020

ActuallyaDeviloper commented Sep 29, 2020 • edited Loading

Qix- commented Sep 17, 2021

jhasse commented Sep 19, 2021

Qix- commented Sep 19, 2021

jhasse commented Sep 19, 2021

Qix- commented Dec 27, 2021

bauen1 commented Jun 2, 2019 •

edited

Loading

ActuallyaDeviloper commented Sep 14, 2019 •

edited

Loading

ActuallyaDeviloper commented Sep 29, 2020 •

edited

Loading