From 9626cceaf851bb94112721d156cfd0d28b0d7037 Mon Sep 17 00:00:00 2001 From: Elliot Goodrich Date: Wed, 9 Oct 2024 06:24:14 +0100 Subject: [PATCH] Improve order-only dependency behaviour For incremental builds, changes in order-only dependencies do not cause a build command to be rerun by ninja. The idea being that after their first run, their real dependencies have been loaded into `.ninja_deps` and, as they are more accurate, can be used instead. trimja is trying to emulate an incremental build to rebuild only those commands affected by a certain set of files. So if an affected file is mentioned as an order-only dependencies, but it doesn't appear for that output in `.ninja_deps` - it wasn't **actually** needed. This should reduce the number of build commands that we include in the trimmed file. Additionally, there was a bug where we incorrectly included the order-only dependencies in the `$in` variables - `out5` in the dependencies test case should cover this. --- src/graph.cpp | 5 +++++ src/graph.h | 13 +++++++++++-- src/trimutil.cpp | 14 +++++++++++++- tests/dependencies/build.ninja | 1 + tests/dependencies/expected.ninja | 9 +++++---- tests/phony/build.ninja | 2 +- tests/phony/expected.ninja | 2 +- 7 files changed, 37 insertions(+), 9 deletions(-) diff --git a/src/graph.cpp b/src/graph.cpp index b9a9180..8488b08 100644 --- a/src/graph.cpp +++ b/src/graph.cpp @@ -137,6 +137,11 @@ void Graph::addEdge(std::size_t in, std::size_t out) { m_outputToInput[out].insert(in); } +void Graph::addOneWayEdge(std::size_t in, std::size_t out) { + assert(in != out); + m_inputToOutput[in].insert(out); +} + bool Graph::isDefault(std::size_t pathIndex) const { return pathIndex == m_defaultIndex; } diff --git a/src/graph.h b/src/graph.h index 0a262ef..260bc47 100644 --- a/src/graph.h +++ b/src/graph.h @@ -112,12 +112,20 @@ class Graph { std::size_t addDefault(); /** - * @brief Adds an edge between the specified input and output nodes. + * @brief Adds bidirectional edge between the specified input and output + * nodes. * @param in The index of the input node. * @param out The index of the output node. */ void addEdge(std::size_t in, std::size_t out); + /** + * @brief Adds an one-way edge from the specified input to output node. + * @param in The index of the input node. + * @param out The index of the output node. + */ + void addOneWayEdge(std::size_t in, std::size_t out); + /** * @brief Checks if the specified path index is the default node. * @param pathIndex The index of the path to check. @@ -146,7 +154,8 @@ class Graph { const std::set& out(std::size_t pathIndex) const; /** - * @brief Gets the set of input nodes for the specified path index. + * @brief Gets the set of input nodes for the specified path index. Note that + * this does not include order-only dependencies. * @param pathIndex The index of the path. * @return The set of input nodes. */ diff --git a/src/trimutil.cpp b/src/trimutil.cpp index 3a4158a..4eaa6ff 100644 --- a/src/trimutil.cpp +++ b/src/trimutil.cpp @@ -224,8 +224,10 @@ struct BuildContext { for (const EvalString& path : r.implicitIn()) { ins.push_back(evaluatePath(path)); } + + std::vector orderOnlyDeps; for (const EvalString& path : r.orderOnlyDeps()) { - ins.push_back(evaluatePath(path)); + orderOnlyDeps.push_back(evaluatePath(path)); } // Collect validations but ignore what they are. If we include a build @@ -273,6 +275,16 @@ struct BuildContext { } } + // We only need to add an input to output edge and not a bidirectional + // one for order-only dependencies. This is because we only include a + // build edge if an input (implicit or not) is affected. + for (std::string& orderOnlyDep : orderOnlyDeps) { + const std::size_t inIndex = getPathIndex(orderOnlyDep); + for (const std::size_t outIndex : outIndices) { + graph.addOneWayEdge(inIndex, outIndex); + } + } + { std::string command; scope.appendValue(command, "command"); diff --git a/tests/dependencies/build.ninja b/tests/dependencies/build.ninja index 3340bdc..1aad7a9 100644 --- a/tests/dependencies/build.ninja +++ b/tests/dependencies/build.ninja @@ -4,6 +4,7 @@ build out1: copy in build out2: copy foo | in build out3: copy foo || in build out4: copy foo | bar || in +build out5: copy foo || bar build extra1: copy out1 build extra2: copy foo | out2 build extra3: copy foo || out3 diff --git a/tests/dependencies/expected.ninja b/tests/dependencies/expected.ninja index 3340bdc..1ce8081 100644 --- a/tests/dependencies/expected.ninja +++ b/tests/dependencies/expected.ninja @@ -2,9 +2,10 @@ rule copy command = ninja --version $in -> $out build out1: copy in build out2: copy foo | in -build out3: copy foo || in -build out4: copy foo | bar || in +build out3: phony +build out4: phony +build out5: phony build extra1: copy out1 build extra2: copy foo | out2 -build extra3: copy foo || out3 -build extra4: copy foo | bar || out4 +build extra3: phony +build extra4: phony diff --git a/tests/phony/build.ninja b/tests/phony/build.ninja index 3c68653..e39cfaa 100644 --- a/tests/phony/build.ninja +++ b/tests/phony/build.ninja @@ -10,4 +10,4 @@ build out5: copy affected3 build out6: copy notAffected3 build out7: copy notAffected4 build bootstrap: phony out5 out6 -build topout: copy out7 || bootstrap +build topout: copy out7 bootstrap diff --git a/tests/phony/expected.ninja b/tests/phony/expected.ninja index 0b1b33d..b8f4f8a 100644 --- a/tests/phony/expected.ninja +++ b/tests/phony/expected.ninja @@ -10,4 +10,4 @@ build out5: copy affected3 build out6: copy notAffected3 build out7: copy notAffected4 build bootstrap: phony out5 out6 -build topout: copy out7 || bootstrap +build topout: copy out7 bootstrap