Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

dyndep: dynamically discovered dependencies for Fortran and C++20 modules #1521

Merged
merged 20 commits into from Apr 20, 2019

Conversation

@bradking
Copy link
Contributor

bradking commented Jan 23, 2019

Fortran modules (and likely C++20 modules) require sources that provide modules to be compiled before sources that consume modules. Therefore the content of the source files defines dependencies. Unlike preprocessor dependencies (handled by depfile), module dependencies must be discovered before compiling sources even on the first build. This PR introduces a dyndep feature to provide buildsystem generators with a way to handle such dynamically discovered dependencies.

For some history, see the ninja-build list thread Adding Ninja features needed for Fortran and issue #1265.

This PR includes the full implementation, documentation, and thorough tests. The changes have been used in practice by CMake users for years (via a Kitware-maintained branch of ninja). They have been used to build large Fortran projects with thousands of sources and hundreds of modules.

The feature works by providing a new dyndep = binding for build statements. It specifies one of the (order-only) inputs that will provide the dynamically discovered dependencies that must be loaded before the build statement's command is executed. Once the "dyndep" file is ready, ninja loads it to inject additional dependencies in its build graph, re-evaluate the affected portion of the graph, and then continue the build normally.

For example:

rule f95
  command = f95 -o $out -c $in
rule fscan
  command = fscan -o $out $in

build foobar.dd: fscan foo.f90 bar.f90

build foo.o: f95 foo.f90 || foobar.dd
  dyndep = foobar.dd
build bar.o: f95 bar.f90 || foobar.dd
  dyndep = foobar.dd

In this example the order-only dependencies ensure that foobar.dd is generated before either source compiles. The hypothetical fscan tool (provided by the buildsystem generator in practice) scans the source files and writes foobar.dd with content such as:

ninja_dyndep_version = 1
build foo.o | foo.mod: dyndep
build bar.o: dyndep |  foo.mod

The "dyndep" file specifies additional inputs and outputs for the relevant build statements beyond those specified in the build manifest.

See documentation added by this PR for more details and examples.

Fixes: #1265

@mqudsi

This comment has been minimized.

Copy link
Contributor

mqudsi commented Jan 25, 2019

Please forgive me if I'm overstepping here, it seems like a lot of good work has gone into this - but is it necessary to require foobar.dd to be listed both as the dyndep source and explicitly as an order-only dependency? Can't ninja (be made to) deduce the latter?

@bradking

This comment has been minimized.

Copy link
Contributor Author

bradking commented Jan 25, 2019

@mqudsi I did think about that while designing this but decided against having dependencies specified in build.ninja by means other than the normal syntax. Also, the dyndep input does not have to be an order-only input and the binding would not have a clear way to specify which type. Currently it is an error for the dyndep = binding to name a path that is not an input so we would have an option in the future to replace the error case with an implied input.

@jhasse jhasse added the feature label Jan 29, 2019
@jhasse jhasse added this to the 1.10.0 milestone Jan 29, 2019
@bradking bradking force-pushed the bradking:dyndep branch from 874b248 to 6470fb6 Jan 31, 2019
@bradking

This comment has been minimized.

Copy link
Contributor Author

bradking commented Jan 31, 2019

Rebased on master for post-1.9 development.

@dublet

This comment has been minimized.

Copy link

dublet commented Feb 1, 2019

@bradking Is there any CMake branch where this feature is used?

@bradking

This comment has been minimized.

Copy link
Contributor Author

bradking commented Feb 1, 2019

@dublet CMake 3.7 and higher already have support. It is activated by recognizing a special .dyndep-1 suffix on Ninja's version number that Kitware added to our branch of Ninja we maintain pending upstream integration. One can activate it on top of this PR's branch by locally hacking src/version.cc:

-const char* kNinjaVersion = "1.9.0.git";
+const char* kNinjaVersion = "1.9.0.git.dyndep-1";

Of course once Ninja integrates this PR in a release then we can update CMake to check for that instead.

@dublet

This comment has been minimized.

Copy link

dublet commented Feb 1, 2019

@bradking Ah, thanks. This feature is something I'm after, though I'd like to use it with a custom_target/custom_command (well, ExternalProject in particular), so I guess CMake would need some tweaks to allow for that as well.

@bradking bradking force-pushed the bradking:dyndep branch from 6470fb6 to b4e2a16 Feb 4, 2019
@mathstuf

This comment has been minimized.

Copy link
Contributor

mathstuf commented Feb 8, 2019

This is probably future work, but outputting a build graph with dyndep information can help to find bottlenecks in the build. Is this already available with this PR or should I open an issue for it?

@bradking bradking force-pushed the bradking:dyndep branch 2 times, most recently from fec3c76 to b571790 Feb 12, 2019
@bradking

This comment has been minimized.

Copy link
Contributor Author

bradking commented Feb 12, 2019

@mathstuf I updated the branch here to load dyndep files for the clean, graph, and query tools.

Copy link
Collaborator

jhasse left a comment

Thanks again for this!

I just had a another quick look, sorry for the mostly nit-picky comments ;) I will have a close look in the coming weeks.

src/build.cc Outdated Show resolved Hide resolved
src/build.cc Outdated Show resolved Hide resolved
src/build_test.cc Outdated Show resolved Hide resolved
src/build_test.cc Show resolved Hide resolved
src/build_test.cc Outdated Show resolved Hide resolved
src/dyndep.h Outdated Show resolved Hide resolved
src/dyndep_parser.cc Show resolved Hide resolved
src/dyndep_parser.h Outdated Show resolved Hide resolved
src/parser.h Outdated Show resolved Hide resolved
src/dyndep.h Outdated Show resolved Hide resolved
@bradking bradking force-pushed the bradking:dyndep branch from b571790 to 1504d91 Apr 10, 2019
@bradking

This comment has been minimized.

Copy link
Contributor Author

bradking commented Apr 10, 2019

@jhasse thanks for the review. I've updated the branch to address all review comments, either with fixes or additional source comments with answers to your questions.

Copy link
Collaborator

jhasse left a comment

Thanks! I've added some more comments. Can you rebase on master, too?

doc/manual.asciidoc Outdated Show resolved Hide resolved
doc/manual.asciidoc Outdated Show resolved Hide resolved
src/build.h Show resolved Hide resolved
src/parser.h Outdated Show resolved Hide resolved
src/manifest_parser.h Outdated Show resolved Hide resolved
src/parser.h Show resolved Hide resolved
src/dyndep.h Outdated Show resolved Hide resolved
src/dyndep_parser.cc Outdated Show resolved Hide resolved
src/build_test.cc Outdated Show resolved Hide resolved
src/build_test.cc Outdated Show resolved Hide resolved
bradking added 10 commits Jun 18, 2015
Add an 'err' string argument and return a boolean for success.  Update
call sites to pass an 'err' string argument and check the return value.
This will be useful later for adding logic to these methods that may
fail.
In order to later support dynamic updates to the build plan while
building, the Plan will need access to its Builder.  Since this access
will be needed only for specific features we can avoid updating all Plan
constructions in the test suite by making this access optional.
Create a Parser base class that holds parser functionality not specific
to the build manifest file format.  This will allow it to be re-used for
other parsers later.
Move the logic to mark edges as wanted over to a Plan::EdgeWanted method
so it can be re-used elsewhere later.
Move the logic to a new Plan::EdgeMaybeReady method so it can be re-used
elsewhere.
Define a file format suitable for specifying dynamically-discovered
dependency information for build edges.  Design a format inspired by the
build manifest format and using the same lexer.  Start with a required
format version specification followed by "build" statements that add
implicit inputs and outputs to existing edges.
Allow rules or build statements to specify one of the build statement
inputs in a "dyndep" binding.  This will later be used to load
dependency information from the specified file.
Add a LoadDyndeps method to load a dyndep file and update
the edges that name it in their dyndep binding.
The full readiness of a node that has a dyndep binding cannot be known
until after the dyndep file is loaded.  If a dyndep file is ready while
constructing the build plan it can be loaded immediately so full
information can be used to decide whether anything needs to be built.
If a dyndep file is not ready while constructing the build plan then the
edges naming it cannot be ready either because the dyndep file is one of
their inputs.  In this case we defer loading the dyndep file until the
build plan is being executed.
Track for each Edge whether depfile information has been loaded using an
explicit flag.  This will allow RecomputeDirty to be repeated for an
edge without loading deps again.
bradking added 10 commits Dec 2, 2015
After finishing an edge that produces a dyndep file, load the file and
update the build graph structure.  Recompute the dirty state of all its
dependents and of newly reachable portions of the graph.  Add edges to
the build plan that are discovered to be wanted.  Finally, schedule
edges that are wanted and now ready to build.
Show a simple example of Fortran module dependencies (this use case
motivated the entire dyndep feature).  Also show an example of tarball
extraction, a case that few other buildsystems can handle cleanly.
Replace our single active edge pointer with a vector and add a
parameter that tests can set to limit the number of concurrent
edges.  Set the default to 1 to preserve the current behavior.
Specific tests will be able to override it later to simulate
concurrent builds.
This method should be called only with edges that have not
already been started.
Some outputs may not be known in the main build manifest and are instead
discovered through a dyndep binding.  Load dyndep files that are
available during cleaning so that we can clean these outputs too.
`Cleaner` provides two constructors that are the same except that one
constructs a "real" disk interface internally and the other takes a
caller-provided disk interface.  A real disk interface is already
available at the only call site for the former constructor.  Use it
directly and drop the unnecessary constructor variant.
Teach the `-t graph` tool to load dyndep files because they are part of
the build graph.  Issue a warning when the dyndep file cannot be loaded
cleanly.  This will help users visualize the complete build graph.
@bradking bradking force-pushed the bradking:dyndep branch from 1504d91 to 1d55d05 Apr 18, 2019
@bradking

This comment has been minimized.

Copy link
Contributor Author

bradking commented Apr 18, 2019

@jhasse thanks for the review. I've updated the branch to address all review comments and rebase on master.

@jhasse jhasse merged commit 2e64645 into ninja-build:master Apr 20, 2019
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jhasse

This comment has been minimized.

Copy link
Collaborator

jhasse commented Apr 20, 2019

Awesome stuff! Again: Thank you so much for the hard work :)

@bradking bradking deleted the bradking:dyndep branch Apr 22, 2019
@bradking

This comment has been minimized.

Copy link
Contributor Author

bradking commented Apr 22, 2019

@jhasse great, thanks for the review and integration!

@Ericson2314

This comment has been minimized.

Copy link

Ericson2314 commented Oct 3, 2019

#1303 Per this long thread, I think it would be great to have an optional warning for depfile saying to use dyndep instead. I'm really stoked for dyndep; I would always like to use it instead of depfile so initial builds and rebuilds do the same thing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.