-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Introduce a new tool: ninja -t compdb-targets
#2497
base: master
Are you sure you want to change the base?
Conversation
squash to 1 commit to get rid of the merge commits and combine the original with your rename, add yourself to the tags at the end of the original commit it would be nice if we can find a way to get rid of I still think the remaining arguments should not require a long option at all or otherwise |
@mcprat Agreed on squashing/rebasing, and i will look into the as for the command-line syntax: i wrote about this in #2319 (though i forgot to tag you there; sorry!), but it would be good to have it inline here too: You wrote:
Prior to the commits in this PR, the output of
so
but that means the syntax would become:
...which means that users will need to remember that How would you feel about a short option instead of a long option? (meaning: Also: if we go with an option, are you ok with making the target names comma-delimited? (i think the main reason to use commas is so that we stick to the format expected by Fwiw, in bash 4.2 and later, you can join an array with commas using the printf builtin like so:
|
@mcprat oh wait, when you wrote:
are you proposing something like:
...or maybe:
...? |
@mcprat btw, i assumed that we don't want to break the pre-existing Do we want to make the breaking change of removing the |
@mcprat i just thought of another way we could do this: up until now, i've been stuck thinking that
...where the
...which, although it would add an option, it would at least mean that targets are separated by spaces as you requested. (In this case, i would have a weak preference for the long form ( |
@JamesWidman I'm aware of your replies in the other PR. I was reiterating, but also clarifying that neither a short or long option would be the ideal (closer to POSIX-like behavior), because it seems like what I said previously was interpreted as only preferring short option or All of this definitely clears things up, it is more complicated than I thought... Here's an idea... what's stopping the parsing stage from checking both the list of targets and list of rules for each argument? Without having looked into it, there's probably different functions used depending on whether the argument is a rule or a target. Maybe each argument can be passed to both, a failure can be caught, and if only one has a problem that's expected, but both failing (or even if neither are failing) would indicate an actual problem. |
or rather, since this is a printing-only tool, it can just ignore any errors... |
@mcprat I am trying to understand:
(I do plan on getting to your other points, but first, i really want to understand the answers to these questions.) |
it's not really an option but rather more input, as it doesn't adjust existing outputs but rather just adds to it. consider the way the manual is written right now
instead of now having to describe that there's different ways that one adds a rule to the list vs how one adds a target to the list, it would be nice if the manual can just say:
I consider the design of POSIX has a section called "Utility Syntax Guidelines" that's used to form the standard syntax for |
@mcprat First: i think my confusion might be shrinking; thank you! i didn't quite understand what you meant here:
...but i think i understand this part:
so... if i understand you correctly, it sounds like you're saying something like: Is that about right? (If so: i see that the consistency that you're asking for could make it easier for a user to transition from using rules to using targets or vice-versa, and that would end the confusion that prompted my first question above.) Second:
i started reading Issue 8 (IEEE Std 1003.1™-2024 Edition) as well as a draft from 1991, and i just realized that you might be targeting some other version in between (one that is old enough that it is implemented by all the platforms the ninja project wants to run on, but new enough that it specifies all the OS features that the project wants ninja to depend on). i checked so: what version of POSIX should we be reading/targeting? Also: would it make sense to add a mention of that version of POSIX to the wiki and/or |
src/ninja.cc
Outdated
@@ -945,6 +948,32 @@ void printCompdb(const char* const directory, const Edge* const edge, | |||
printf("\"\n }"); | |||
} | |||
|
|||
struct DependentEdgeCollector { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please move this class definition to src/graph.h
and provide proper documentation + unit-tests for it. See InputsCollector
there for inspiration.
I also recommend to make depend_edges
a member of the class, and add a method like TakeResult()
to move the final result out of the instance one all root targets have been visited.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please move this class definition to
src/graph.h
is it ok if i move it to a new header file instead? It's very compdb
-specific, and only used by ninja.cc
and graph_test.cc
, so it would be nice if changing it didn't cause a lot of unrelated source files to be recompiled
src/ninja.cc
Outdated
std::vector<Edge*> user_interested_edges; | ||
DependentEdgeCollector deps_edge_collector; | ||
|
||
while (std::getline(iss, target, ',')) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use SplitStringPiece()
from string_piece_util.h
instead for simpler and smaller generated code. Use std::
prefixes for all standard types, as we are trying to move away from using namespace std
. E.g.:
for (StringPiece target_piece : SplitStringPiece(optarg, ',')) {
std::string err;
std::string target = target_piece.AsString();
Node* node = CollectTarget(target.c_str(), &err);
...
}
src/ninja.cc
Outdated
return 1; | ||
} | ||
if (!deps_edge_collector.CollectFrom(node, &user_interested_edges)) { | ||
return 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Silent failure without a message explaning what's going on is a bad user experience :-/
Besides, it looks like your CollectFrom() implementation cannot return false, so get rid of the return value and the condition check here.
src/ninja.cc
Outdated
@@ -945,6 +948,32 @@ void printCompdb(const char* const directory, const Edge* const edge, | |||
printf("\"\n }"); | |||
} | |||
|
|||
struct DependentEdgeCollector { | |||
bool CollectFrom(Node* node, std::vector<Edge*>* depend_edges) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: use const Node*
for the first argument :-)
We do really not care about following POSIX guidelines or imitating the GNU Make design. As such, changing how non-option arguments to It is ok to introduce new options, even if they change the tool's behavior, as long as everything is properly documented, and there is no place for ambiguity. If this is not possible, or makes the UI too cluncky, introducing a new tool might be the better option. |
In this specific case, using |
@digit-google Works for me, but is there consensus among maintainers on this point? (I got the sense from @mcprat that some POSIX-ness and Make-ness was desired. I don't feel a strong preference either way, but it would help to have unambiguous direction.)
[thumbs up]
So for example (and please correct me if i'm wrong):
[edit: ...or would item (2) here be viable because it uses a new option that changes the tool's behavior?]
Sounds good to me!
... and by "tool" you mean a new sub-command, right? For example, maybe like:
|
Yes, except for 2, since Personally, I do not care about sticking to Posix guidelines. Ninja is supposed to be cross-platform anyway, and there are plenty of command-line parsers (like python I also forgot to ask for a regression test for the tool in |
@digit-google btw, what's our C++ version for compiling ninja? (i grepped for i see uses of i ask because i wanted to use a structured binding in a place where i thought it would help to add clarity, but that's a C++17 feature, which would require setting Note, according to the clang website:
...and likewise, GCC 11 defaults to C++17. So if we're not going to depend on C++14 or C++17 features, we may want to set |
IIRC, the goal is to have the core Ninja sources building with C++11 support only, in order to allow building the tool on older systems (in particular with However, the tests now rely on GTest which mandates C++14 though (and soon C++17), which is why CXX_STANDARD is not applied to them in our I think these requirements were in the top-level I wouldn't mind switching to C++17 personally, but @jhasse is the authority on this specific issue (and this may require updating some of our CI recipes). |
@digit-google @jhasse this is a little off-topic for this PR, but: what's our policy on GNU core-language extensions)? some pros/cons (in case this hasn't come up before): pros:
con:
|
I believe right now we absolutely want to build with the MSVC toolchain, and support non-GNU and non-Clang toolchains, so these are a deal breakers. |
so it turns out that this is doable, but it requires using at the moment i'm working on regression tests and documentation, but here's a preview of the argument parsing routine (for early feedback on the general shape of this approach): (Before my next push, i will review the coding style guidelines, so hopefully that push will not contain e.g. any naming style violations that might appear below.) Compdb parseCompdbArguments(int argc, char* argv[]) {
Compdb ret;
//
// grammar:
// ninja -t compdb [-hx] [rules] [--targets targets]
//
// in phase 1 of argument parsing, we process [-hx] and "--targets". The
// pseudo-option "--targets" is conceptually more like "--" than an option.
// Traditionally, "--" means "every argument that follows is an operand [i.e.,
// a non-option argument]". Here, "--targets" means "every argument that
// follows is grammatically an operand, and semantically a target. Also, rule
// operands may precede this argument."
//
// If getopt_long returns an instance of "--targets", we store its `optind`
// (see `man 3 getopt`) as the starting index for the list of targets, and we
// store the previous `optind` as the starting index for the list of rules.
//
// in phase 2 of argument parsing, we process the list of rules (if any) and
// the list of targets (if any).
/* [...] */
return ret;
} ...where struct Compdb {
enum class Action {
display_help_and_exit,
emit_all_commands,
emit_userSpecified_commands
};
Action action;
EvaluateCommandMode eval_mode = ECM_NORMAL;
typedef std::vector<StringPiece> ArgList;
ArgList targets;
ArgList rules;
}; ...and the usage looks like this: int NinjaMain::ToolCompilationDatabase(const Options* options, int argc,
char* argv[]) {
Compdb compdb = parseCompdbArguments(argc, argv);
switch (compdb.action) {
/* [...] */
}
return 0;
}
Feedback welcome! (Note: the comment about "storing |
I did originally read the section from Issue 7, but I don't see anything different from Issue 8
I'm not bringing up POSIX-ness or Make functionality to make things harder for us, it should actually make things easier. I understand Ninja does not have POSIX-compliance as a major goal, but I believe it should be done whenever it is easy to do so. POSIX likely recommends against an option having multiple arguments because it's simply not efficient, and doesn't match what most consider to be an "option" compared to what one would define as "input". I personally don't see why we need the secondary stage of imagine instead of
or
we can just have
where the order of operands affects the order of output, but not the actual total content (amount of JSON elements). Then in the manual you don't have to explain a new "option", rather it can simply read: I'm going to try to get that method to work myself, to make sure I'm not preaching for something that isn't possible to begin with... I wanted to have it done already but I've been busy... |
Sry misclicked. Regarding C++ standard: I'm okay with everything that works relativly easy on the oldest supported RHEL version (8 currently). I think it would be best to stick with a command line way that would also work easily with other command line parsers. |
@mcprat , what you propose is ambiguous, and I assure you that someone, somewhere is going to have a rule and a target with the same name, and won´t understand why they cannot use the tool as expected. There needs to be a explicit separation in the parsing format. Or simply introduce a new tool name for targets, nobody will ever get confused. |
Regarding C++, it looks like we should be able to switch to C++17 (to be performed in a separate CL of course). this page says RHEL ships with GCC 8.x or 9.x, and that page says that all C++17 features are supported starting with GCC 8 (and even GCC 7 if you don't care about full support of template deductions). Similarly, this page shows that VS 2019, which we currently use in CI, supports C++17 (unlike VS 2017 which still lacks some features). |
yes, this is a real problem. but options without arguments like if that's not acceptable, then I would vote for separate tools with separate names. |
it turns out that, as soon as |
@mcprat One reason why i prefer having some explicit option (or separate sub-tool, as in as a user reading a shell script, But also, "--targets" makes it clear to ninja that subsequent arguments must be taken as names of targets, and that makes the diagnosis of errors very very simple & straightforward. |
no worries (:
Sorry; i'm not sure i understand what you mean here. What do you mean by "other command line parsers", and how would they be affected by the suggested |
Agreed; Hyrum's Law definitely applies here.
I'm happy to implement @digit-google how should we/i proceed? (Note, |
5e21e2d
to
dae99e6
Compare
the commit in this force-push is obviously very much a work-in-progress, but it seems like people might benefit from experimenting with its proposed command syntax: ninja -t compdb [-hx] [rules] [--targets targets] (note, if you don't use "--targets", then all non-option arguments after At this point, it seems like the main open question is whether we want this ninja -t compdb-targets [-hx] [targets] (the i guess the main question is: does anyone actually have a use-case for merging the commands from a mix of both rules and targets (as implemented in the most recent push)? If not, then maybe it's better to proceed with @digit-google thoughts? |
coding note: since this represents an overhaul of |
update: i was just playing with jq, and it turns out that it can be used to merge & de-duplicate objects from two different json files. So, if you want to have a single ninja -t compdb rule1 rule2 [...] > a.json
ninja -t compdb --targets target1 target2 [...] > b.json
echo '[]' | jq --slurpfile a a.json --slurpfile b b.json '($a[] + $b[]) | unique_by(.output)' > compile_commands.json (the jq function Since this merging-and-deduplication can be done with jq, i propose that this PR should discontinue pursuit of the
@digit-google @jhasse What do you think? |
I continue to be regularly confused every time I look at ninja issues: Are you, @digit-google, a maintainer authority on ninja or are you not? Sometimes you say "I am but a humble contributor", most of the time you use the possessive "our", sometimes you use wording such as "I require you to do X. Note that jhasse is the authority on Y". |
ninja -t compdb-targets
dae99e6
to
660d89b
Compare
todo: - review coding-style guidelines & fix any violations Co-authored-by: Linkun Chen <[email protected]> Co-authored-by: csmoe <[email protected]> Co-authored-by: James Widman <[email protected]>
660d89b
to
1986b5b
Compare
@digit-google note, i moved your line-cooking stuff in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is a good approach, thanks for working on this :)
@@ -53,3 +53,7 @@ div.chapter { | |||
p { | |||
margin-top: 0; | |||
} | |||
|
|||
code.literal { | |||
white-space: nowrap; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
||
static std::string GetWorkingDirectory(); | ||
|
||
std::string working_directory = GetWorkingDirectory(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hm ... why have this here which means it runs at startup although it might not be needed? Am I missing something?
This fixes #1544.
Here we use the same commit as in #2319, plus one more commit so that the option is spelled
--targets
.Many thanks to @lk-chen for the original work in #1546, and to @csmoe for carrying it forward in #2319!
update 2024-10-04:
The most recent push implements the new tool,
ninja -t compdb-targets
.todo:
compdb
)