-
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
Propagate supbrocess' exit codes to the ninja exit code #2540
base: master
Are you sure you want to change the base?
Conversation
64c6798
to
91bffbe
Compare
The reason why do we need it: In ClickHouse, we use prlimit to identify when some binary becomes too big, but it fails in the entirely same way as a standard failing build. And here how the
|
After the last push, it works like this:
|
The latest commit adds the exit code to the
|
Dear @jhasse and @digit-google, could you please leave a comment on whether it makes sense or if there is something left to improve before the merge? |
Thank you for your contribution. I am very busy this week, but I'll take a look the next one. Sorry about that. |
Thank you very much for you PR. This feature will be useful. I think that keeping a dedicated type for the exit status will help keeping the code easier to read and understand. Besides, native exit codes on Win32 are unsigned long, even though the
Also, on Posix, ensure that an interrupted build returns
Apart from that, can I ask you to rebase your commits to remove temporary fixes? Nobody really needs this information but you, and it makes reviews, and later looking at the git history, difficult. Ideally tests should be introduced with the modifications they are related to. In this specific case, I would recommend one commit to propagate the status (+tests), then another commit to change the printed status line in case of failure to include the code. Finally add a regression test in There is a tiny chance that the status line change is going to break existing parsing scripts. I recommend you try to keep the original |
Hi @digit-google, thanks for the review! I think the only unclear thing to me is how to use ExitStatus for other codes than are defined there. I think, you mean using EXIT_STATUS_TYPE in all the places? Than, looks good to me. I'll apply all the fixes |
Can you clarify what you mean. It should be straightforward to just use the |
Just in case I am not a real C++ dev 👉👈, I could miss the point. |
It's the other way around, lines 1804-1807 of ninja.cc look like this:
What I am suggesting is having
|
but Finish() should as well return the same code it received from the subprocess, so it will be passed from there to -int Subprocess::Finish() {
+ExitStatus Subprocess::Finish() {
assert(pid_ != -1);
int status;
if (waitpid(pid_, &status, 0) < 0)
@@ -166,7 +168,7 @@ int Subprocess::Finish() {
if (WIFEXITED(status)) {
// propagate the status transparently
- return WEXITSTATUS(status);
+ return static_cast<ExitStatus>(WEXITSTATUS(status));
}
if (WIFSIGNALED(status)) {
// Overwrite interrupts to exit code 2
@@ -175,7 +177,7 @@ int Subprocess::Finish() {
return ExitInterrupted;
}
// At this point, we exit with any other signal+128
- return status | 0x80;
+ return static_cast<ExitStatus>(status | 0x80);
} |
24bca44
to
5579fc5
Compare
I think all points are addressed:
Besides, I updated all the tests depending on |
Thanks very much for the latest commits. I added a few review comments, please address them, but this is looking very good so far. Now it's up to @jhasse to add his feedback and approve the PR. |
c8dfe33
to
cc694a7
Compare
Thank you very much, latest version seems perfect to me. |
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.
Thanks for the PR!
From my experience you're entering a world of pain as soon as you want to do anything with exit codes outside of the range 0 to 254. Therefore I'm not sure if Ninja should support it, even on Windows.
I will think about it.
import unittest | ||
from textwrap import dedent | ||
import typing as T |
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.
Why this change? Probably some Python formatter?
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.
It was done by https://github.com/PyCQA/isort/, yes.
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.
Is it ok to keep it sorted? The same question regarding the other pep8-related changes
|
||
default_env = dict(os.environ) | ||
default_env.pop('NINJA_STATUS', None) | ||
default_env.pop('CLICOLOR_FORCE', None) | ||
default_env['TERM'] = '' | ||
NINJA_PATH = os.path.abspath('./ninja') | ||
|
||
|
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.
this one too
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.
This is the pep-8 requirement "two empty lines before and after function/class definitions"
@@ -120,7 +121,7 @@ def run( | |||
cwd=self.d.name, env=env) | |||
except subprocess.CalledProcessError as err: | |||
if print_err_output: | |||
sys.stdout.buffer.write(err.output) | |||
sys.stdout.buffer.write(err.output) |
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.
this one too
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.
Having mixed alignments in the same file confuses very much. Should I revert it?
Addressed most of the questions and made review-related changes as fixups to track the progress. |
Can I kindly get another review round? If the things are addressed, I'd rebase everything into proper commits. |
Make all the methods return ExitStatus: Subprocess::Finish-> CommandRunner::Result.status-> Builder::Build-> NinjaMain::RunBuild Ninja now return different exit codes for Win32 and Posix systems: - Win32 -> 2 - Posix -> 130 (128+2)
I've rebased all the changes, let me know if something here is still blocker to proceed 🙏 |
Now, ninjas could exit with codes 0, 1, and 2.
With these codes, it's nearly impossible to get what went wrong in the subcommands.
This patch propagates the subcommand's exit code to the ninja's exit code.
It fixes #1123, fixes #1507, and technically substitutes the #1805