Skip to content

Conversation

@digit-google
Copy link
Contributor

This patch simplifies Ninja internals without modifying its behavior. It removes the creation (and removal) of phony edges as producers for nodes loaded by dependency loaders, i.e. coming from depfiles, dyndep files or the deps log.

These edges were only used to ensure the build did not abort when these files are missing, unlike regular source inputs. This can be easily checked by adding a new flag to the Node class instead. This makes it easier to reason about how Ninja works internally.

More specifically:

  • Move the generated_by_dep_loader_ flag from the Edge class to the Node class. The flag is true by default to minimize changes to the source code, since node instances can be first created by reading the deps or build logs before the manifest itself.

  • Modify Plan::AddSubTarget() to avoid aborting the build when a generated-by-deploader node is missing. Instead the function exits immediately, which corresponds to what happened before.

  • State::AddOut(), State::AddIn(), State::AddValidation(): Ensure that nodes added by these methods, which are only called from the manifest parser and unit-tests set the |generated_by_dep_loader_| flag to false, to indicate that these are regular input / output nodes.

  • ManifestParser::ParseEdge(): Add an assertion verifying that the dyndep file is marked as a regular input.

  • DyndepLoader::UpdateEdge(): Remove code path that looked for phony in-edges and ignored them.

  • DepLoader::CreatePhonyInEdge() is removed as no longer necessary.

  • Update a few places in unit-tests that were checking for the creation of the phony edges.

@digit-google
Copy link
Contributor Author

Summonning @bradking and @jdrouhard who worked on this part of the code for their feedback.

@bradking
Copy link
Contributor

bradking commented Nov 7, 2023

Putting the generated_by_dep_loader_ field on the node itself, and dropping the phony input edge, makes sense to me. It more accurately represents that no edge explicitly produces the node, unless and until one is added by a dyndep binding.

I just built ninja from this PR and verified that CMake's entire test suite still passes with it, including Fortran and C++ modules test cases that exercise ninja's dyndep bindings.

This patch simplifies Ninja internals without modifying
its behavior. It removes the creation (and removal) of phony
edges as producers for nodes loaded by dependency loaders, i.e.
coming from depfiles, dyndep files or the deps log.

These edges were only used to ensure the build did not
abort when these files are missing, unlike regular source
inputs. This can be easily checked by adding a new flag to
the Node class instead. This makes it easier to reason about
how Ninja works internally.

More specifically:

- Move the generated_by_dep_loader_ flag from the Edge class
  to the Node class. The flag is true by default to minimize
  changes to the source code, since node instances can be
  first created by reading the deps or build logs before
  the manifest itself.

- Modify Plan::AddSubTarget() to avoid aborting the build
  when a generated-by-deploader node is missing. Instead
  the function exits immediately, which corresponds to
  what happened before.

- State::AddOut(), State::AddIn(), State::AddValidation():
  Ensure that nodes added by these methods, which are
  only called from the manifest parser and unit-tests
  set the |generated_by_dep_loader_| flag to false, to
  indicate that these are regular input / output nodes.

- ManifestParser::ParseEdge(): Add an assertion verifying
  that the dyndep file is marked as a regular input.

- DyndepLoader::UpdateEdge(): Remove code path that
  looked for phony in-edges and ignored them.

- DepLoader::CreatePhonyInEdge() is removed as no
  longer necessary.

+ Update a few places in unit-tests that were checking
  for the creation of the phony edges.

Fuchsia-Topic: persistent-mode
Change-Id: I98998238002351ef9c7a103040eb8a26d4183969
@digit-google digit-google force-pushed the remove-phony-in-edges branch from 9df7b02 to a744eea Compare November 7, 2023 19:10
@digit-google
Copy link
Contributor Author

Also thank you Brad for verifying that this passes the whole CMake test suite. I just completed a run of it on my local machine (which is failing on two tests because I don't have all pre-requisites like hg installed). Impressive work, I'll certainly use it to verify my other Ninja changes.

@jhasse jhasse added the cleanup label Nov 16, 2023
@jhasse jhasse added this to the 1.12.0 milestone Nov 16, 2023
@jhasse jhasse merged commit 72e5727 into ninja-build:master Nov 18, 2023
@jhasse
Copy link
Collaborator

jhasse commented Nov 18, 2023

Thanks to both of you!

@digit-google digit-google deleted the remove-phony-in-edges branch January 11, 2024 17:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants