From 43dde89bba7e125070fd6fc40638a111cd50c956 Mon Sep 17 00:00:00 2001 From: Ihar Hrachyshka Date: Thu, 8 May 2025 22:31:46 -0400 Subject: [PATCH 1/5] Add tails of logs of failed packages to github comments Snippets will be hidden under dropdown sections so as not to clutter UI. Snippets are limited to 20 lines, and if too many logs are generated, the report is truncated so that it fits into a single github comment. Signed-off-by: Ihar Hrachyshka --- README.md | 7 +++++ nixpkgs_review/cli/__init__.py | 5 +++ nixpkgs_review/cli/pr.py | 1 + nixpkgs_review/report.py | 56 ++++++++++++++++++++++++++++++---- nixpkgs_review/review.py | 7 +++-- 5 files changed, 68 insertions(+), 8 deletions(-) diff --git a/README.md b/README.md index 810c7b70..45506e3d 100644 --- a/README.md +++ b/README.md @@ -119,6 +119,13 @@ pass the `--post-result` flag: $ nixpkgs-review pr --post-result 37242 ``` +If you'd like to exclude log snippets for failed builds, add the `--no-logs` +flag: + +```console +$ nixpkgs-review pr --post-result --no-logs 37242 +``` + Instead of posting a PR comment, nixpkgs-review can also print the report to the terminal using the `--print-result` flag. This flag will work for the `rev` and `wip` command.. diff --git a/nixpkgs_review/cli/__init__.py b/nixpkgs_review/cli/__init__.py index a3e9e3e5..33c5b322 100644 --- a/nixpkgs_review/cli/__init__.py +++ b/nixpkgs_review/cli/__init__.py @@ -71,6 +71,11 @@ def pr_flags( action="store_true", help="Do not render the header in the markdown report", ) + pr_parser.add_argument( + "--no-logs", + action="store_true", + help="Do not include build error log snippets in the markdown report", + ) pr_parser.set_defaults(func=pr_command) return pr_parser diff --git a/nixpkgs_review/cli/pr.py b/nixpkgs_review/cli/pr.py index e544d848..480866b8 100644 --- a/nixpkgs_review/cli/pr.py +++ b/nixpkgs_review/cli/pr.py @@ -114,6 +114,7 @@ def pr_command(args: argparse.Namespace) -> str: extra_nixpkgs_config=args.extra_nixpkgs_config, num_parallel_evals=args.num_parallel_evals, show_header=not args.no_headers, + show_logs=not args.no_logs, ) contexts.append( (pr, builddir.path, review.build_pr(pr), review.head_commit) diff --git a/nixpkgs_review/report.py b/nixpkgs_review/report.py index 9399f15f..06701691 100644 --- a/nixpkgs_review/report.py +++ b/nixpkgs_review/report.py @@ -12,11 +12,18 @@ from .nix import Attr from .utils import System, info, link, skipped, system_order_key, to_link, warn +# https://github.com/orgs/community/discussions/27190 +MAX_GITHUB_COMMENT_LENGTH = 65536 + def get_log_filename(a: Attr, system: str) -> str: return f"{a.name}-{system}.log" +def get_log_dir(root: Path) -> Path: + return root / "logs" + + def print_number( logs_dir: Path, system: str, @@ -59,6 +66,28 @@ def html_pkgs_section( return res +def get_file_tail(file: Path, lines: int = 20) -> str: + try: + with file.open("rb") as f: + f.seek(0, os.SEEK_END) + end = f.tell() + f.seek(max(end - lines * 1024, 0), os.SEEK_SET) + return "\n".join( + f.read().decode("utf-8", errors="replace").splitlines()[-lines:] + ) + except OSError: + return "" + + +def html_logs_section(logs_dir: Path, packages: list[Attr], system: str) -> str: + res = "" + for pkg in packages: + tail = get_file_tail(logs_dir / get_log_filename(pkg, system)) + if tail: + res += f"
\n{pkg.name}\n
{tail}
\n
\n" + return res + + class LazyDirectory: def __init__(self, path: Path) -> None: self.path = path @@ -112,7 +141,7 @@ def write_error_logs( *, max_workers: int | None = 1, ) -> None: - logs = LazyDirectory(directory.joinpath("logs")) + logs = LazyDirectory(get_log_dir(directory)) results = LazyDirectory(directory.joinpath("results")) failed_results = LazyDirectory(directory.joinpath("failed_results")) @@ -243,12 +272,14 @@ def __init__( skip_packages: set[str], skip_packages_regex: list[Pattern[str]], show_header: bool = True, + show_logs: bool = False, max_workers: int | None = 1, *, checkout: Literal["merge", "commit"] = "merge", ) -> None: self.commit = commit self.show_header = show_header + self.show_logs = show_logs self.max_workers = max_workers self.attrs = attrs_per_system self.checkout = checkout @@ -274,10 +305,10 @@ def built_packages(self) -> dict[System, list[str]]: } def write(self, directory: Path, pr: int | None) -> None: - directory.joinpath("report.md").write_text(self.markdown(pr)) - directory.joinpath("report.json").write_text(self.json(pr)) - + # write logs first because snippets from them may be needed for the report write_error_logs(self.attrs, directory, max_workers=self.max_workers) + directory.joinpath("report.md").write_text(self.markdown(directory, pr)) + directory.joinpath("report.json").write_text(self.json(pr)) def succeeded(self) -> bool: """Whether the report is considered a success or a failure""" @@ -303,7 +334,7 @@ def json(self, pr: int | None) -> str: indent=4, ) - def markdown(self, pr: int | None) -> str: + def markdown(self, root: Path, pr: int | None) -> str: msg = "" if self.show_header: msg += "## `nixpkgs-review` result\n\n" @@ -350,6 +381,19 @@ def markdown(self, pr: int | None) -> str: ) msg += html_pkgs_section(":white_check_mark:", report.built, "built") + if self.show_logs: + for system, report in self.system_reports.items(): + if not report.failed: + continue + full_msg = msg + full_msg += "\n---\n" + full_msg += f"### Error logs: `{system}`\n" + full_msg += html_logs_section(get_log_dir(root), report.failed, system) + # if the final message won't fit a single github comment, stop + if len(full_msg) > MAX_GITHUB_COMMENT_LENGTH: + break + msg = full_msg + return msg def print_console(self, root: Path, pr: int | None) -> None: @@ -358,7 +402,7 @@ def print_console(self, root: Path, pr: int | None) -> None: info("\nLink to currently reviewing PR:") link(to_link(pr_url, pr_url)) - logs_dir = root / "logs" + logs_dir = get_log_dir(root) for system, report in self.system_reports.items(): info(f"--------- Report for '{system}' ---------") p = functools.partial(print_number, logs_dir, system) diff --git a/nixpkgs_review/review.py b/nixpkgs_review/review.py index fd2b14fe..e8bb9eea 100644 --- a/nixpkgs_review/review.py +++ b/nixpkgs_review/review.py @@ -112,6 +112,7 @@ def __init__( sandbox: bool = False, num_parallel_evals: int = 1, show_header: bool = True, + show_logs: bool = False, ) -> None: if skip_packages_regex is None: skip_packages_regex = [] @@ -149,6 +150,7 @@ def __init__( self.extra_nixpkgs_config = extra_nixpkgs_config self.num_parallel_evals = num_parallel_evals self.show_header = show_header + self.show_logs = show_logs self.head_commit: str | None = None def _process_aliases_for_systems(self, system: str) -> set[str]: @@ -412,6 +414,7 @@ def start_review( skip_packages=self.skip_packages, skip_packages_regex=self.skip_packages_regex, show_header=self.show_header, + show_logs=self.show_logs, # we don't use self.num_parallel_evals here since its choice # is mainly capped by available RAM max_workers=min(32, os.cpu_count() or 1), # 'None' assumes IO tasks @@ -422,7 +425,7 @@ def start_review( success = report.succeeded() if pr and post_result: - self.github_client.comment_issue(pr, report.markdown(pr)) + self.github_client.comment_issue(pr, report.markdown(path, pr)) if pr and approve_pr and success: self.github_client.approve_pr( @@ -431,7 +434,7 @@ def start_review( ) if print_result: - print(report.markdown(pr)) + print(report.markdown(path, pr)) if not self.no_shell: nix_shell( From 5b77541e8746284fee41f67ee8507ddc365ec0d0 Mon Sep 17 00:00:00 2001 From: Ihar Hrachyshka Date: Wed, 14 May 2025 20:48:54 -0400 Subject: [PATCH 2/5] Add a warning about not all logs included if there are too many Signed-off-by: Ihar Hrachyshka --- nixpkgs_review/report.py | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/nixpkgs_review/report.py b/nixpkgs_review/report.py index 06701691..20c3aff9 100644 --- a/nixpkgs_review/report.py +++ b/nixpkgs_review/report.py @@ -382,6 +382,10 @@ def markdown(self, root: Path, pr: int | None) -> str: msg += html_pkgs_section(":white_check_mark:", report.built, "built") if self.show_logs: + truncated_msg = ( + "\n---\n" + "WARNING: Some logs were not included in this report: there were too many." + ) for system, report in self.system_reports.items(): if not report.failed: continue @@ -390,7 +394,8 @@ def markdown(self, root: Path, pr: int | None) -> str: full_msg += f"### Error logs: `{system}`\n" full_msg += html_logs_section(get_log_dir(root), report.failed, system) # if the final message won't fit a single github comment, stop - if len(full_msg) > MAX_GITHUB_COMMENT_LENGTH: + if len(full_msg) > MAX_GITHUB_COMMENT_LENGTH - len(truncated_msg): + msg += truncated_msg break msg = full_msg From 377266d8a1974716a0f55d12a4dd3686f8e38bcd Mon Sep 17 00:00:00 2001 From: Ihar Hrachyshka Date: Wed, 14 May 2025 20:51:48 -0400 Subject: [PATCH 3/5] Clean ANSII escape codes from log snippets, if any present MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Jörg Thalheim Signed-off-by: Ihar Hrachyshka --- nixpkgs_review/report.py | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/nixpkgs_review/report.py b/nixpkgs_review/report.py index 20c3aff9..b6478202 100644 --- a/nixpkgs_review/report.py +++ b/nixpkgs_review/report.py @@ -1,6 +1,7 @@ import functools import json import os +import re import socket import subprocess from collections.abc import Callable @@ -79,10 +80,18 @@ def get_file_tail(file: Path, lines: int = 20) -> str: return "" +def remove_ansi_escape_sequences(text: str) -> str: + """Remove ANSI escape sequences from a string.""" + ansi_escape_pattern = re.compile(r"\x1B(?:[@-Z\\-_]|\[[0-?]*[ -/]*[@-~])") + return ansi_escape_pattern.sub("", text) + + def html_logs_section(logs_dir: Path, packages: list[Attr], system: str) -> str: res = "" for pkg in packages: - tail = get_file_tail(logs_dir / get_log_filename(pkg, system)) + tail = remove_ansi_escape_sequences( + get_file_tail(logs_dir / get_log_filename(pkg, system)) + ) if tail: res += f"
\n{pkg.name}\n
{tail}
\n
\n" return res From e4f8d7c4419b9f75a69cc8460019505ca7db8d6c Mon Sep 17 00:00:00 2001 From: Ihar Hrachyshka Date: Sat, 17 May 2025 19:58:05 -0400 Subject: [PATCH 4/5] Include log section only when some logs will be included Signed-off-by: Ihar Hrachyshka --- nixpkgs_review/report.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/nixpkgs_review/report.py b/nixpkgs_review/report.py index b6478202..60d233b7 100644 --- a/nixpkgs_review/report.py +++ b/nixpkgs_review/report.py @@ -93,6 +93,9 @@ def html_logs_section(logs_dir: Path, packages: list[Attr], system: str) -> str: get_file_tail(logs_dir / get_log_filename(pkg, system)) ) if tail: + if not res: + res = "\n---\n" + res += f"### Error logs: `{system}`\n" res += f"
\n{pkg.name}\n
{tail}
\n
\n" return res @@ -399,8 +402,6 @@ def markdown(self, root: Path, pr: int | None) -> str: if not report.failed: continue full_msg = msg - full_msg += "\n---\n" - full_msg += f"### Error logs: `{system}`\n" full_msg += html_logs_section(get_log_dir(root), report.failed, system) # if the final message won't fit a single github comment, stop if len(full_msg) > MAX_GITHUB_COMMENT_LENGTH - len(truncated_msg): From 08c9ad01390ea4138f2dbc06c2b5ac9e566840ca Mon Sep 17 00:00:00 2001 From: Ihar Hrachyshka Date: Sun, 18 May 2025 16:52:57 -0400 Subject: [PATCH 5/5] Fold report logs per platform too Signed-off-by: Ihar Hrachyshka --- nixpkgs_review/report.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/nixpkgs_review/report.py b/nixpkgs_review/report.py index 60d233b7..f6ab53ad 100644 --- a/nixpkgs_review/report.py +++ b/nixpkgs_review/report.py @@ -95,8 +95,9 @@ def html_logs_section(logs_dir: Path, packages: list[Attr], system: str) -> str: if tail: if not res: res = "\n---\n" - res += f"### Error logs: `{system}`\n" + res += f"
\nError logs: `{system}`\n" res += f"
\n{pkg.name}\n
{tail}
\n
\n" + res += "
\n" return res