From 7e03348fe303d9af4a32be07f0558ee57e19c0e4 Mon Sep 17 00:00:00 2001 From: David 'Digit' Turner Date: Thu, 29 Aug 2024 11:02:47 +0200 Subject: [PATCH 1/3] Add InputsCollector class. Add a new class to wrap the logic needed to implement the `inputs` tool correctly (see Issue #2482 for details), and provide a unit-test for it. --- src/graph.cc | 44 ++++++++++++++++++++++ src/graph.h | 37 ++++++++++++++++++ src/graph_test.cc | 95 +++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 176 insertions(+) diff --git a/src/graph.cc b/src/graph.cc index 143eabdfb4..072e3d1365 100644 --- a/src/graph.cc +++ b/src/graph.cc @@ -779,3 +779,47 @@ vector::iterator ImplicitDepLoader::PreallocateSpace(Edge* edge, edge->implicit_deps_ += count; return edge->inputs_.end() - edge->order_only_deps_ - count; } + +void InputsCollector::VisitNode(const Node* node) { + const Edge* edge = node->in_edge(); + + if (!edge) // A source file. + return; + + // Add inputs of the producing edge to the result, + // except if they are themselves produced by a phony + // edge. + for (const Node* input : edge->inputs_) { + if (!visited_nodes_.insert(input).second) + continue; + + VisitNode(input); + + const Edge* input_edge = input->in_edge(); + if (!(input_edge && input_edge->is_phony())) { + inputs_.push_back(input); + } + } +} + +std::vector InputsCollector::GetInputsAsStrings( + bool shell_escape) const { + std::vector result; + result.reserve(inputs_.size()); + + for (const Node* input : inputs_) { + std::string unescaped = input->PathDecanonicalized(); + if (shell_escape) { + std::string path; +#ifdef _WIN32 + GetWin32EscapedString(unescaped, &path); +#else + GetShellEscapedString(unescaped, &path); +#endif + result.push_back(std::move(path)); + } else { + result.push_back(std::move(unescaped)); + } + } + return result; +} diff --git a/src/graph.h b/src/graph.h index 314c44296a..5af85a27a7 100644 --- a/src/graph.h +++ b/src/graph.h @@ -425,4 +425,41 @@ class EdgePriorityQueue: } }; +/// A class used to collect the transitive set of inputs from a given set +/// of starting nodes. Used to implement the `inputs` tool. +/// +/// When collecting inputs, the outputs of phony edges are always ignored +/// from the result, but are followed by the dependency walk. +/// +/// Usage is: +/// - Create instance. +/// - Call VisitNode() for each root node to collect inputs from. +/// - Call inputs() to retrieve the list of input node pointers. +/// - Call GetInputsAsStrings() to retrieve the list of inputs as a string +/// vector. +/// +struct InputsCollector { + /// Visit a single @arg node during this collection. + void VisitNode(const Node* node); + + /// Retrieve list of visited input nodes. A dependency always appears + /// before its dependents in the result, but final order depends on the + /// order of the VisitNode() calls performed before this. + const std::vector& inputs() const { return inputs_; } + + /// Same as inputs(), but returns the list of visited nodes as a list of + /// strings, with optional shell escaping. + std::vector GetInputsAsStrings(bool shell_escape = false) const; + + /// Reset collector state. + void Reset() { + inputs_.clear(); + visited_nodes_.clear(); + } + + private: + std::vector inputs_; + std::set visited_nodes_; +}; + #endif // NINJA_GRAPH_H_ diff --git a/src/graph_test.cc b/src/graph_test.cc index f909b906fd..2b858f5716 100644 --- a/src/graph_test.cc +++ b/src/graph_test.cc @@ -248,6 +248,101 @@ TEST_F(GraphTest, CollectInputs) { EXPECT_EQ("order_only", inputs[4]); } +TEST_F(GraphTest, InputsCollector) { + // Build plan for the following graph: + // + // in1 + // |___________ + // | | + // === === + // | | + // out1 mid1 + // | ____|_____ + // | | | + // | === ======= + // | | | | + // | out2 out3 out4 + // | | | + // =======phony====== + // | + // all + // + ASSERT_NO_FATAL_FAILURE(AssertParse(&state_, + "build out1: cat in1\n" + "build mid1: cat in1\n" + "build out2: cat mid1\n" + "build out3 out4: cat mid1\n" + "build all: phony out1 out2 out3\n")); + + InputsCollector collector; + + // Start visit from out1, this should add in1 to the inputs. + collector.Reset(); + collector.VisitNode(GetNode("out1")); + auto inputs = collector.GetInputsAsStrings(); + ASSERT_EQ(1u, inputs.size()); + EXPECT_EQ("in1", inputs[0]); + + // Add a visit from out2, this should add mid1. + collector.VisitNode(GetNode("out2")); + inputs = collector.GetInputsAsStrings(); + ASSERT_EQ(2u, inputs.size()); + EXPECT_EQ("in1", inputs[0]); + EXPECT_EQ("mid1", inputs[1]); + + // Another visit from all, this should add out1, out2 and out3, + // but not out4. + collector.VisitNode(GetNode("all")); + inputs = collector.GetInputsAsStrings(); + ASSERT_EQ(5u, inputs.size()); + EXPECT_EQ("in1", inputs[0]); + EXPECT_EQ("mid1", inputs[1]); + EXPECT_EQ("out1", inputs[2]); + EXPECT_EQ("out2", inputs[3]); + EXPECT_EQ("out3", inputs[4]); + + collector.Reset(); + + // Starting directly from all, will add out1 before mid1 compared + // to the previous example above. + collector.VisitNode(GetNode("all")); + inputs = collector.GetInputsAsStrings(); + ASSERT_EQ(5u, inputs.size()); + EXPECT_EQ("in1", inputs[0]); + EXPECT_EQ("out1", inputs[1]); + EXPECT_EQ("mid1", inputs[2]); + EXPECT_EQ("out2", inputs[3]); + EXPECT_EQ("out3", inputs[4]); +} + +TEST_F(GraphTest, InputsCollectorWithEscapes) { + ASSERT_NO_FATAL_FAILURE(AssertParse( + &state_, + "build out$ 1: cat in1 in2 in$ with$ space | implicit || order_only\n")); + + InputsCollector collector; + collector.VisitNode(GetNode("out 1")); + auto inputs = collector.GetInputsAsStrings(); + ASSERT_EQ(5u, inputs.size()); + EXPECT_EQ("in1", inputs[0]); + EXPECT_EQ("in2", inputs[1]); + EXPECT_EQ("in with space", inputs[2]); + EXPECT_EQ("implicit", inputs[3]); + EXPECT_EQ("order_only", inputs[4]); + + inputs = collector.GetInputsAsStrings(true); + ASSERT_EQ(5u, inputs.size()); + EXPECT_EQ("in1", inputs[0]); + EXPECT_EQ("in2", inputs[1]); +#ifdef _WIN32 + EXPECT_EQ("\"in with space\"", inputs[2]); +#else + EXPECT_EQ("'in with space'", inputs[2]); +#endif + EXPECT_EQ("implicit", inputs[3]); + EXPECT_EQ("order_only", inputs[4]); +} + TEST_F(GraphTest, VarInOutPathEscaping) { ASSERT_NO_FATAL_FAILURE(AssertParse(&state_, "build a$ b: cat no'space with$ space$$ no\"space2\n")); From 5b94d3407fe2f77e462bcb7f35890318e1a215d6 Mon Sep 17 00:00:00 2001 From: David 'Digit' Turner Date: Thu, 29 Aug 2024 11:14:32 +0200 Subject: [PATCH 2/3] Fix `inputs` tool implementation. This uses the InputsCollector class introduced in the previous patch to implement the tool properly. Results are still shell-escaped and sorted alphabetically. Fixed #2482 --- misc/output_test.py | 21 +++++++++++++++++++++ src/graph.cc | 22 ---------------------- src/graph.h | 3 --- src/graph_test.cc | 33 --------------------------------- src/ninja.cc | 42 +++++++++++++----------------------------- 5 files changed, 34 insertions(+), 87 deletions(-) diff --git a/misc/output_test.py b/misc/output_test.py index cba3ff02fa..b82664c3d7 100755 --- a/misc/output_test.py +++ b/misc/output_test.py @@ -213,6 +213,27 @@ def test_tool_inputs(self) -> None: out2 ''') + # Verify that results are shell-escaped by default. + # Also verify that phony outputs are never part of the results. + quote = '"' if platform.system() == "Windows" else "'" + + plan = ''' +rule cat + command = cat $in $out +build out1 : cat in1 +build out$ 2 : cat out1 +build out$ 3 : phony out$ 2 +build all: phony out$ 3 +''' + + self.assertEqual(run(plan, flags='-t inputs all'), +f'''{quote}out 2{quote} +in1 +out1 +''') + + + def test_explain_output(self): b = BuildDir('''\ build .FORCE: phony diff --git a/src/graph.cc b/src/graph.cc index 072e3d1365..f04ffb47c8 100644 --- a/src/graph.cc +++ b/src/graph.cc @@ -496,28 +496,6 @@ std::string EdgeEnv::MakePathList(const Node* const* const span, return result; } -void Edge::CollectInputs(bool shell_escape, - std::vector* out) const { - for (std::vector::const_iterator it = inputs_.begin(); - it != inputs_.end(); ++it) { - std::string path = (*it)->PathDecanonicalized(); - if (shell_escape) { - std::string unescaped; - unescaped.swap(path); -#ifdef _WIN32 - GetWin32EscapedString(unescaped, &path); -#else - GetShellEscapedString(unescaped, &path); -#endif - } -#if __cplusplus >= 201103L - out->push_back(std::move(path)); -#else - out->push_back(path); -#endif - } -} - std::string Edge::EvaluateCommand(const bool incl_rsp_file) const { string command = GetBinding("command"); if (incl_rsp_file) { diff --git a/src/graph.h b/src/graph.h index 5af85a27a7..806260e5d7 100644 --- a/src/graph.h +++ b/src/graph.h @@ -201,9 +201,6 @@ struct Edge { void Dump(const char* prefix="") const; - // Append all edge explicit inputs to |*out|. Possibly with shell escaping. - void CollectInputs(bool shell_escape, std::vector* out) const; - // critical_path_weight is the priority during build scheduling. The // "critical path" between this edge's inputs and any target node is // the path which maximises the sum oof weights along that path. diff --git a/src/graph_test.cc b/src/graph_test.cc index 2b858f5716..6c654eeb32 100644 --- a/src/graph_test.cc +++ b/src/graph_test.cc @@ -215,39 +215,6 @@ TEST_F(GraphTest, RootNodes) { } } -TEST_F(GraphTest, CollectInputs) { - ASSERT_NO_FATAL_FAILURE(AssertParse( - &state_, - "build out$ 1: cat in1 in2 in$ with$ space | implicit || order_only\n")); - - std::vector inputs; - Edge* edge = GetNode("out 1")->in_edge(); - - // Test without shell escaping. - inputs.clear(); - edge->CollectInputs(false, &inputs); - EXPECT_EQ(5u, inputs.size()); - EXPECT_EQ("in1", inputs[0]); - EXPECT_EQ("in2", inputs[1]); - EXPECT_EQ("in with space", inputs[2]); - EXPECT_EQ("implicit", inputs[3]); - EXPECT_EQ("order_only", inputs[4]); - - // Test with shell escaping. - inputs.clear(); - edge->CollectInputs(true, &inputs); - EXPECT_EQ(5u, inputs.size()); - EXPECT_EQ("in1", inputs[0]); - EXPECT_EQ("in2", inputs[1]); -#ifdef _WIN32 - EXPECT_EQ("\"in with space\"", inputs[2]); -#else - EXPECT_EQ("'in with space'", inputs[2]); -#endif - EXPECT_EQ("implicit", inputs[3]); - EXPECT_EQ("order_only", inputs[4]); -} - TEST_F(GraphTest, InputsCollector) { // Build plan for the following graph: // diff --git a/src/ninja.cc b/src/ninja.cc index 2902359f15..cb4f5865df 100644 --- a/src/ninja.cc +++ b/src/ninja.cc @@ -761,27 +761,12 @@ int NinjaMain::ToolCommands(const Options* options, int argc, char* argv[]) { return 0; } -void CollectInputs(Edge* edge, std::set* seen, - std::vector* result) { - if (!edge) - return; - if (!seen->insert(edge).second) - return; - - for (vector::iterator in = edge->inputs_.begin(); - in != edge->inputs_.end(); ++in) - CollectInputs((*in)->in_edge(), seen, result); - - if (!edge->is_phony()) { - edge->CollectInputs(true, result); - } -} - int NinjaMain::ToolInputs(const Options* options, int argc, char* argv[]) { // The inputs tool uses getopt, and expects argv[0] to contain the name of // the tool, i.e. "inputs". argc++; argv--; + optind = 1; int opt; const option kLongOptions[] = { { "help", no_argument, NULL, 'h' }, @@ -794,8 +779,9 @@ int NinjaMain::ToolInputs(const Options* options, int argc, char* argv[]) { printf( "Usage '-t inputs [options] [targets]\n" "\n" -"List all inputs used for a set of targets. Note that this includes\n" -"explicit, implicit and order-only inputs, but not validation ones.\n\n" +"List all inputs used for a set of targets.\n" +"Note that results are shell escaped, and sorted alphabetically,\n" +"and never include validation target paths.\n\n" "Options:\n" " -h, --help Print this message.\n"); // clang-format on @@ -805,24 +791,22 @@ int NinjaMain::ToolInputs(const Options* options, int argc, char* argv[]) { argv += optind; argc -= optind; - vector nodes; - string err; + std::vector nodes; + std::string err; if (!CollectTargetsFromArgs(argc, argv, &nodes, &err)) { Error("%s", err.c_str()); return 1; } - std::set seen; - std::vector result; - for (vector::iterator in = nodes.begin(); in != nodes.end(); ++in) - CollectInputs((*in)->in_edge(), &seen, &result); + InputsCollector collector; + for (const Node* node : nodes) + collector.VisitNode(node); - // Make output deterministic by sorting then removing duplicates. - std::sort(result.begin(), result.end()); - result.erase(std::unique(result.begin(), result.end()), result.end()); + std::vector inputs = collector.GetInputsAsStrings(true); + std::sort(inputs.begin(), inputs.end()); - for (size_t n = 0; n < result.size(); ++n) - puts(result[n].c_str()); + for (const std::string& input : inputs) + puts(input.c_str()); return 0; } From 284349311d77eae19947aa7bfc849608b28f173b Mon Sep 17 00:00:00 2001 From: David 'Digit' Turner Date: Thu, 29 Aug 2024 11:16:32 +0200 Subject: [PATCH 3/3] Add new options to `inputs` tool. Add new options to the `inputs` tool in order to change the format of its output: - `--no-shell-escape` to avoid shell-escaping the results. - `--dependency-order` to return results in dependency order, instead of sorting them alphabetically. - `--print0` to use \0 as the list separator in the list, useful to process the target paths with `xargs -0` and similar tools. --- misc/output_test.py | 48 +++++++++++++++++++++++++++++++++++++++++++-- src/ninja.cc | 47 +++++++++++++++++++++++++++++++++++--------- 2 files changed, 84 insertions(+), 11 deletions(-) diff --git a/misc/output_test.py b/misc/output_test.py index b82664c3d7..7d8face51e 100755 --- a/misc/output_test.py +++ b/misc/output_test.py @@ -213,8 +213,17 @@ def test_tool_inputs(self) -> None: out2 ''') - # Verify that results are shell-escaped by default. - # Also verify that phony outputs are never part of the results. + self.assertEqual(run(plan, flags='-t inputs --dependency-order out3'), +'''in2 +in1 +out1 +out2 +implicit +order_only +''') + + # Verify that results are shell-escaped by default, unless --no-shell-escape + # is used. Also verify that phony outputs are never part of the results. quote = '"' if platform.system() == "Windows" else "'" plan = ''' @@ -226,12 +235,47 @@ def test_tool_inputs(self) -> None: build all: phony out$ 3 ''' + # Quoting changes the order of results when sorting alphabetically. self.assertEqual(run(plan, flags='-t inputs all'), f'''{quote}out 2{quote} in1 out1 ''') + self.assertEqual(run(plan, flags='-t inputs --no-shell-escape all'), +'''in1 +out 2 +out1 +''') + + # But not when doing dependency order. + self.assertEqual( + run( + plan, + flags='-t inputs --dependency-order all' + ), + f'''in1 +out1 +{quote}out 2{quote} +''') + + self.assertEqual( + run( + plan, + flags='-t inputs --dependency-order --no-shell-escape all' + ), + f'''in1 +out1 +out 2 +''') + + self.assertEqual( + run( + plan, + flags='-t inputs --dependency-order --no-shell-escape --print0 all' + ), + f'''in1\0out1\0out 2\0''' + ) def test_explain_output(self): diff --git a/src/ninja.cc b/src/ninja.cc index cb4f5865df..9b77679786 100644 --- a/src/ninja.cc +++ b/src/ninja.cc @@ -767,23 +767,44 @@ int NinjaMain::ToolInputs(const Options* options, int argc, char* argv[]) { argc++; argv--; + bool print0 = false; + bool shell_escape = true; + bool dependency_order = false; + optind = 1; int opt; const option kLongOptions[] = { { "help", no_argument, NULL, 'h' }, + { "no-shell-escape", no_argument, NULL, 'E' }, + { "print0", no_argument, NULL, '0' }, + { "dependency-order", no_argument, NULL, + 'd' }, { NULL, 0, NULL, 0 } }; - while ((opt = getopt_long(argc, argv, "h", kLongOptions, NULL)) != -1) { + while ((opt = getopt_long(argc, argv, "h0Ed", kLongOptions, NULL)) != -1) { switch (opt) { + case 'd': + dependency_order = true; + break; + case 'E': + shell_escape = false; + break; + case '0': + print0 = true; + break; case 'h': default: // clang-format off printf( "Usage '-t inputs [options] [targets]\n" "\n" -"List all inputs used for a set of targets.\n" -"Note that results are shell escaped, and sorted alphabetically,\n" +"List all inputs used for a set of targets, sorted in dependency order.\n" +"Note that by default, results are shell escaped, and sorted alphabetically,\n" "and never include validation target paths.\n\n" "Options:\n" -" -h, --help Print this message.\n"); +" -h, --help Print this message.\n" +" -0, --print0 Use \\0, instead of \\n as a line terminator.\n" +" -E, --no-shell-escape Do not shell escape the result.\n" +" -d, --dependency-order Sort results by dependency order.\n" + ); // clang-format on return 1; } @@ -802,12 +823,20 @@ int NinjaMain::ToolInputs(const Options* options, int argc, char* argv[]) { for (const Node* node : nodes) collector.VisitNode(node); - std::vector inputs = collector.GetInputsAsStrings(true); - std::sort(inputs.begin(), inputs.end()); - - for (const std::string& input : inputs) - puts(input.c_str()); + std::vector inputs = collector.GetInputsAsStrings(shell_escape); + if (!dependency_order) + std::sort(inputs.begin(), inputs.end()); + if (print0) { + for (const std::string& input : inputs) { + fwrite(input.c_str(), input.size(), 1, stdout); + fputc('\0', stdout); + } + fflush(stdout); + } else { + for (const std::string& input : inputs) + puts(input.c_str()); + } return 0; }