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

Make the output of ninja -t inputs deterministic #2075

Merged
merged 1 commit into from
Mar 22, 2022

Conversation

digit-google
Copy link
Contributor

This sorts the output of ninja -t inputs to make it
deterministic and remove duplicates.

@digit-google
Copy link
Contributor Author

This is a followup to the very uesful #1730 which just got merged.

Note that this doesn't change the content of the output, though the documentation states that this tool is supposed to list all inputs necessary for a group of targets, the implementation currently only lists explicit inputs, not implicit and order-only ones (or those stored in .ninja_deps, but that's probably ok). Maybe a different issue though.

@jhasse
Copy link
Collaborator

jhasse commented Feb 5, 2022

LGTM

@naarcini what do you think?

@jhasse jhasse added this to the 1.11.0 milestone Feb 5, 2022
Copy link

@naarcini naarcini left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally LGTM % a couple of suggestions

@digit-google
Copy link
Contributor Author

I have added a unit-test for Edge::CollectInputs. It seems there are no good ways to test tools though. Should we want to add these, should we go with regression tests (e.g. scripts that invoke Ninja with pre-determined build plans, and verify behaviour), or refactor ninja.cc to make tools unit-testable? Probably not in this PR though :-)

@jhasse
Copy link
Collaborator

jhasse commented Feb 8, 2022

Some very basic regression tests are in misc/output_test.py, e.g.:

self.assertEqual(run('', flags='-t recompact'), '')

Making tools unit-testable is probably a good idea though.

@digit-google
Copy link
Contributor Author

Thanks, I could add a regression test to output_test.py for the new tool.

I'm still concerned about the fact that we do not include implicit inputs in the the tool's output. The fact that they are not listed in the command line itself doesn't make them less important. And I don't see a use case where you would want only the explicit inputs anyway. should we add an "explicit-inputs" tool for these otherwise?

@digit-google
Copy link
Contributor Author

Friendly ping. Actually, we have a use case where we need to really get all inputs, including order-only ones to pick up the right set of files from the build directory after an incremental build (i.e. this allows us to skip any stale outputs from a previous build, since cleandead will not work in certain cases).

@naarcini i, can you tell us why you designed the tool in this way? I'm curious about your use case. Otherwise, what would be the best way to configure a tool for multiple types of outputs? I.e. Multiple tool names (e.g. 'explicit-inputs', 'explicit-and-implicit-inputs', 'all-inputs') or additional flags (e.g. -d inputs=all)?

@naarcini
Copy link

@digit-google In my specific use case, we only needed the explicit dependencies. At the time, I was trying to improve upon a project in Chromium that audits certain metadata in code (Network Traffic Annotation Auditor). In that case, all I needed was specifically the code files that are used to build a particular target. Since Chromium's tooling does a pretty good job of explicitly listing all dependencies, I found that "explicit" was enough to fulfill this requirement.

I'm not opposed to adding an additional flag or changing the default behavior to include more information. That'd still work fine for my use case and I suspect it'd work fine for most other peoples' use cases too. If I get a vote, I vote for using an additional flag instead of multiple tool names :)

@digit-google
Copy link
Contributor Author

Thanks, I'll update the code to add a flag plus the proper checks tomorrow :-)

@digit-google
Copy link
Contributor Author

Latest update adds --type=TYPE option to the tool, as in -t inputs --type=TYPE <targets>. The default is to print explicit + implicit inputs, but this can be changed with --type=explicit and --type=all. I think that should address all use cases now :)

@digit-google digit-google force-pushed the deterministic-inputs branch 3 times, most recently from 56ba9a2 to 1f780ae Compare February 14, 2022 13:36
@digit-google
Copy link
Contributor Author

Friendly ping, anything else to get this merged? I would really prefer if the existing behavior was not part of the next 1.11 release :-/

@jhasse
Copy link
Collaborator

jhasse commented Feb 20, 2022

The --type flag needs to be split into its own commit. But before you do that: Is the flag really necessary? I would like to keep the inputs tool simple for the moment.

Also:

(i.e. this allows us to skip any stale outputs from a previous build, since cleandead will not work in certain cases).

Sounds like a bug of the cleandead tool?

@digit-google
Copy link
Contributor Author

Well, I have a use case where I need all implicit + explicit dependencies, and @naarcini only wants the explicit ones, so a --type flag seems the better option here.

This is not really a bug in cleandead, it is more a Ninja design issue: the .ninja_deps file will record all entries generated by depfiles, and sometimes, some of them will be stale (because a command changed and will no longer use that file to the build and the corresponding depfile). Ninja has no way to detect these cases before re-running the build, so cleandead will always keep the stale files in the build directory. This has been causing a variety of breakages in our incremental builds (e.g. https://fxbug.dev/83397 or more recently https://fxbug.dev/85465.

This tool would be one of the different ways we want to work around this problem.

I'll update the PR to make the `--type flag its own commit.

Copy link
Collaborator

@jhasse jhasse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and @naarcini only wants the explicit ones

If I understood him correctly, he wouldn't mind the implicit ones, but doesn't need them?

The extra InputsType struct is quite a lot of code and I only merged the inputs tool because it was such a small change.

src/ninja.cc Outdated
" -t TOOL run a subtool (use '-t list' to list subtools)\n"
" terminates toplevel options; further flags are passed to the tool\n"
" -w FLAG adjust warnings (use '-w list' to list warnings)\n",
kNinjaVersion, config.parallelism);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, because I had changed this text a little bit in a previous version of this pull request, and clang-format hapilly re-indented everything. I later removed the change, but without the reformatting, sorry. This is now fixed, and I have added //clang-format off and //clang-format on directives to avoid that this happens again in the future, unless intended of course.

src/ninja.cc Outdated
#include "clean.h"
#include "debug_flags.h"
#include "depfile_parser.h"
#include "deps_log.h"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any reason for this reordering?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clang-format did it, I swear! :-)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. Can you still move it back? Best to avoid unnecessary noise in the diff.

src/ninja.cc Outdated
#include "clean.h"
#include "debug_flags.h"
#include "depfile_parser.h"
#include "deps_log.h"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clang-format did it, I swear! :-)

src/ninja.cc Outdated
" -t TOOL run a subtool (use '-t list' to list subtools)\n"
" terminates toplevel options; further flags are passed to the tool\n"
" -w FLAG adjust warnings (use '-w list' to list warnings)\n",
kNinjaVersion, config.parallelism);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, because I had changed this text a little bit in a previous version of this pull request, and clang-format hapilly re-indented everything. I later removed the change, but without the reformatting, sorry. This is now fixed, and I have added //clang-format off and //clang-format on directives to avoid that this happens again in the future, unless intended of course.

@digit-google
Copy link
Contributor Author

I pushed a new version with the changes you requested 10 days ago, but forgot to submit my review comments, which I just did. Sorry. Let me know if there is something else to do to get this submitted. Thanks.

@digit-google digit-google force-pushed the deterministic-inputs branch from 40dc0c7 to bde3f2f Compare March 11, 2022 15:22
Copy link
Collaborator

@jhasse jhasse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still think that the tool shouldn't include the --type option and always print all inputs, at least for 1.11.0.

src/ninja.cc Outdated
#include "clean.h"
#include "debug_flags.h"
#include "depfile_parser.h"
#include "deps_log.h"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. Can you still move it back? Best to avoid unnecessary noise in the diff.

@digit-google digit-google force-pushed the deterministic-inputs branch from bde3f2f to 3199119 Compare March 18, 2022 15:56
@digit-google
Copy link
Contributor Author

Done, let me know what you think :)

This sorts the output of `ninja -t inputs` to make it
deterministic and remove duplicates, and adds a regression
test in output_test.py

+ Ensure all inputs are listed, not only explicit ones.
+ Document the `inputs` tool in doc/manual.asciidoc.
@digit-google digit-google force-pushed the deterministic-inputs branch from 3199119 to 65c82b7 Compare March 21, 2022 13:53
@digit-google
Copy link
Contributor Author

I have refactored the two commits so that the first one would print all inputs in a deterministic way (and update the documentation), while the second one adds the --type=TYPE option. I'm just waiting for the checks here to complete, and when everything works, I'll move the second commit to a different PR so that the first one can be part of the 1.11 release as you requested.

@digit-google digit-google force-pushed the deterministic-inputs branch from 65c82b7 to 988c847 Compare March 21, 2022 16:10
@digit-google
Copy link
Contributor Author

Done, the current PR only contains the fix to the output. The --type option was moved to my Github fork as https://github.com/digit-google/ninja/tree/inputs-type which I will rebase on top of upstream when this one is merged.
Let me know if this requires further changes. This now prints all inputs by default.

@jhasse jhasse merged commit 25cdbae into ninja-build:master Mar 22, 2022
@jhasse
Copy link
Collaborator

jhasse commented Mar 22, 2022

Thanks!

@digit-google digit-google deleted the deterministic-inputs branch September 5, 2024 11:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants