don't Fail if trying to mkdir when the dir already exists

[mostynb]

When ninja tries to do the equivalent of "mkdir -p", it will fail if another process (or possibly another ninja task) creates one of the intermediate dirs in the meantime. This can be quite annoying, but it's fairly simple to catch.

NinjaMain::EnsureBuildDirExists doesn't fail in case of EEXIST (the check would be redundant if this patch is landed), but Builder::StartEdge does. Perhaps we should just check for EEXIST in Builder::StartEdge?

[buildhive]

Evan Martin » ninja #668 SUCCESS
This pull request looks good
(what's this?)

[nico]

Can you explain in some more detail how this can happen (e.g. sample build.ninja + invocation). I can imagine how this could happen, but we've never seen this problem in chromium as far as I know so I'm somewhat curious.

(Running several ninjas at once is fairly strongly not supported; the only guarantee we make is that running two ninjas at once won't crash and won't corrupt build state.)

[mostynb]

I don't have a minimised testcase yet (I have a large set of local gyp files added to chromium's build system, and this only reproduces intermittently), but I think the situation that I'm encountering is:

1. we have an action in a gyp target that executes mkdir -p
2. ninja's built-in directory tree creation can simultaneously run another target that places files a shared subtree of that created by (1).

If (2) starts then (1) finishes, (2) can fail.

Since ninja's built-in directory tree creation looks like it is trying to do the equivalent of mkdir -p, I think it should be made to cope with this situation. But of course I'm also trying to fix this in my gyp files by adding dependencies to prevent this overlap.

[nico]

Having a reduced repro would be good.

[mostynb]

To simulate a loaded system / slow fs, I added a sleep(1) immediately before the MakeDir call in DiskInterface::MakeDirs. Then if you produce a ninja file from the contrived gyp file below, and run ninja on the top target with multiple jobs, it's quite easy to trigger "ninja: error: mkdir(../../staging): File exists".

Technically, this isn't a bug in ninja: it's really a problem of incompletely specified dependencies. But seeing as it's so easy to handle this and that it's already handled in NinjaMain::EnsureBuildDirExists, I think it's worth handling here too.

{
'targets': [
{
'target_name': 'top',
'type': 'none',
'dependencies': [
'mkdir_p',
'other_task',
'other_task2',
],
},
{
'target_name': 'mkdir_p',
'type': 'none',
'actions': [
{
'action_name': 'shell_mkdir',
'message': 'creating a bunch of directory trees with mkdir -p',
'inputs': [],
'outputs': [ 'staging/a/a/a' ],
'action': [
'mkdir', '-p', 'staging/a/a/a',
],
},
],
},
{
'target_name': 'other_task',
'type': 'none',
'actions': [
{
'action_name': 'create file',
'inputs': [],
'outputs': [ 'staging/a/a/a/x/x/x'],
'action': [
'eval',
'mkdir -p staging/a/a/a/x/x ; touch staging/a/a/a/x/x/x',
],
},
],
},
{
'target_name': 'other_task2',
'type': 'none',
'actions': [
{
'action_name': 'create file',
'inputs': [],
'outputs': [
# Note: ideally, you would want to specify "staging/a/a/a/a/x/1"
# here too, but let's pretend that the action below creates
# directories that we don't know ahead of time.
'fake_stamp_file',
],
'action': [
'eval',
'mkdir -p staging/a/a/a/a/x/1',
],
},
],
},
],
}

[ilor]

Any updates on this?

[mostynb]

@nico: ping would you prefer this to work in the same way that NinjaMain::EnsureBuildDirExists checks for EEXIST?

[evmar]

Sorry for not looking earlier.

I don't mind too much passing over EEXIST. I can't think of any scenarios where it'd be harmful.

If it's true that Ninja always wants to skip on EEXIST, I think it'd be good to rename the relevant functions to make their intent clearer. Perhaps MakeDirs should be renamed MakeContainingDirs (since it expects a path to a file and creates all the parents), which then calls a MakeDirs that does the mkdir -p equivalent, and then the various callers could use that. It'd let the call from EnsureBuildDirExist also be clearer (in that it wouldn't need to construct a fake path within the directory).

(I think Nico is probably the right person to finally eval this in any case.)

[nico]

I can't think of problems with this patch either, but I think trying to work well with bad build files (which is what this is about, if I understand the above correctly) shouldn't be a goal. I'm afraid that merging this will beget similar patches. (Not necessarily from the same author.)

[evmar]

The plausible use case I can think of is if you have something like:

build out/generated_headers.stamp: generate_headers ...
  # where generate_headers is a shell script that, among other things,
  # calls mkdir -p out/gen and writes files to out/gen/foo.h, etc.
  # note that the build line does *not* mention out/gen/

build out/gen/some_other_file: generate_other_file ...

If these two rules interleave at exactly the wrong times, then Ninja can race the header-generator in creating the directory. Is it always the case that Ninja must know the paths to all generated files? I think there are legitimate cases where it won't, but I'm not positive...

[mostynb]

A possible scenario that I think it would be reasonable for ninja to support: you have a tool that generates output that is not easy to predict exactly beforehand (eg doxygen, but I'm not sure if it creates many directories) and a ninja rule that outputs a file somewhere into the same tree.

A purist could rightly say that you should simply add intermediate rules to create the shared directory paths, plus some dependencies to get the ordering right. But since this is so easy to fix in the ninja source, I think it's worth doing there.

[ilor]

Please reconsider merging this. Ninja's manual claims "Output directories are always implicitly created before running the command that relies on them." as an advantage/feature. This is not true if it can be foiled by an external tool that might create the same directories at the "wrong" time, necessitating somewhat redundant mkdir targets to sync things. I believe it makes more sense to do this robustly than it is to add a lengthy explanation of when things might go wrong in some parallel scenarios.

[nico]

I suppose the patch is innocent enough.