Add include target option to inputs tool #2524
Conversation
The '-i' option will list all <target>:<inputs> for the given targets. Run: ninja -t inputs -i <target1> <target2> <target3> Ninja will then output: <target1>:<input_x> <target1>:<input_y> <target2>:<input_x> <target2>:<input_z> <target3>:<input_y>
|
Please feel free to ask if you need more information about why this feature might be useful by ninja users. |
| } else { | ||
| for (const std::string& input : inputs) | ||
| puts(input.c_str()); | ||
| } |
There was a problem hiding this comment.
Avoid code duplication by defining a local lambda that captures shell_escape and dependency_order, takes an InputsCollector& and const char* prefix as input and does the printing. You'll be able to call it from the two branches of your if (include_target) condition.
| @@ -804,6 +809,7 @@ int NinjaMain::ToolInputs(const Options* options, int argc, char* argv[]) { | |||
| " -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" | |||
| " -i, --include-target Add target before result in every line, separated by the : character.\n" | |||
There was a problem hiding this comment.
If I understand correctly, this is a convenience feature to avoid calling ninja -t inputs <target> in a loop, by reusing the same build plan loaded from memory.
However, it drastically changes the behavior of this tool as it now performs one graph traversal per target, instead of a single one, and changes the output considerably: in addition to the prefix, actual input paths will be duplicated when they are used by more than one target, which does not happen when not using this flag.
I think the documentation should reflect that much more clearly.
There was a problem hiding this comment.
I thus suggest finding a more descriptive name than -include-target. for example --separate-target-inputs, wdyt?
There was a problem hiding this comment.
Finally, this needs a regression test in misc/output_test.py. Please add the correspond test cases to test_tool_inputs() in that file.
There was a problem hiding this comment.
Also, are we sure that target names are never going to include a ': in their path? Maybe use a TAB character as the separator instead, at least these are forbidden from Ninja build plans at the moment. Or allow the user to provide an alternative separator here? Just thinking out loud, let me know what you think.
There was a problem hiding this comment.
I started out making a pull request on with this feature as a completely different tool. That was before the (very nice) feature InputsCollector collector was added to the inputs tool.
When the InputsCollector was added I saw that it could be added in the same tool but with very few (be as you say, drastically different) lines of code.
If I would choose I would like to implement it as a new tool (name TBD, any good names? last time I called it targetinputs). If it is implemented as a new tool, do we want the print0 / escape features in that one as well.
It will be much easier to document and to add unittests for a different/new tool.
I have no opinion whatsoever about the delimiter, it can be chosen by the user.
There was a problem hiding this comment.
Yeah, a new tool seems to make sense. What about multi-inputs to clarify that it provides different sets of inputs?
Regarding flags/features:
-
Make
--dependency-orderthe default behavior. Historically, this flag was added toinputsto make the tool faster (this is important for me too :-)). The initial implementation (beforeInputsCollector) was actually slightly broken and would collect duplicate targets, sostd::sort()+std::unique()calls were needed to get rid of them. I don't see why keeping the output sorted in the new tool would be useful, though you could add a--sortoption if you really like it. -
Do not implement escaping, but do support
--print0or--format=print0for scripts. It is trivial to implement, and it's a better alternative imho.
Regarding the delimiter, I think TAB is a good default choice, with the ability to use a different one with a flag. Another alternative is to provide a --format=json flag to output a JSON object that maps target paths to list of inputs instead (but the result will be significantly slower to generate, and to consume).
There was a problem hiding this comment.
New PR created #2527
|
Dropping this in favor of #2527 |
This feature can be used if one would like to to a reverse search on what
targets that will be built by ninja (for example from the file listing of a git commit)
Instead of just listing all the inputs (that ninja -t inputs do) it will also list the target that use that specific input.
ninja -t inputs -i targetA targetB targetC
Ninja will then output:
targetA:file1.c
targetB:file1.c
targetC:file1.c
targetB:file2.c
targetC:file2.c
Using the list above:
If we have a git commit that changes file1 we know that we want to test everything related to targetA, targetB, targetC.
on the other hand if we have a git commit that changes file2 we only have to test everything related to targetB and targetC.
We use this feature a lot to get the correct (reduce the) test scope of a certain git commit.