You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@trafficserver.apache.org by ma...@apache.org on 2026/09/22 11:57:47 UTC
(trafficserver) branch master updated: hrw4u: exit non-zero on compile errors (#13656)
This is an automated email from the ASF dual-hosted git repository.
masaori335 pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/trafficserver.git
The following commit(s) were added to refs/heads/master by this push:
new 915543f9a5 hrw4u: exit non-zero on compile errors (#13656)
915543f9a5 is described below
commit 915543f9a5ceb0b8255413d63682ed9247a60543
Author: Masaori Koshiba <ma...@apache.org>
AuthorDate: Tue Sep 22 20:57:40 2026 +0900
hrw4u: exit non-zero on compile errors (#13656)
* hrw4u: exit non-zero on compile errors
The exit gate required `tree is None`, but ANTLR error recovery almost
always yields a tree, so both syntax and semantic errors exited 0 while
printing diagnostics and a partial .conf. Collecting every error and
failing the build were mutually exclusive: only --stop-on-error exited 1.
Sandbox denials were caught by the same gate, so the "denied" outcome the
sandbox docs describe also exited 0.
generate_output now reports failure by return value and run_main owns the
exit, so a bad file in a bulk run no longer aborts the files after it.
A failing compile still prints its partial .conf; the exit code now marks
it untrustworthy. Suppressing those bytes would change behavior for
existing pipelines and is left as a separate decision.
* hrw4u: parse real input in generate_output return-value tests
The failure test passed parser_obj=None, which only worked because a
None tree short-circuits before the AST branch reads it. Parsing a real
input instead also pins the regression: the input parses, so the tree is
not None -- exactly what the old exit gate let through.
* hrw4u: cover u4wrh in the exit-code tests, scope the doc claim
u4wrh drives the same run_main(), so the contract regresses just as
easily there; verified the new test exits 0 against the pre-fix code.
The doc said every input is processed before the status is decided,
which reads as covering the fatal argument and I/O paths too -- those
still exit immediately.
* hrw4u: document exit status 2 for usage errors
run_main() lets argparse handle the command line, so an unknown option,
a bad option value, or conflicting output modes exit 2, not the 1 the
table claimed. Normalizing them to 1 would merge "you typed the command
wrong" into "your rules did not compile", so the doc follows the code.
Row 1 now lists only what actually exits 1; the mixed bulk/stdout
rejection is caught after parsing and belongs there, not with argparse.
---
doc/admin-guide/configuration/hrw4u.en.rst | 21 +++++++
tools/hrw4u/src/common.py | 24 +++++---
tools/hrw4u/tests/test_cli.py | 91 ++++++++++++++++++++++++++++++
tools/hrw4u/tests/test_common.py | 26 ++++++---
4 files changed, 148 insertions(+), 14 deletions(-)
diff --git a/doc/admin-guide/configuration/hrw4u.en.rst b/doc/admin-guide/configuration/hrw4u.en.rst
index 049454f33c..e387d0f2fb 100644
--- a/doc/admin-guide/configuration/hrw4u.en.rst
+++ b/doc/admin-guide/configuration/hrw4u.en.rst
@@ -115,6 +115,27 @@ This is particularly useful for build systems or when processing many configurat
files at once. All files are processed in a single invocation, improving performance
for large batches of files.
+Exit Status
+^^^^^^^^^^^
+
+====== ==========================================================================
+Status Meaning
+====== ==========================================================================
+0 Every input compiled. Warnings may still have been reported.
+1 At least one input had an error, or the run aborted on a fatal problem:
+ a missing or unreadable input, an unwritable output, or ``input:output``
+ pairs mixed with plain file arguments.
+2 The command line was rejected before any input was read: an unknown
+ option, an invalid option value, or conflicting output modes. This comes
+ from ``argparse`` and follows Python's usage-error convention.
+====== ==========================================================================
+
+A compile error does not stop the run: every input is still processed before
+the status is decided, so one bad file in a multi-file or bulk run does not
+skip the files after it. The fatal problems above still abort immediately. A
+failing compile writes its partial output; the exit status is what marks that
+output untrustworthy.
+
Reverse Tool (u4wrh)
^^^^^^^^^^^^^^^^^^^^
diff --git a/tools/hrw4u/src/common.py b/tools/hrw4u/src/common.py
index 15f1885f4b..22ee24e793 100644
--- a/tools/hrw4u/src/common.py
+++ b/tools/hrw4u/src/common.py
@@ -238,8 +238,12 @@ def generate_output(
filename: str,
args: Any,
error_collector: ErrorCollector | None = None,
- extra_kwargs: dict[str, Any] | None = None) -> None:
- """Generate and print output based on mode with optional error collection."""
+ extra_kwargs: dict[str, Any] | None = None) -> bool:
+ """Generate and print output based on mode with optional error collection.
+
+ Returns True when the input produced errors, so the caller can set the exit
+ status after every input has been processed rather than aborting mid-run.
+ """
if args.ast:
if tree is not None:
print(tree.toStringTree(recog=parser_obj))
@@ -278,8 +282,8 @@ def generate_output(
if error_collector and (error_collector.has_errors() or error_collector.has_warnings()):
print(error_collector.get_error_summary(), file=sys.stderr)
- if error_collector.has_errors() and not args.ast and tree is None:
- sys.exit(1)
+
+ return bool(error_collector and error_collector.has_errors())
def run_main(
@@ -363,10 +367,12 @@ def run_main(
emit_fatal_error(args.error_format, e)
tree, parser_obj, error_collector = create_parse_tree(
content, filename, lexer_class, parser_class, error_prefix, not args.stop_on_error, args.max_errors, args.error_format)
- generate_output(tree, parser_obj, visitor_class, filename, args, error_collector, extra_kwargs)
+ if generate_output(tree, parser_obj, visitor_class, filename, args, error_collector, extra_kwargs):
+ sys.exit(1)
return
if any(':' in f for f in args.files):
+ failed = False
for pair in args.files:
if ':' not in pair:
emit_fatal_message(
@@ -398,12 +404,13 @@ def run_main(
original_stdout = sys.stdout
try:
sys.stdout = output_file
- generate_output(tree, parser_obj, visitor_class, filename, args, error_collector, extra_kwargs)
+ failed |= generate_output(tree, parser_obj, visitor_class, filename, args, error_collector, extra_kwargs)
finally:
sys.stdout = original_stdout
except Exception as e:
emit_fatal_message(args.error_format, f"Error writing to '{output_path}': {e}", filename=output_path)
else:
+ failed = False
for i, input_path in enumerate(args.files):
if i > 0:
print("# ---")
@@ -426,4 +433,7 @@ def run_main(
content, filename, lexer_class, parser_class, error_prefix, not args.stop_on_error, args.max_errors,
args.error_format)
- generate_output(tree, parser_obj, visitor_class, filename, args, error_collector, extra_kwargs)
+ failed |= generate_output(tree, parser_obj, visitor_class, filename, args, error_collector, extra_kwargs)
+
+ if failed:
+ sys.exit(1)
diff --git a/tools/hrw4u/tests/test_cli.py b/tools/hrw4u/tests/test_cli.py
index 886728de05..ddb1ebcd85 100644
--- a/tools/hrw4u/tests/test_cli.py
+++ b/tools/hrw4u/tests/test_cli.py
@@ -48,6 +48,14 @@ def run_hrw4u(args: list[str], stdin: str | None = None) -> subprocess.Completed
return subprocess.run(cmd, capture_output=True, text=True, input=stdin, cwd=Path.cwd())
+def run_u4wrh(args: list[str], stdin: str | None = None) -> subprocess.CompletedProcess:
+ """Run u4wrh script with given arguments."""
+ script = Path("scripts/u4wrh").resolve()
+ cmd = [sys.executable, str(script)] + args
+
+ return subprocess.run(cmd, capture_output=True, text=True, input=stdin, cwd=Path.cwd())
+
+
def test_cli_single_file_to_stdout(sample_hrw4u_files: tuple[Path, Path, Path]) -> None:
"""Test compiling a single file to stdout."""
file1, _, _ = sample_hrw4u_files
@@ -244,3 +252,86 @@ def test_cli_help_lists_error_format_flag() -> None:
assert "--error-format" in result.stdout
for choice in ("plain", "json", "markdown"):
assert choice in result.stdout
+
+
+#
+# Exit-code contract: a compile error must fail the build.
+#
+
+
+def test_cli_exits_nonzero_on_syntax_error(tmp_path: Path) -> None:
+ """A syntax error must exit non-zero even though ANTLR recovers and yields a tree."""
+ bad = tmp_path / "bad.hrw4u"
+ bad.write_text("REMAP {\n inbound.req.X-Foo = \n}\n")
+
+ result = run_hrw4u([str(bad)])
+
+ assert result.returncode != 0
+ assert ": error:" in result.stderr
+
+
+def test_cli_exits_nonzero_on_semantic_error(tmp_path: Path) -> None:
+ """A semantic error must exit non-zero; the parse tree exists, so only sema catches it."""
+ bad = tmp_path / "bad.hrw4u"
+ bad.write_text("REMAP {\n test::add-debug-header(\"foo\");\n}\n")
+
+ result = run_hrw4u([str(bad)])
+
+ assert result.returncode != 0
+ assert "unknown procedure" in result.stderr
+
+
+def test_cli_collects_all_errors_and_still_exits_nonzero(tmp_path: Path) -> None:
+ """Multi-error mode must report every diagnostic AND fail; the two are not exclusive."""
+ bad = tmp_path / "bad.hrw4u"
+ bad.write_text("REMAP {\n bogus.one = \"a\";\n bogus.two = \"b\";\n}\n")
+
+ result = run_hrw4u([str(bad)])
+
+ assert result.returncode != 0
+ assert result.stderr.count(": error:") >= 2
+
+
+def test_cli_multi_file_exits_nonzero_if_any_fails(sample_hrw4u_files: tuple[Path, Path, Path], tmp_path: Path) -> None:
+ """One bad file among good ones fails the run, but the good ones are still processed."""
+ good, _, _ = sample_hrw4u_files
+ bad = tmp_path / "bad.hrw4u"
+ bad.write_text("REMAP {\n test::nope(\"x\");\n}\n")
+
+ result = run_hrw4u([str(bad), str(good)])
+
+ assert result.returncode != 0
+ assert "no-op" in result.stdout, "processing must continue past the failing file"
+
+
+def test_cli_u4wrh_exits_nonzero_on_error(tmp_path: Path) -> None:
+ """u4wrh shares run_main(), so it must honor the same exit-status contract."""
+ bad = tmp_path / "bad.conf"
+ bad.write_text("cond %{READ_REQUEST_HDR_HOOK}\n set-header X-Foo\n")
+
+ result = run_u4wrh([str(bad)])
+
+ assert result.returncode != 0
+ assert ": error:" in result.stderr
+
+
+@pytest.mark.parametrize(
+ "argv", [["--bogus"], ["--ast", "--hrw"], ["--error-format", "nope"], ["--max-errors", "x"]],
+ ids=["unknown-option", "conflicting-output-modes", "invalid-choice", "invalid-int"])
+def test_cli_usage_error_exits_two(argv: list[str]) -> None:
+ """Usage errors come from argparse and exit 2, not 1; the documented status must hold."""
+ result = run_hrw4u(argv, stdin="")
+
+ assert result.returncode == 2
+ assert "usage:" in result.stderr
+
+
+def test_cli_mixed_file_formats_exits_one(tmp_path: Path) -> None:
+ """Mixed bulk/stdout arguments are rejected by run_main() itself, so they exit 1, not 2."""
+ good = tmp_path / "good.hrw4u"
+ good.write_text("REMAP {\n no-op();\n}\n")
+
+ result = run_hrw4u([f"{good}:{tmp_path / 'out.conf'}", str(good)])
+
+ assert result.returncode == 1
+ assert "Mixed formats not allowed" in result.stderr
diff --git a/tools/hrw4u/tests/test_common.py b/tools/hrw4u/tests/test_common.py
index d17cdf6ad5..1997f28cbe 100644
--- a/tools/hrw4u/tests/test_common.py
+++ b/tools/hrw4u/tests/test_common.py
@@ -164,14 +164,26 @@ class TestGenerateOutput:
out = capsys.readouterr().out
assert "Parse tree not available" in out
- def test_error_collector_exits_on_parse_failure(self, capsys):
- """When tree is None and errors exist in non-AST mode, should exit(1)."""
- errors = ErrorCollector()
- errors.add_error(Hrw4uSyntaxError("<test>", 1, 0, "parse failed", "bad"))
+ def test_error_collector_reports_failure_to_caller(self):
+ """generate_output reports errors via its return value; run_main owns the exit status.
+
+ The input parses, so ``tree`` is not None -- the exact shape the old
+ ``tree is None`` exit gate let through.
+ """
+ tree, parser_obj, errors = create_parse_tree(
+ 'REMAP { test::nope("x"); }', "<test>", hrw4uLexer, hrw4uParser, "hrw4u", collect_errors=True)
args = SimpleNamespace(ast=False, debug=False, no_comments=False)
- with pytest.raises(SystemExit) as exc_info:
- generate_output(None, None, HRW4UVisitor, "<test>", args, errors)
- assert exc_info.value.code == 1
+
+ assert tree is not None
+ assert generate_output(tree, parser_obj, HRW4UVisitor, "<test>", args, errors) is True
+
+ def test_clean_input_reports_no_failure(self):
+ """A clean parse must report False so a multi-file run keeps exit status 0."""
+ tree, parser_obj, errors = create_parse_tree(
+ 'REMAP { no-op(); }', "<test>", hrw4uLexer, hrw4uParser, "hrw4u", collect_errors=True)
+ args = SimpleNamespace(ast=False, debug=False, no_comments=False)
+
+ assert generate_output(tree, parser_obj, HRW4UVisitor, "<test>", args, errors) is False
def test_visitor_exception_collected(self, capsys):
"""When visitor.visit() raises, error is collected and reported."""