Detection of missing implicit dependencies in dyndep files

[moritzx22]

Related issue

#2573

Detailed Description

#2573 (comment)

Example Build

rule dyndep
  command = printf 'ninja_dyndep_version = 1\nbuild output.pdf : dyndep | thisfilexists.md  spare.md\n' > $out

rule touch
  command =  touch $out

build output.pdf: touch output.ditamap || output.pdf.dd 
  dyndep = output.pdf.dd

build output.pdf.dd: dyndep

Graph

File spare.md is missing.

Proposed solution

$ ninja
ninja: build stopped: 'spare.md', needed by dyndep 'output.pdf.dd', missing and no known rule to make it.

The missing file is detected, ninja reports the missing file.

Current Master Branch

$ ninja
[1/1] touch output.pdf

The missing file is silently ignored.

[moritzx22]

Change in push

One previously unhandled scenario has been fixed:
when a missing node was marked as both depfile‑loaded and dyndep‑loaded, Ninja incorrectly reported it as missing due to the dyndep path, even though depfile loading should suppress that error. This bug is now resolved.

Limitations

There remains one limitation: Ninja still cannot report a missing dyndep‑listed dependency of an input when that dependency also appears in a depfile. The dependency graph does not track loader information for the dependencies of inputs, so it is not possible to distinguish whether such a dependency originated from a depfile or a dyndep file. See input.

Support for tracking loader information at that level has not been added, as it may not be desirable.

[digit-google]

That's a good change, but please use a default initializer here. I suggest making Loader::Manifest the first value in the enum, with value 0, and use it as the default.

[digit-google]

Feel free to rename this to loader_ too, as the generated_by_dep_ prefix is misleading in the case of manifest-created nodes :)

[moritzx22]

That's a good change, but please use a default initializer here.

The constructor always sets loader_, so a default initializer wouldn’t change anything. Matching slash_bits_ might be strong enough reason to add one.

struct Node{
Node(const std::string& path, uint64_t slash_bits, Loader loader)
      : path_(path), slash_bits_(slash_bits), loader_(loader) {}
...
std::string path_;
uint64_t slash_bits_ = 0;
// Loader loader_ = Loader::Manifest;
Loader loader_;
...
};

Introducing a default here might give the impression that Node can be constructed without specifying a loader, which isn’t the case. That’s why I avoided it, but I’m happy to adjust if it is desirable, please confirm.

I suggest making Loader::Manifest the first value in the enum, with value 0, and use it as the default.

I will do. Historically the depfile acted as the implicit default, but changing the order doesn’t affect behavior.

Feel free to rename this to loader_ too, as the generated_by_dep_ prefix is misleading in the case of manifest-created nodes :)

I will do.

[digit-google]

nit: use an enum class here for clarity

[moritzx22]

Appreciate the feedback - will do. I didn’t use a scoped enum here to stay consistent with the existing codebase.

Would it be useful to open a separate administrative PR to migrate the existing enums to enum class for consistency?

[digit-google]

See comment below, I don't think that's worth it, and to be honest the main project owner and sole submitter doesn't seem to have the time to do reviews of non-trivial PRs very often. I fear such a cosmetic change would stay in review limbo for a long time :-/ Thanks for the idea though.

[digit-google]

This naming is really confusing, as this function does not return a manifest. Also the Get prefix hides the fact that this mutates the state of the node. Same goes for GetNodeDeps() and GetNodeDyn().

Consider renaming the functions, for example:

• GetNode() -> FindOrCreateNode()
• GetNodeManifest() -> MakeManifestNode()
• GetNodeDeps() -> MakeDepfileNode()
• GetModeDyn() -> MakeDyndepNode()

Then you can keep GetNode() as is and won't have to change all the test code.

(you can also make slash_bits default-initialized to 0 to simplify the tests, but this is unrelated to the problem you're trying to fix here, though useful, so I would leave that in a separate PR).

[moritzx22]

I renamed the functions to:

/// For use in unit tests only; the node must already exist.
  Node* GetNode(StringPiece path) const;
  // Convenience helpers for creating or retrieving nodes with a specific loader.
  Node* FindOrCreateManifestNode(StringPiece path, uint64_t slash_bits);
  Node* FindOrCreateDepfileNode(StringPiece path, uint64_t slash_bits);
  Node* FindOrCreateDyndepNode(StringPiece path, uint64_t slash_bits);
private:
  Node* FindOrCreateNode(StringPiece path, uint64_t slash_bits, Node::Loader);

They’re a bit long, but they’re consistent and descriptive.

Then you can keep GetNode() as is and won't have to change all the test code.

the old GetNode(StringPiece path, uint64_t slash_bits) was intentionally removed. Its responsibilities are now covered by the three FindOrCreate…Node() functions, and in practice it maps directly to FindOrCreateDepfileNode().

The remaining GetNode(StringPiece path) const is not the successor of the historical GetNode. It’s a const lookup-only function that never creates nodes, so it doesn’t need slash_bits. It’s mainly used in unit tests, where the goal is often to check whether a node already exists.

The changes to the unit tests are inherent to the PR. The new API doesn’t require callers to distinguish between looking up and creating a node, but it does make that distinction possible. In my view, this makes the tests cleaner and reinforces the clearer separation between lookup and creation.

If the intention is to keep the unittest changes small, GetNode can be replaced with FindOrCreateDepfileNode—this is already the pattern used in deps_log_test.cc.

(you can also make slash_bits default-initialized to 0 to simplify the tests, but this is unrelated to the problem you're trying to fix here, though useful, so I would leave that in a separate PR).

I think that’s a good idea. I’d be in favor of including it in this PR as well, since the unit tests are already affected by the GetNode refactoring and would benefit from the simplification. I haven’t made the change.

[digit-google]

Excellent names, thanks. And the associated comments are very helpful.

Regarding the enum class, they're not mandatory all, just a good way to clarify that these are enums. Most other enum values in the source code use prefixes to clarify the context. E.g. LoaderManifest, LoaderDepfile, etc... would be more clear where they are used and would be consistent with the rest of the project.

Personnaly, I am a fan of the kTypeName convention too because the k prefix (which stands for constant) makes it explicit that this is a value, and not a type name, but there is no point changing the current project's conventions any way.

[digit-google]

This PR seems fine to me, I would prefer the enum values to use a Loader prefix, but I don't see that as a blocker. @jhasse what do you think?

[jhasse]

I would prefer the enum values to use a Loader prefix

It's an enum class, so that would mean Loader::LoaderDyndep which is a bit redundant.

@jhasse what do you think?

I'm not that familiar with dyndeps, will have to take a closer look when I find the time. I trust your judgment and @bradking, @mathstuf.

[digit-google]

I would prefer the enum values to use a Loader prefix

It's an enum class, so that would mean Loader::LoaderDyndep which is a bit redundant.

I agree, but I meant either use a regular enum LoaderDyndep, to be consistent with the rest of the sources, or an enum class value like Loader::Dyndep :-)