Allow directory as inputs, i.e. input paths ending in / - #2815
Conversation
| If an input path on a build line ends with a path separator (`/`), the input is | ||
| treated as a directory rather than a regular file. When Ninja checks whether the | ||
| build edge is up-to-date, it stats the directory itself and compares its | ||
| modification time against the outputs. |
There was a problem hiding this comment.
I think this phrase is confusing because Ninja already stats directory paths if there is no trailing slash. And I am sure that someone is using this in their build system, even if it is a fragile thing to do, so this behavior cannot be changed lightly.
A better phrasing might explain what the trailing slash does, e.g.:
"""
An input path with a trailing separator (e.g. foo/bar/) must always point to a valid directory, otherwise Ninja will complain with an error. By contrast, paths without it (e.g. foo/bar) can point to either a file or a directory.
In both cases, the timestamp used by Ninja corresponds to the file-system's directory entry itself (which does not change when the files within it are modified).
There was a problem hiding this comment.
I must say that I fail to see the benefit of this PR. I assumed it is to prepare for glob support but that would require another non-trivial change of logic in Ninja, so why introduce it here and not as a preliminary commit in a PR that does that?
| /// True if this node refers to a directory rather than a regular file. | ||
| /// Set when a manifest input/output path ends with a trailing slash. When | ||
| /// stat()ing, the path is treated as a directory and its mtime is used. | ||
| bool is_directory_ = false; |
There was a problem hiding this comment.
nit: there is no need for a boolean flag. Just use path_.back() == '/' here.
There was a problem hiding this comment.
Actually scratch that, it depends if you want to treat foo/ and foo in the same manifest as pointing to the same graph Node. From this code it looks like that if there is at least one use of foo/ in the manifest for an input path, then the Node for foo is marked as a directory, even if other uses are not.
This is quite subtle, and also doesn't happen if foo is an output, or comes from a depfile, or a dyndep file.
It would probably make more sense to just check for the trailing separator when adding a new Node instead to cover all cases, otherwise this will lead to very hard-to-debug inconsistencies.
| - list the affected files explicitly (or via a depfile) if that is desired. | ||
|
|
||
| A path with a trailing separator that exists but is not a directory is treated | ||
| as missing. |
There was a problem hiding this comment.
This doesn't seem to correspond to the implementation. If the path points to a file, it will be reported as an error ??
2 participants
See #2709.