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

Propagate supbrocess' exit codes to the ninja exit code #2540

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
88 changes: 81 additions & 7 deletions misc/output_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,16 +10,17 @@
import subprocess
import sys
import tempfile
import typing as T
import unittest
from textwrap import dedent
import typing as T
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? Probably some Python formatter?

Copy link
Author

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.

Copy link
Author

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')


Copy link
Collaborator

Choose a reason for hiding this comment

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

this one too

Copy link
Author

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"

def remove_non_visible_lines(raw_output: bytes) -> str:
# When running in a smart terminal, Ninja uses CR (\r) to
# return the cursor to the start of the current line, prints
Expand Down Expand Up @@ -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)
Copy link
Collaborator

Choose a reason for hiding this comment

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

this one too

Copy link
Author

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?

err.cooked_output = remove_non_visible_lines(err.output)
raise err

Expand Down Expand Up @@ -152,14 +153,18 @@ class Output(unittest.TestCase):
'',
))

def _test_expected_error(self, plan: str, flags: T.Optional[str], expected: str):
def _test_expected_error(self, plan: str, flags: T.Optional[str],expected: str,
*args, exit_code: T.Optional[int]=None, **kwargs)->None:
"""Run Ninja with a given plan and flags, and verify its cooked output against an expected content.
All *args and **kwargs are passed to the `run` function
"""
actual = ''
try:
actual = run(plan, flags, print_err_output=False)
except subprocess.CalledProcessError as err:
actual = err.cooked_output
kwargs['print_err_output'] = False
with self.assertRaises(subprocess.CalledProcessError) as cm:
run(plan, flags, *args, **kwargs)
actual = cm.exception.cooked_output
if exit_code is not None:
self.assertEqual(cm.exception.returncode, exit_code)
self.assertEqual(expected, actual)

def test_issue_1418(self) -> None:
Expand Down Expand Up @@ -281,6 +286,75 @@ def test_issue_2048(self) -> None:
except subprocess.CalledProcessError as err:
self.fail("non-zero exit code with: " + err.output)

def test_pr_2540(self)->None:
py = sys.executable
plan = f'''\
rule CUSTOM_COMMAND
command = $COMMAND

build 124: CUSTOM_COMMAND
COMMAND = {py} -c 'exit(124)'

build 127: CUSTOM_COMMAND
COMMAND = {py} -c 'exit(127)'

build 130: CUSTOM_COMMAND
COMMAND = {py} -c 'exit(130)'

build 137: CUSTOM_COMMAND
COMMAND = {py} -c 'exit(137)'

build success: CUSTOM_COMMAND
COMMAND = sleep 0.3; echo success
'''
# Disable colors
env = default_env.copy()
env['TERM'] = 'dumb'
self._test_expected_error(
plan, '124',
f'''[1/1] {py} -c 'exit(124)'
FAILED: [code=124] 124 \n{py} -c 'exit(124)'
ninja: build stopped: subcommand failed.
''',
exit_code=124, env=env,
)
self._test_expected_error(
plan, '127',
f'''[1/1] {py} -c 'exit(127)'
FAILED: [code=127] 127 \n{py} -c 'exit(127)'
ninja: build stopped: subcommand failed.
''',
exit_code=127, env=env,
)
self._test_expected_error(
plan, '130',
'ninja: build stopped: interrupted by user.\n',
exit_code=130, env=env,
)
self._test_expected_error(
plan, '137',
f'''[1/1] {py} -c 'exit(137)'
FAILED: [code=137] 137 \n{py} -c 'exit(137)'
ninja: build stopped: subcommand failed.
''',
exit_code=137, env=env,
)
self._test_expected_error(
plan, 'non-existent-target',
"ninja: error: unknown target 'non-existent-target'\n",
exit_code=1, env=env,
)
self._test_expected_error(
plan, '-j2 success 127',
f'''[1/2] {py} -c 'exit(127)'
FAILED: [code=127] 127 \n{py} -c 'exit(127)'
[2/2] sleep 0.3; echo success
success
ninja: build stopped: subcommand failed.
''',
exit_code=127, env=env,
)

def test_depfile_directory_creation(self) -> None:
b = BuildDir('''\
rule touch
Expand Down
27 changes: 17 additions & 10 deletions src/build.cc
Original file line number Diff line number Diff line change
Expand Up @@ -34,12 +34,12 @@
#include "depfile_parser.h"
#include "deps_log.h"
#include "disk_interface.h"
#include "exit_status.h"
#include "explanations.h"
#include "graph.h"
#include "metrics.h"
#include "state.h"
#include "status.h"
#include "subprocess.h"
#include "util.h"

using namespace std;
Expand Down Expand Up @@ -687,7 +687,7 @@ bool Builder::AlreadyUpToDate() const {
return !plan_.more_to_do();
}

bool Builder::Build(string* err) {
ExitStatus Builder::Build(string* err) {
assert(!AlreadyUpToDate());
plan_.PrepareQueue();

Expand Down Expand Up @@ -726,14 +726,14 @@ bool Builder::Build(string* err) {
if (!StartEdge(edge, err)) {
Cleanup();
status_->BuildFinished();
return false;
return ExitFailure;
}

if (edge->is_phony()) {
if (!plan_.EdgeFinished(edge, Plan::kEdgeSucceeded, err)) {
Cleanup();
status_->BuildFinished();
return false;
return ExitFailure;
}
} else {
++pending_commands;
Expand All @@ -760,14 +760,16 @@ bool Builder::Build(string* err) {
Cleanup();
status_->BuildFinished();
*err = "interrupted by user";
return false;
return result.status;
}

--pending_commands;
if (!FinishCommand(&result, err)) {
bool command_finished = FinishCommand(&result, err);
SetFailureCode(result.status);
if (!command_finished) {
Cleanup();
status_->BuildFinished();
return false;
return result.status;
}

if (!result.success()) {
Expand All @@ -791,11 +793,11 @@ bool Builder::Build(string* err) {
else
*err = "stuck [this is a bug]";

return false;
return GetExitCode();
}

status_->BuildFinished();
return true;
return ExitSuccess;
}

bool Builder::StartEdge(Edge* edge, string* err) {
Expand Down Expand Up @@ -883,7 +885,7 @@ bool Builder::FinishCommand(CommandRunner::Result* result, string* err) {
running_edges_.erase(it);

status_->BuildEdgeFinished(edge, start_time_millis, end_time_millis,
result->success(), result->output);
result->status, result->output);

// The rest of this function only applies to successful commands.
if (!result->success()) {
Expand Down Expand Up @@ -1036,3 +1038,8 @@ bool Builder::LoadDyndeps(Node* node, string* err) {

return true;
}

void Builder::SetFailureCode(ExitStatus code) {
// The ExitSuccess should not overwrite any error
if (code != ExitSuccess) exit_code_ = code;
}
12 changes: 10 additions & 2 deletions src/build.h
Original file line number Diff line number Diff line change
Expand Up @@ -209,9 +209,9 @@ struct Builder {
/// Returns true if the build targets are already up to date.
bool AlreadyUpToDate() const;

/// Run the build. Returns false on error.
/// Run the build. Returns ExitStatus or the exit code of the last failed job.
/// It is an error to call this function when AlreadyUpToDate() is true.
bool Build(std::string* err);
ExitStatus Build(std::string* err);

bool StartEdge(Edge* edge, std::string* err);

Expand All @@ -233,6 +233,10 @@ struct Builder {
std::unique_ptr<CommandRunner> command_runner_;
Status* status_;

/// Returns ExitStatus or the exit code of the last failed job
/// (doesn't need to be an enum value of ExitStatus)
ExitStatus GetExitCode() const { return exit_code_; }

private:
bool ExtractDeps(CommandRunner::Result* result, const std::string& deps_type,
const std::string& deps_prefix,
Expand All @@ -253,6 +257,10 @@ struct Builder {

DependencyScan scan_;

/// Keep the global exit code for the build
ExitStatus exit_code_ = ExitSuccess;
void SetFailureCode(ExitStatus code);

// Unimplemented copy ctor and operator= ensure we don't copy the auto_ptr.
Builder(const Builder &other); // DO NOT IMPLEMENT
void operator=(const Builder &other); // DO NOT IMPLEMENT
Expand Down
Loading
Loading