diff --git a/.github/workflows/build-and-test-ci.yml b/.github/workflows/build-and-test-ci.yml index 71b5400ba2..0d32a01cf4 100644 --- a/.github/workflows/build-and-test-ci.yml +++ b/.github/workflows/build-and-test-ci.yml @@ -308,7 +308,7 @@ jobs: runs-on: ubuntu-latest timeout-minutes: 120 container: - image: ghcr.io/edgcpp/ci-runner@sha256:565d7b6581912008ca91c5dbc940280c6e28a02c8cb6487d88ee5f0d7e8c51d7 + image: ghcr.io/edgcpp/ci-runner@sha256:daae67adf04085b9ae54a816b35053b79086070434d2da4aafd6662f6ecb0bf7 defaults: run: shell: edg-container-exec "bash -eo pipefail {0}" diff --git a/.github/workflows/ci-toolchain.yml b/.github/workflows/ci-toolchain.yml index 87cfdb7cd8..9d92a72a52 100644 --- a/.github/workflows/ci-toolchain.yml +++ b/.github/workflows/ci-toolchain.yml @@ -66,7 +66,7 @@ jobs: runs-on: ubuntu-latest timeout-minutes: 120 container: - image: &ci-runner-image ghcr.io/edgcpp/ci-runner@sha256:565d7b6581912008ca91c5dbc940280c6e28a02c8cb6487d88ee5f0d7e8c51d7 + image: &ci-runner-image ghcr.io/edgcpp/ci-runner@sha256:daae67adf04085b9ae54a816b35053b79086070434d2da4aafd6662f6ecb0bf7 defaults: &container-defaults run: shell: edg-container-exec "bash -eo pipefail {0}" @@ -267,6 +267,17 @@ jobs: edg-prefix-env-loader .tmp/extensive-ci.edgivk + # Reviews (acknowledg-forge, edgy-review) read only the .rt.tar.gz + # files changes.elist names; edgy -A archives every test's. Pruning + # must never keep the results from being uploaded, hence + # continue-on-error. + - name: Prune .rt.tar.gz results changes.elist doesn't name + if: ${{ failure() }} + continue-on-error: true + env: + RUN_TIMESTAMP: ${{ inputs.run_timestamp }} + run: edg-prune-test-run "tests/runs/${RUN_TIMESTAMP}" + - name: Upload partial elist, elog, and .rt.tar.gz results if: ${{ failure() }} uses: actions/upload-artifact@v7 diff --git a/.github/workflows/promote-test-cache.yml b/.github/workflows/promote-test-cache.yml index 85f0784ee4..29bb26eea8 100644 --- a/.github/workflows/promote-test-cache.yml +++ b/.github/workflows/promote-test-cache.yml @@ -72,7 +72,7 @@ jobs: # The test legs' image, so that actions/cache picks the same compression # (part of an entry's version) and the test legs can restore the entry. container: - image: ghcr.io/edgcpp/ci-runner@sha256:565d7b6581912008ca91c5dbc940280c6e28a02c8cb6487d88ee5f0d7e8c51d7 + image: ghcr.io/edgcpp/ci-runner@sha256:daae67adf04085b9ae54a816b35053b79086070434d2da4aafd6662f6ecb0bf7 strategy: fail-fast: false matrix: diff --git a/.github/workflows/strict-build-check.yml b/.github/workflows/strict-build-check.yml index ebdb1f94ff..c74d38f5f1 100644 --- a/.github/workflows/strict-build-check.yml +++ b/.github/workflows/strict-build-check.yml @@ -158,7 +158,7 @@ jobs: runs-on: ubuntu-latest timeout-minutes: 120 container: - image: ghcr.io/edgcpp/ci-runner@sha256:565d7b6581912008ca91c5dbc940280c6e28a02c8cb6487d88ee5f0d7e8c51d7 + image: ghcr.io/edgcpp/ci-runner@sha256:daae67adf04085b9ae54a816b35053b79086070434d2da4aafd6662f6ecb0bf7 defaults: run: shell: edg-container-exec "bash -eo pipefail {0}" @@ -185,7 +185,7 @@ jobs: runs-on: ubuntu-latest timeout-minutes: 120 container: - image: ghcr.io/edgcpp/ci-runner@sha256:565d7b6581912008ca91c5dbc940280c6e28a02c8cb6487d88ee5f0d7e8c51d7 + image: ghcr.io/edgcpp/ci-runner@sha256:daae67adf04085b9ae54a816b35053b79086070434d2da4aafd6662f6ecb0bf7 defaults: run: shell: edg-container-exec "bash -eo pipefail {0}" @@ -212,7 +212,7 @@ jobs: runs-on: ubuntu-latest timeout-minutes: 120 container: - image: ghcr.io/edgcpp/ci-runner@sha256:565d7b6581912008ca91c5dbc940280c6e28a02c8cb6487d88ee5f0d7e8c51d7 + image: ghcr.io/edgcpp/ci-runner@sha256:daae67adf04085b9ae54a816b35053b79086070434d2da4aafd6662f6ecb0bf7 defaults: run: shell: edg-container-exec "bash -eo pipefail {0}" @@ -239,7 +239,7 @@ jobs: runs-on: ubuntu-latest timeout-minutes: 120 container: - image: ghcr.io/edgcpp/ci-runner@sha256:565d7b6581912008ca91c5dbc940280c6e28a02c8cb6487d88ee5f0d7e8c51d7 + image: ghcr.io/edgcpp/ci-runner@sha256:daae67adf04085b9ae54a816b35053b79086070434d2da4aafd6662f6ecb0bf7 defaults: run: shell: edg-container-exec "bash -eo pipefail {0}" diff --git a/dev_tools/bin/edg-prune-test-run b/dev_tools/bin/edg-prune-test-run new file mode 100755 index 0000000000..451f49fc5a --- /dev/null +++ b/dev_tools/bin/edg-prune-test-run @@ -0,0 +1,58 @@ +#!/usr/bin/env python3 + +# Part of the EDG Compiler Project, under the Apache License v2.0 with LLVM +# Exceptions. +# See https://edgcpp.org/LICENSE.txt for license information. +# SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception + +'''Prune a test run down to what a review of it reads, e.g., before CI +uploads the run: + + edg-prune-test-run tests/runs/ + +Reviews of a run read only the .rt.tar files its changes.elist files name. +The replays directory is removed whole. In each configuration, a suite +with no test named there is removed whole (its directory, and its +suite- logs), as is every suite of a configuration without a +changes.elist; of the other suites, the .rt.tar files not named there are +removed. +''' + +import argparse +import sys + +import edgtest +import edgutil + +from pathlib import Path + +def _parse_args() -> argparse.Namespace: + parser = argparse.ArgumentParser( + description = ( + "Remove the .rt.tar files of a test run that no configuration's " + 'changes.elist names.' + ) + ) + parser.add_argument( + 'run_dir', + type = Path, + help = 'the run directory (e.g., tests/runs/)' + ) + return parser.parse_args() + +def main() -> int: + args = _parse_args() + if not args.run_dir.is_dir(): + edgutil.eprint(f"error: {args.run_dir} is not a directory") + return 1 + + # Counts: of .rt.tar files kept and removed, and of suites removed. + kept, removed, suites = edgtest.prune_run_archives(args.run_dir) + print( + f"Kept {kept} .rt.tar files named by changes.elist files; removed " + f"{removed} others, and {suites} suites with no changes." + ) + return 0 + +if __name__ == '__main__': + sys.exit(main()) diff --git a/dev_tools/bin/edg-rematerialize-test-run b/dev_tools/bin/edg-rematerialize-test-run index 4f2c99b7f1..d4c3948613 100755 --- a/dev_tools/bin/edg-rematerialize-test-run +++ b/dev_tools/bin/edg-rematerialize-test-run @@ -73,8 +73,14 @@ def build_diff_files(diff_file: Path, curr_diff_file_descr = None diff_file_to_handle = {} - for line in diff_file_handle: - line_match = GIT_DIFF_REGEX.match(line) + # Lines are read in bounded pieces so that an enormous one is never held + # whole; only a piece that starts a line can be a header, and every piece + # is written through unchanged. + for line, at_line_start in edgutil.iter_line_pieces( + diff_file_handle, + edgutil.MAX_LINE_CHARS + ): + line_match = GIT_DIFF_REGEX.match(line) if at_line_start else None if line_match is not None: if curr_diff_file_descr is not None: # Close the file handle for the previous diff file (if any) to @@ -180,8 +186,15 @@ def collect_affected_tests(test_home: Path, suite_name = file_path.stem # An expectations update. with open(diff_file.diff_file) as file_handle: - for line in file_handle: - if line.startswith('---') or line.startswith('+++'): + # edgutil.MAX_LINE_CHARS is far more than any genuine expectation + # entry needs, so a longer line is refused (as when the file is + # read), without being held whole. + try: + for line in edgutil.iter_limited_lines( + file_handle, + edgutil.MAX_LINE_CHARS + ): + if line.startswith('---') or line.startswith('+++'): # Do not take in lines like: # # --- a/tests/expectations/edg_x86_64_cp/modules.log @@ -189,13 +202,19 @@ def collect_affected_tests(test_home: Path, # continue - if line.startswith('-') or line.startswith('+'): - # Do a naive parse to get the test file name. - test_file_rel_path = PurePosixPath(line[1:].split(':')[0]) - result.tests.add( - tests_dir.joinpath(suite_name, test_file_rel_path) - ) - result.configs.add(config_name) + if line.startswith('-') or line.startswith('+'): + # Do a naive parse to get the test file name. + test_file_rel_path = PurePosixPath(line[1:].split(':')[0]) + result.tests.add( + tests_dir.joinpath(suite_name, test_file_rel_path) + ) + result.configs.add(config_name) + except edgutil.LineTooLongError: + eprint( + f"{diff_file.file_name}: a changed line is longer than " + f"{edgutil.MAX_LINE_CHARS} characters" + ) + sys.exit(1) return result @@ -548,10 +567,20 @@ async def simulate_test(test_home: Path, config_run_dir: Path, return test_status_lines def read_suite_expectation_lines(suite_file: Path) -> List[str]: + '''The lines of ``suite_file`` (none if there is no such file); exits if + one is longer than ``edgutil.MAX_LINE_CHARS``, rather than holding it. + ''' if not suite_file.exists(): return [] - with open(suite_file, 'r') as file_handle: - return file_handle.readlines() + try: + with open(suite_file, 'r') as file_handle: + return list(edgutil.iter_limited_lines( + file_handle, + edgutil.MAX_LINE_CHARS + )) + except edgutil.LineTooLongError as ex: + eprint(f"{suite_file}: {ex}") + sys.exit(1) def load_test_suite_expectations(all_expectations: Dict[str, List[str]], suite_name: str, expectations_dir: Path, diff --git a/dev_tools/pylibs/edgacknowledg/__init__.py b/dev_tools/pylibs/edgacknowledg/__init__.py index 9b21f50672..6ee7b8bdbe 100644 --- a/dev_tools/pylibs/edgacknowledg/__init__.py +++ b/dev_tools/pylibs/edgacknowledg/__init__.py @@ -121,12 +121,17 @@ def request_tags_with_prefix(prefix: str) -> FrozenSet[str]: ALLOWED_BENCH_EDIT_TAGS ) -PROTOCOL_REVISION = 20 +PROTOCOL_REVISION = 21 # 10MiB MAX_MESSAGE_SIZE = 10485760 # 8MiB MAX_DISPLAY_CONTENT_SIZE = 8388608 +# 16Ki characters: a longer line is cut for display (see lines_truncated). +MAX_DISPLAY_LINE_SIZE = 16384 +# 1Ki characters: a longer label (e.g. a status line's variant, at most a few +# hundred in practice) is cut for display, ending in an ellipsis. +MAX_DISPLAY_LABEL_SIZE = 1024 # Most lines shown on each side of the requested line by a ranged # ``test.src.show-src`` request. diff --git a/dev_tools/pylibs/edgacknowledg/budget.py b/dev_tools/pylibs/edgacknowledg/budget.py new file mode 100644 index 0000000000..11a70f1750 --- /dev/null +++ b/dev_tools/pylibs/edgacknowledg/budget.py @@ -0,0 +1,27 @@ +# Part of the EDG Compiler Project, under the Apache License v2.0 with LLVM +# Exceptions. +# See https://edgcpp.org/LICENSE.txt for license information. +# SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception + +'''A size budget shared by readers that count bytes or characters.''' + + +class BudgetExceeded(Exception): + '''Raised when an operation would spend more than its budget allows.''' + + +class Budget: + def __init__(self, limit: int) -> None: + if limit < 0: + raise ValueError('budget limit must be nonnegative') + self.limit = limit + self.remaining = limit + self.exceeded = False + + def spend(self, count: int) -> None: + if count < 0: + raise ValueError('budget count must be nonnegative') + if count > self.remaining: + self.exceeded = True + raise BudgetExceeded() + self.remaining -= count diff --git a/dev_tools/pylibs/edgacknowledg/client/shared.py b/dev_tools/pylibs/edgacknowledg/client/shared.py index 9d47963490..7779fbe95a 100644 --- a/dev_tools/pylibs/edgacknowledg/client/shared.py +++ b/dev_tools/pylibs/edgacknowledg/client/shared.py @@ -7,6 +7,7 @@ import asyncio import errno +import io import os import signal import stat @@ -39,12 +40,15 @@ from edgacknowledg import ( MAX_DISPLAY_CONTENT_SIZE, MAX_SOURCE_RANGE_CONTEXT, + MAX_DISPLAY_LABEL_SIZE, + MAX_DISPLAY_LINE_SIZE, ProtocolMessage, RequestTags, RichName, decode_response, message, ) +from edgacknowledg.budget import Budget, BudgetExceeded if Protocol is not None: @@ -85,10 +89,14 @@ async def handle_request(self, channel: Channel) -> None: ReviewClientHandler = Any # type: ignore[misc,assignment] +import edgutil + from edgutil import get_running_event_loop, is_path_relative_to CoroFactory = Callable[[], Awaitable[Any]] +_MAX_SOURCE_READ_CHARS = 16 * 1024 * 1024 + class BackgroundWorkerPool: '''Async worker pool used to regulate parallel background work.''' @@ -156,51 +164,161 @@ def source_range_window(line: int, context: int) -> Tuple[int, int]: context = max(0, min(context, MAX_SOURCE_RANGE_CONTEXT)) return max(1, line - context), max(1, line) + context -def slice_source_range(lines: Iterable[str], line: int, - context: int) -> Dict[str, Union[str, int]]: - '''Reply fields for the window around ``line`` of ``lines``. - ``lines`` may be a lazy iterable (such as an open file). It is consumed - in full to count the lines, but only the window is kept in memory. +def slice_source_range(handle: IO[str], line: int, + context: int) -> Dict[str, Any]: + '''Reply fields for the window around ``line`` of the text ``handle``, + with ``lines_truncated`` as ``display_content`` sets it (numbered within + the window), and ``truncated`` if the read budget is exhausted. + + ``handle`` is read no further than the window's last line, and only the + window is kept in memory, of each line at most ``MAX_DISPLAY_LINE_SIZE`` + characters. ''' first, last = source_range_window(line, context) shown: List[str] = [] - total = 0 - for total, text in enumerate(lines, start = 1): - if first <= total <= last: - shown.append(text.rstrip('\n')) - return { + lines_truncated: List[int] = [] + budget = Budget(_MAX_SOURCE_READ_CHARS) + for line_number, (text, _, cut) in enumerate( + _display_lines( + edgutil.iter_line_pieces(handle, MAX_DISPLAY_LINE_SIZE + 1), + budget + ), + start = 1 + ): + if line_number >= first: + shown.append(text) + if cut: + lines_truncated.append(len(shown)) + if line_number == last: + break + result: Dict[str, Any] = { 'content': '\n'.join(shown), - 'first_line': min(first, max(total, 1)), - 'total_lines': total + 'first_line': first } + if lines_truncated: + result['lines_truncated'] = lines_truncated + if budget.exceeded: + result['truncated'] = True + return result + +def trunc_display_label(text: str) -> str: + '''``text`` (one line, e.g. a status) cut for UI display to + ``MAX_DISPLAY_LABEL_SIZE`` characters, a cut ending in an ellipsis. + ''' + if len(text) > MAX_DISPLAY_LABEL_SIZE: + text = text[:MAX_DISPLAY_LABEL_SIZE - 1] + '\N{HORIZONTAL ELLIPSIS}' + return text -def trunc_display_content(content: str) -> Dict[str, Union[str, bool]]: - '''Truncate ``content`` for UI display; may set ``truncated``.''' - max_content_size = MAX_DISPLAY_CONTENT_SIZE - if len(content) > max_content_size: - # Attempt to truncate at line boundaries. - # - # To save time, find the newline closest to the boundary before walking - # the file size back. - last_newline_permitted = content.find('\n', max_content_size) - if last_newline_permitted != -1: - content = content[:last_newline_permitted] - # Now walk the file size back. - while len(content) > max_content_size: - last_new_line_idx = content.rfind('\n') - if last_new_line_idx != -1: - content = content[:(last_new_line_idx - 1)] - else: - # If there are no more newlines, resort to truncating within - # the line. - content = content[:(max_content_size - 4)] - content += '...' - return { - 'truncated': True, - 'content': content - } - return { 'content': content } + +def _display_lines(pieces: Iterable[Tuple[str, bool]], + budget: Budget) -> Iterator[Tuple[str, str, bool]]: + '''Join ``(piece, at_line_start)`` pieces (as ``edgutil.iter_line_pieces`` + yields them) into lines, yielding ``(text, line_end, cut)`` for each: at + most ``MAX_DISPLAY_LINE_SIZE`` characters of its text (``cut`` if it had + more, which are dropped as they come), and its end ('\\n', or '' for a + last line without one). Stop once ``budget`` is exhausted. + + A piece that starts a line ends the one before it, so a text that doesn't + end in a line end (e.g. a command's output) never runs into the next. + ''' + # The line being joined (as much of it as is kept), and whether some was + # dropped; ``None`` between lines. + line: Optional[str] = None + cut = False + for piece, at_line_start in pieces: + try: + budget.spend(len(piece)) + except BudgetExceeded: + if not budget.remaining: + break + piece = piece[:budget.remaining] + budget.spend(len(piece)) + if line is not None and at_line_start: + yield line, '\n', cut + line = None + if line is None: + line = '' + cut = False + text = piece[:-1] if piece.endswith('\n') else piece + room = MAX_DISPLAY_LINE_SIZE - len(line) + if len(text) > room: + cut = True + line += text[:room] + if piece.endswith('\n'): + yield line, '\n', cut + line = None + if budget.exceeded: + break + if line is not None: + yield line, '', cut + + +def display_content(pieces: Iterable[Tuple[str, bool]]) -> Dict[str, Any]: + '''The ``content`` of a display payload, from ``(piece, at_line_start)`` + pieces (as ``edgutil.iter_line_pieces`` yields them), and as needed + ``truncated`` and ``lines_truncated``: + + - Each line is cut to ``MAX_DISPLAY_LINE_SIZE`` characters, its 1-based + number listed in ``lines_truncated``. + - The whole is cut to ``MAX_DISPLAY_CONTENT_SIZE``, after the last line + that fits (setting ``truncated``); no more pieces are taken then, so + what they come from is read no further. (Closing ``pieces``, should it + be a generator with files open, is left to the caller.) + Reading also stops at ``_MAX_SOURCE_READ_CHARS`` and sets ``truncated``. + + Neither key is set when nothing was cut, and nothing marks a cut in the + content itself: the UI does. + ''' + kept: List[str] = [] + size = 0 + lines_truncated: List[int] = [] + truncated = False + budget = Budget(_MAX_SOURCE_READ_CHARS) + for line_number, (text, line_end, cut) in enumerate( + _display_lines(pieces, budget), + start = 1 + ): + num_chars_for_line = len(text) + len(line_end) + if size + num_chars_for_line > MAX_DISPLAY_CONTENT_SIZE: + truncated = True + if not kept: + # The first line alone doesn't fit (MAX_DISPLAY_LINE_SIZE is + # larger than MAX_DISPLAY_CONTENT_SIZE): keep what does of it, as + # a cut line. + kept.append(text[:MAX_DISPLAY_CONTENT_SIZE]) + lines_truncated.append(line_number) + break + kept.append(text + line_end) + size += num_chars_for_line + if cut: + lines_truncated.append(line_number) + + truncated = truncated or budget.exceeded + content = ''.join(kept) + if truncated and content.endswith('\n'): + # Without the end of the last line kept. + content = content[:-1] + result: Dict[str, Any] = { 'content': content } + if truncated: + result['truncated'] = True + if lines_truncated: + result['lines_truncated'] = lines_truncated + return result + + +def display_text(handle: IO[str]) -> Dict[str, Any]: + '''``display_content`` of the text ``handle`` reads: at most the display + budget plus one read piece, even if a line is longer. + ''' + return display_content( + edgutil.iter_line_pieces(handle, MAX_DISPLAY_LINE_SIZE + 1) + ) + + +def trunc_display_content(content: str) -> Dict[str, Any]: + '''``display_content`` of ``content``, already in memory.''' + return display_text(io.StringIO(content)) def reviewer_of(request: Any) -> Optional[str]: diff --git a/dev_tools/pylibs/edgacknowledg/client/test/__init__.py b/dev_tools/pylibs/edgacknowledg/client/test/__init__.py index caacc92693..e7f17e068d 100644 --- a/dev_tools/pylibs/edgacknowledg/client/test/__init__.py +++ b/dev_tools/pylibs/edgacknowledg/client/test/__init__.py @@ -6,6 +6,10 @@ '''AcknowlEDG test-review provider package.''' import asyncio +import contextlib +import filecmp +import functools +import io import os import re import shutil @@ -27,7 +31,7 @@ ) from edgacknowledg import ( - MAX_DISPLAY_CONTENT_SIZE, + MAX_DISPLAY_LINE_SIZE, ProtocolMessage, RequestTags, WebSocketsMessageType, @@ -40,9 +44,12 @@ ConfinedPath, IllegalFileAccessError, SandboxContext, + display_content, + display_text, reviewer_of, slice_source_range, trunc_display_content, + trunc_display_label, ) from edgutil import eprint @@ -71,7 +78,13 @@ def _create_status_dict(elog_file_paths: List[ConfinedPath], *, for elog_file_path in elog_file_paths: config_name = elog_file_path.parent.name with elog_file_path.open() as file_handle: - for line in file_handle: + # edgutil.MAX_LINE_CHARS is far more than any genuine status line + # needs, so a longer line is refused (failing the request, as a line + # that isn't a status line does), without being held whole. + for line in edgutil.iter_limited_lines( + file_handle, + edgutil.MAX_LINE_CHARS + ): key, parsed_diff = ( edgtest.runtest.parse_test_status_diff(line) ) @@ -80,7 +93,7 @@ def _create_status_dict(elog_file_paths: List[ConfinedPath], *, pending[key] = {} lines_by_test_config[key] = {} if parsed_diff.remark: - remarks[key] = parsed_diff.remark + remarks[key] = trunc_display_label(parsed_diff.remark) if config_name not in pending[key]: rt_file_path = _rt_path_for(elog_file_path.parent, key) @@ -90,9 +103,9 @@ def _create_status_dict(elog_file_paths: List[ConfinedPath], *, lines_by_test_config[key][config_name] = [] lines_by_test_config[key][config_name].append({ - "qualifier": parsed_diff.qualifier, - "variant": parsed_diff.variant, - "status": parsed_diff.status.full_status + "qualifier": trunc_display_label(parsed_diff.qualifier), + "variant": trunc_display_label(parsed_diff.variant), + "status": trunc_display_label(parsed_diff.status.full_status) }) keyed_statuses: _StatusDict = {} @@ -287,13 +300,17 @@ def test_id_to_path(test_id: str) -> ConfinedPath: )) return - async def form_curr_diff_text(config_name: str, - case_rt_path: ConfinedPath) -> str: - '''Live-diff observed vs recordings per command. - - Per-command body content is diffed (recording command line stripped), - then equal bodies are collapsed with # command comments — same - presentation as static. + async def write_curr_diff(config_name: str, + case_rt_path: ConfinedPath, + out_path: Path) -> None: + '''Write the live diff of observed output vs recordings to + ``out_path``. + + Per-command body content is diffed (recording command line + stripped), then equal bodies are collapsed with # command + comments, the same presentation as static. The per-command diffs + are files of its own, gone once ``out_path`` is written; none is + held in memory. ''' suite_name = test_id.split('/')[0] suite_path = test_home / 'tests' / suite_name @@ -312,8 +329,7 @@ async def form_curr_diff_text(config_name: str, with tempfile.TemporaryDirectory() as tmp_dir_name: tmp_dir = Path(tmp_dir_name) - entries: List[Tuple[Dict[str, Any], List[str]]] = [] - + entries: List[Tuple[Dict[str, Any], Path]] = [] with edgtest.runtest.RunTestOutput(case_rt_path.path) as rt_output: for case_number, case in enumerate( rt_output.metadata.get('cases', []), @@ -356,13 +372,9 @@ async def form_curr_diff_text(config_name: str, if diff_hash is None: continue - with open(change_diff_path, 'r', - errors = 'replace') as file_handle: - diff_lines = file_handle.readlines() - command_entry = dict(command) command_entry['diff_hash'] = diff_hash - entries.append((command_entry, diff_lines)) + entries.append((command_entry, change_diff_path)) if case.get('matches') is None: continue @@ -396,57 +408,82 @@ async def form_curr_diff_text(config_name: str, if matches_diff_hash is None: continue - with open(matches_change_diff, 'r', - errors = 'replace') as file_handle: - matches_diff_lines = file_handle.readlines() - matches_entry = { 'command': edgtest.runtest.case_matches_annotation( case_number ), 'diff_hash': matches_diff_hash } - entries.append((matches_entry, matches_diff_lines)) - - return ''.join( - edgtest.runtest.format_collapsed_case_diffs(entries) - ) + entries.append((matches_entry, matches_change_diff)) + + with open(out_path, 'w', encoding = 'utf-8') as out_handle: + with contextlib.closing( + edgtest.runtest.iter_collapsed_case_diff_pieces( + [ + (entry, + functools.partial(open, path, 'r', errors = 'replace')) + for entry, path in entries + ], + edgutil.MAX_LINE_CHARS + ) + ) as pieces: + for piece, _ in pieces: + out_handle.write(piece) + # Reads stop at the display budget, even for a long line or member + # of the run artifact. + piece_limit = MAX_DISPLAY_LINE_SIZE + 1 if request_tag == RequestTags.TEST_SHOW_DIFF: with edgtest.runtest.RunTestOutput(rt_file_path.path) as rt_output: - static_text = ''.join(rt_output.combined_static_diff()) + with contextlib.closing( + rt_output.iter_static_diff_pieces(piece_limit) + ) as pieces: + shown = display_content(pieces) client_response = { 'id': configured_test_id, - **trunc_display_content(static_text) + **shown } elif request_tag == RequestTags.TEST_SHOW_RAW_OUTPUT: with edgtest.runtest.RunTestOutput(rt_file_path.path) as rt_output: - raw_output = ''.join(rt_output.combined_raw_output()) + with contextlib.closing( + rt_output.iter_raw_output_pieces(piece_limit) + ) as pieces: + shown = display_content(pieces) client_response = { 'id': configured_test_id, - **trunc_display_content(raw_output) + **shown } elif request_tag == RequestTags.TEST_SHOW_CURR_DIFF: rep_config = changes_file_path.parent.name group_configs = request_value.get('configs') or [rep_config] - rep_curr_text = await form_curr_diff_text(rep_config, rt_file_path) divergent_configs: List[str] = [] - for config_name in group_configs: - if config_name == rep_config: - continue - sibling_changes = changes_files[config_name] - sibling_rt = _rt_path_for(sibling_changes.parent, test_id) - if not sibling_rt.exists(): - divergent_configs.append(config_name) - continue - sibling_text = await form_curr_diff_text(config_name, sibling_rt) - if sibling_text != rep_curr_text: - divergent_configs.append(config_name) + with tempfile.TemporaryDirectory() as tmp_dir_name: + rep_path = Path(tmp_dir_name, 'curr.diff') + await write_curr_diff(rep_config, rt_file_path, rep_path) + for n, config_name in enumerate(group_configs): + if config_name == rep_config: + continue + sibling_changes = changes_files[config_name] + sibling_rt = _rt_path_for(sibling_changes.parent, test_id) + if not sibling_rt.exists(): + divergent_configs.append(config_name) + continue + # Compared a chunk at a time, then gone. + # Use a distinct path: filecmp caches results by path, size, + # and mtime even with shallow=False; reuse can hide divergence. + sibling_path = Path(tmp_dir_name, f"sibling-{n}.diff") + await write_curr_diff(config_name, sibling_rt, sibling_path) + if not filecmp.cmp(rep_path, sibling_path, shallow = False): + divergent_configs.append(config_name) + sibling_path.unlink() + + with open(rep_path, encoding = 'utf-8') as curr_diff_handle: + shown = display_text(curr_diff_handle) client_response = { 'id': configured_test_id, - **trunc_display_content(rep_curr_text) + **shown } if len(divergent_configs) != 0: @@ -552,8 +589,9 @@ async def form_curr_diff_text(config_name: str, ifc_dump_output = result.output.decode(errors = 'replace') if is_ranged: assert range_line is not None and range_context is not None + # Universal newlines, as a file opened as text has. reply_fields = slice_source_range( - ifc_dump_output.splitlines(), + io.StringIO(ifc_dump_output, newline = None), range_line, range_context ) @@ -568,11 +606,10 @@ async def form_curr_diff_text(config_name: str, range_context ) else: - # Only as much as can be shown is read (one character more marks - # the content as truncated). + # Only as much as can be shown is read. with path_to_show.open(errors = 'replace') as file_handle: - shown = file_handle.read(MAX_DISPLAY_CONTENT_SIZE + 1) - reply_fields = trunc_display_content(shown) + shown = display_text(file_handle) + reply_fields = shown await channel.send(message( RequestTags.TEST_SHOW_SOURCE, diff --git a/dev_tools/pylibs/edgacknowledg/forge/config.py b/dev_tools/pylibs/edgacknowledg/forge/config.py index 07485371f1..5f10ab8d45 100644 --- a/dev_tools/pylibs/edgacknowledg/forge/config.py +++ b/dev_tools/pylibs/edgacknowledg/forge/config.py @@ -32,8 +32,9 @@ # suite's name, is short. DEFAULT_MAX_PATTERNS = 20_000 # The largest diff (of tests/, in bytes) a pull request may have: it is -# held in memory, and copied and split into lines more than once. -DEFAULT_MAX_DIFF_BYTES = 64 * 1024 * 1024 +# kept with the review's rematerialized run (on disk, never in memory +# whole), and the builder applies all of it. +DEFAULT_MAX_DIFF_BYTES = 512 * 1024 * 1024 # The most entries an artifact may have (its zip's, and its tarballs' # together), and the largest its zip's central directory may be: zipfile # reads all of that directory into memory, before we can count. CI's test diff --git a/dev_tools/pylibs/edgacknowledg/forge/http.py b/dev_tools/pylibs/edgacknowledg/forge/http.py index 1398076465..7a905c86d5 100644 --- a/dev_tools/pylibs/edgacknowledg/forge/http.py +++ b/dev_tools/pylibs/edgacknowledg/forge/http.py @@ -26,6 +26,9 @@ _log = logging.getLogger(__name__) PAGE_SIZE = 100 +# The largest API response read (decoded): far more than a page of +# PAGE_SIZE items; downloads have their own limit. +MAX_RESPONSE_BYTES = 32 * 1024 * 1024 MAX_RATE_LIMIT_WAIT = 15 * 60.0 MAX_ATTEMPTS = 4 @@ -88,17 +91,19 @@ async def request(self, params: Optional[Mapping[str, Any]] = None, json_body: Optional[Any] = None) -> httpx.Response: '''Send a request, retrying transient failures and waiting out rate - limits; raise ``ForgeError`` on failure. + limits; raise ``ForgeError`` on failure, or for a response over + ``MAX_RESPONSE_BYTES`` (read only that far). ''' for attempt in range(MAX_ATTEMPTS): last_attempt = attempt + 1 == MAX_ATTEMPTS try: - response = await self._client.request( + request = self._client.build_request( method, path, params = params, json = json_body ) + response = await _send_capped(self._client, request, method, path) except httpx.HTTPError as ex: if last_attempt: raise ForgeError(f"{method} {path} failed: {ex}") from ex @@ -179,6 +184,40 @@ async def download(self, raise ForgeError(f"download of {path} failed: {ex}") from ex +async def _send_capped(client: httpx.AsyncClient, + request: httpx.Request, + method: str, + path: str) -> httpx.Response: + '''Send ``request``; return its response, its body read (decoded) as it + comes, and only to ``MAX_RESPONSE_BYTES``: ``ForgeError`` beyond. + ''' + response = await client.send(request, stream = True) + try: + body = bytearray() + async for chunk in response.aiter_bytes(): + body += chunk + if len(body) > MAX_RESPONSE_BYTES: + raise ForgeError( + f"{method} {path}: the response is over {MAX_RESPONSE_BYTES} bytes" + ) + finally: + await response.aclose() + # The same response with its body in place; it is decoded already, so + # without the headers that describe how it was sent. + headers = [ + (name, value) for name, value in response.headers.multi_items() + if name.lower() not in ('content-encoding', 'content-length', + 'transfer-encoding') + ] + return httpx.Response( + response.status_code, + headers = headers, + content = bytes(body), + request = request, + extensions = response.extensions + ) + + async def _write_capped(stream: httpx.Response, destination: Path, max_bytes: int, diff --git a/dev_tools/pylibs/edgacknowledg/forge/processor.py b/dev_tools/pylibs/edgacknowledg/forge/processor.py index 53491af47a..19a83c52f1 100644 --- a/dev_tools/pylibs/edgacknowledg/forge/processor.py +++ b/dev_tools/pylibs/edgacknowledg/forge/processor.py @@ -76,6 +76,9 @@ # message. MAX_CI_METADATA_BYTES = 64 * 1024 MAX_TEST_IDS_BYTES = 4 * 1024 * 1024 +# The longest test id taken from CI (a path within a suite); a longer line +# names no test, and is skipped rather than carried along. +MAX_TEST_ID_CHARS = 4096 # Retries for a failing pull request: 1 min, 2 min, ... up to 1 h; give up # after this many attempts until the pull request changes again. @@ -103,6 +106,11 @@ def _file_sha256(path: Path) -> str: return digest.hexdigest() +def _diff_patterns(path: Path, max_patterns: int) -> list[str]: + with open(path, encoding = 'utf-8') as handle: + return changes_patterns(handle, max_patterns = max_patterns) + + def _reusable(section: Optional[SectionRecord], source_key: str) -> Optional[SectionRecord]: '''Return ``section`` if it was built from ``source_key`` and its files @@ -117,7 +125,8 @@ def _reusable(section: Optional[SectionRecord], def _changes_test_ids(changes_elists: Iterable[Path]) -> list[str]: '''The test ids ``changes_elists`` name, within ``MAX_TEST_IDS_BYTES`` - (together, both as read and as JSON); ``WorkspaceError`` beyond. + (together, both as read and as JSON); ``WorkspaceError`` beyond. A line + longer than ``MAX_TEST_ID_CHARS`` is skipped. ''' test_ids = [] remaining = MAX_TEST_IDS_BYTES @@ -130,7 +139,14 @@ def _changes_test_ids(changes_elists: Iterable[Path]) -> list[str]: f"CI's changes.elist files are over {MAX_TEST_IDS_BYTES} bytes" ) text = content.decode('utf-8', errors = 'replace') - test_ids += [line.strip() for line in text.splitlines() if line.strip()] + for line in text.splitlines(): + if len(line) > MAX_TEST_ID_CHARS: + _log.warning('skipped a changes.elist line of %d characters', + len(line)) + continue + test_id = line.strip() + if test_id: + test_ids.append(test_id) # JSON escapes control and non-ASCII characters (e.g. the U+FFFD of bytes # that aren't UTF-8), up to six bytes each. if len(json.dumps(test_ids)) > MAX_TEST_IDS_BYTES: @@ -397,42 +413,52 @@ async def _changes_section(self, if reused is not None: return reused - diff = await self._workspaces.diff( - info.repository, - merge_base, - head_sha, - ['tests/'] - ) - if diff.strip() == '': - return None - - worktree = await self._workspaces.checkout( - info, - record.review_id, - 'base', - merge_base, - changes_patterns( - diff, - max_patterns = self._config.max_sparse_patterns - ) - ) - runs_dir = ensure_real_directory(worktree, PurePosixPath('tests', 'runs')) - run_dir = runs_dir / f"remat-{_sha256(source_key.encode())[:16]}" - remove_tree(run_dir) - # The builder sees the worktree read-only, so it writes elsewhere (on # the same file system, for the rename into place). + run_name = f"remat-{_sha256(source_key.encode())[:16]}" staging_root = ensure_real_directory( self._workspaces.build_staging_dir.parent, PurePosixPath(self._workspaces.build_staging_dir.name) ) staging = Path(tempfile.mkdtemp( - prefix = f"{record.review_id}-{run_dir.name}-", + prefix = f"{record.review_id}-{run_name}-", dir = staging_root )) try: + # Only ever in this file, never held in memory whole: it may be large. diff_file = staging / 'pull-request.diff' - diff_file.write_text(diff, encoding = 'utf-8') + await self._workspaces.diff( + info.repository, + merge_base, + head_sha, + ['tests/'], + diff_file + ) + if diff_file.stat().st_size == 0: + remove_tree(staging) + return None + patterns = await asyncio.to_thread( + _diff_patterns, diff_file, self._config.max_sparse_patterns + ) + + worktree = await self._workspaces.checkout( + info, + record.review_id, + 'base', + merge_base, + patterns + ) + # Prepare where the run goes once built: tests/runs/ in the + # worktree (through real directories only, not links the pull request + # committed), cleared of any earlier attempt so that the staging + # directory can be renamed into place. + runs_dir = ensure_real_directory( + worktree, + PurePosixPath('tests', 'runs') + ) + run_dir = runs_dir / run_name + remove_tree(run_dir) + timestamp = self._clock().strftime(edgutil.RUN_TIMESTAMP_FORMAT) result = await self._sandbox.run( [ @@ -477,6 +503,7 @@ async def _changes_section(self, ) remove_tree(staging) return None + fingerprint = await asyncio.to_thread(_file_sha256, diff_file) staging.rename(run_dir) except BaseException: remove_tree(staging) @@ -487,7 +514,7 @@ async def _changes_section(self, name = CHANGES_NAME, slug = section_slug(CHANGES_NAME), source_key = source_key, - content_fingerprint = _sha256(diff.encode()), + content_fingerprint = fingerprint, root = run_dir / timestamp, home = worktree / 'tests', details = {'run_dir': str(run_dir)}, diff --git a/dev_tools/pylibs/edgacknowledg/forge/workspace.py b/dev_tools/pylibs/edgacknowledg/forge/workspace.py index 81b0634f77..062a037922 100644 --- a/dev_tools/pylibs/edgacknowledg/forge/workspace.py +++ b/dev_tools/pylibs/edgacknowledg/forge/workspace.py @@ -26,6 +26,7 @@ import asyncio import base64 +import codecs import logging import os import re @@ -39,11 +40,12 @@ from dataclasses import dataclass from pathlib import Path, PurePosixPath -from typing import Iterable, Optional, Sequence +from typing import IO, Callable, Iterable, Optional, Sequence import edgtest import edgutil +from edgacknowledg.budget import Budget, BudgetExceeded from edgacknowledg.forge import Forge, PullRequestInfo from edgacknowledg.forge.config import ( DEFAULT_MAX_ARTIFACT_DIRECTORY_BYTES, @@ -72,6 +74,14 @@ # files) before it is killed; well within docker stop's default 10. _TERMINATE_ALLOWANCE = 5.0 +# The most output a git command run for its result (not one whose output +# goes to a sink) may have; far more than a rev-parse or for-each-ref of a +# pull request's refs has. +_MAX_GIT_OUTPUT_BYTES = 16 * 1024 * 1024 +# How much of a git command's stderr is kept (the rest is read, and +# dropped): a fetch relays what the remote says, which needn't be short. +_MAX_GIT_STDERR_BYTES = 64 * 1024 + class WorkspaceError(Exception): '''Raised when a workspace operation fails or input is unsafe.''' @@ -81,6 +91,10 @@ class UnsafeArchiveError(WorkspaceError): '''Raised for an artifact that is too large or has unsafe entries.''' +class _OutputTooLarge(Exception): + '''Raised when a process's output is over its limit (it is stopped).''' + + @dataclass(frozen = True) class GitResult: returncode: int @@ -130,26 +144,40 @@ def _patterns_for_changed_test_file(path: PurePosixPath) -> list[str]: return [f"/{_escape_pattern(str(path.parent))}/"] -def changed_paths(diff: str) -> list[str]: - '''Return the paths a ``git diff --no-renames`` output touches.''' - paths = [] - for line in diff.splitlines(): - match = _DIFF_HEADER.match(line) - if match is not None: - paths.append(match.group(1)) - return paths +def _add_patterns(patterns: dict[str, None], + more: list[str], + max_patterns: int) -> None: + '''Add ``more`` to ``patterns`` (an ordered set), within + ``max_patterns``. + ''' + patterns.update(dict.fromkeys(more)) + if len(patterns) > max_patterns: + raise WorkspaceError( + f"more than {max_patterns} sparse-checkout patterns (too many tests; " + f"{ENV_PREFIX}MAX_SPARSE_PATTERNS)" + ) -def changes_patterns(diff: str, +def changes_patterns(diff: IO[str], *, max_patterns: int = DEFAULT_MAX_PATTERNS) -> list[str]: - '''Sparse-checkout patterns for rematerializing ``diff`` (of tests/) on - the base tree: the test home and every affected test; at most - ``max_patterns``. + '''Sparse-checkout patterns for rematerializing the diff (of tests/) read + from ``diff`` on the base tree: the test home and every affected test; at + most ``max_patterns``. ''' patterns = dict.fromkeys(_TEST_HOME_PATTERNS) current_suite: Optional[str] = None - for line in diff.splitlines(): + limit = edgutil.MAX_LINE_CHARS + for piece, at_line_start in edgutil.iter_line_pieces(diff, limit + 1): + # Only whole lines within edgutil.MAX_LINE_CHARS matter: no header or + # expectation line is longer, and a longer one (e.g. of a recording) is + # passed over in pieces, never held whole. + if not at_line_start: + continue + text_length = len(piece) - (1 if piece.endswith('\n') else 0) + if text_length > limit: + continue + line = piece.rstrip('\n') match = _DIFF_HEADER.match(line) if match is not None: path = PurePosixPath(match.group(1)) @@ -199,20 +227,6 @@ def failures_patterns(test_ids: Iterable[str], return list(patterns) -def _add_patterns(patterns: dict[str, None], - more: list[str], - max_patterns: int) -> None: - '''Add ``more`` to ``patterns`` (an ordered set), within - ``max_patterns``. - ''' - patterns.update(dict.fromkeys(more)) - if len(patterns) > max_patterns: - raise WorkspaceError( - f"more than {max_patterns} sparse-checkout patterns (too many tests; " - f"{ENV_PREFIX}MAX_SPARSE_PATTERNS)" - ) - - def ensure_real_directory(root: Path, relative: PurePosixPath) -> Path: '''Create ``root/relative`` one component at a time, refusing to go through anything that isn't a real directory (e.g. a symlink a pull @@ -303,22 +317,25 @@ def remove_tree(path: Path) -> None: class _Budget: '''What extracting an artifact may still spend: bytes, and entries.''' def __init__(self, max_bytes: int, max_entries: int) -> None: - self.bytes = max_bytes - self.max_entries = max_entries - self.entries = max_entries + self.byte_budget = Budget(max_bytes) + self.entry_budget = Budget(max_entries) def spend(self, count: int) -> None: - self.bytes -= count - if self.bytes < 0: - raise UnsafeArchiveError('artifact exceeds the extracted size limit') + try: + self.byte_budget.spend(count) + except BudgetExceeded as ex: + raise UnsafeArchiveError( + 'artifact exceeds the extracted size limit' + ) from ex def count_entry(self) -> None: - self.entries -= 1 - if self.entries < 0: + try: + self.entry_budget.spend(1) + except BudgetExceeded as ex: raise UnsafeArchiveError( - f"artifact has more than {self.max_entries} entries " + f"artifact has more than {self.entry_budget.limit} entries " f"({ENV_PREFIX}MAX_ARTIFACT_ENTRIES)" - ) + ) from ex def _central_directory_size(zip_path: Path) -> Optional[int]: @@ -478,9 +495,15 @@ async def git(self, cwd: Optional[Path] = None, stdin: Optional[str] = None, check: bool = True, - max_output: Optional[int] = None) -> GitResult: - '''Run git; with ``max_output``, its output may be at most that many - bytes (it is stopped, and ``WorkspaceError`` raised, beyond). + max_output: Optional[int] = None, + stdout_sink: Optional[Callable[[bytes], None]] = None + ) -> GitResult: + '''Run git; with ``stdout_sink``, its output goes there as it comes, + rather than into the result, and may be at most ``max_output`` bytes + (it is stopped, and ``_OutputTooLarge`` raised, beyond: the caller + knows which limit that is). Without, its output goes into the result, + and may be at most ``_MAX_GIT_OUTPUT_BYTES`` (``WorkspaceError`` + beyond). Only the start of its stderr is kept. ''' command = ['git'] if git_dir is not None: @@ -497,18 +520,31 @@ async def git(self, cwd = cwd, env = self._git_env() ) - if max_output is None: - stdout, stderr = await process.communicate( - stdin.encode() if stdin is not None else None - ) - else: - assert stdin is None - bounded = await _communicate_bounded(process, max_output) - if bounded is None: - raise WorkspaceError( - f"git {args[0]}'s output is over {max_output} bytes" + stdin_bytes = stdin.encode() if stdin is not None else None + if stdout_sink is None: + assert max_output is None + collected = bytearray() + try: + stderr = await _communicate_bounded( + process, + _MAX_GIT_OUTPUT_BYTES, + collected.extend, + stdin_bytes ) - stdout, stderr = bounded + except _OutputTooLarge: + raise WorkspaceError( + f"git {' '.join(args)} wrote over {_MAX_GIT_OUTPUT_BYTES} bytes" + ) from None + stdout = bytes(collected) + else: + assert max_output is not None + stderr = await _communicate_bounded( + process, + max_output, + stdout_sink, + stdin_bytes + ) + stdout = b'' assert process.returncode is not None result = GitResult( process.returncode, @@ -609,15 +645,51 @@ async def diff(self, repository: str, base: str, head: str, - paths: Sequence[str]) -> str: + paths: Sequence[str], + destination: Path) -> None: + '''Write the diff of ``paths`` from ``base`` to ``head`` to (the new + file) ``destination``, as it comes (it may be large): UTF-8, with what + isn't replaced (U+FFFD). + ''' + decoder = codecs.getincrementaldecoder('utf-8')(errors = 'replace') async with self.lock(repository): - result = await self.git( - 'diff', '--no-renames', '--no-color', '--no-ext-diff', '--no-textconv', - base, head, '--', *paths, - git_dir = self.clone_dir(repository), - max_output = self._max_diff_bytes - ) - return result.stdout + with open(destination, 'xb') as handle: + + # Decoded and encoded again so that what isn't UTF-8 is replaced: + # the builder's edg-rematerialize-test-run (and changes_patterns) + # read it as UTF-8 text, and the Changes section's fingerprint + # (the file's) stays what it was when the diff was decoded whole. + # Incrementally, so a character split between chunks survives. + + written = 0 + + def write(chunk: bytes) -> None: + nonlocal written + decoded = decoder.decode(chunk).encode() + written += len(decoded) + if written > self._max_diff_bytes: + raise _OutputTooLarge() + handle.write(decoded) + + try: + await self.git( + 'diff', '--no-renames', '--no-color', '--no-ext-diff', + '--no-textconv', base, head, '--', *paths, + git_dir = self.clone_dir(repository), + max_output = self._max_diff_bytes, + stdout_sink = write + ) + # Finish decoding: bytes the decoder still holds (a character the + # output ended in the middle of) are written too, replaced. + decoded = decoder.decode(b'', final = True).encode() + if written + len(decoded) > self._max_diff_bytes: + raise _OutputTooLarge() + handle.write(decoded) + except _OutputTooLarge: + raise WorkspaceError( + f"the diff is over {self._max_diff_bytes} bytes " + f"({ENV_PREFIX}MAX_DIFF_BYTES)" + ) from None async def checkout(self, info: PullRequestInfo, @@ -701,32 +773,68 @@ async def collect_garbage(self, repository: str, open_count: int) -> None: await self.git('gc', '--auto', '--quiet', git_dir = clone_dir) +async def _read_start(stream: asyncio.StreamReader, keep: int) -> bytes: + '''Read ``stream`` to its end; return its first ``keep`` bytes (the rest + is dropped as it comes). + ''' + kept = bytearray() + while chunk := await stream.read(64 * 1024): + kept += chunk[:max(0, keep - len(kept))] + return bytes(kept) + + +async def _write_stdin(process: asyncio.subprocess.Process, + data: bytes) -> None: + '''Write ``data`` to ``process``'s stdin, then close it.''' + assert process.stdin is not None + try: + process.stdin.write(data) + await process.stdin.drain() + except (BrokenPipeError, ConnectionResetError): + # It exited without reading it all; its exit status tells. + pass + finally: + process.stdin.close() + + async def _communicate_bounded(process: asyncio.subprocess.Process, - limit: int) -> Optional[tuple[bytes, bytes]]: - '''``process``'s output (stdout, stderr), once it exits; ``None``, having - stopped it, should its stdout be over ``limit`` bytes. Should this be - canceled (or fail), it is stopped too, rather than left writing to a - pipe no one reads. + limit: int, + sink: Callable[[bytes], None], + stdin: Optional[bytes] = None) -> bytes: + '''Pass ``process``'s stdout to ``sink`` as it comes, in chunks (none held + here), having written ``stdin`` (if any) to it; return the start of its + stderr (``_MAX_GIT_STDERR_BYTES``) once it exits. Should its stdout be + over ``limit`` bytes, it is stopped, and ``_OutputTooLarge`` raised. + Should this be canceled (or fail), it is stopped too, rather than left + writing to a pipe no one reads. ''' assert process.stdout is not None and process.stderr is not None - errors = asyncio.create_task(process.stderr.read()) - chunks: list[bytes] = [] + reading_stderr = asyncio.create_task( + _read_start(process.stderr, _MAX_GIT_STDERR_BYTES) + ) + writing_stdin = ( + asyncio.create_task(_write_stdin(process, stdin)) + if stdin is not None else None + ) size = 0 try: while chunk := await process.stdout.read(64 * 1024): size += len(chunk) if size > limit: - await edgutil.async_terminate_within(process, _TERMINATE_ALLOWANCE) - return None - chunks.append(chunk) - output = b''.join(chunks), await errors + raise _OutputTooLarge() + sink(chunk) + if writing_stdin is not None: + await writing_stdin + stderr = await reading_stderr await process.wait() - return output + return stderr except BaseException: await edgutil.async_terminate_within(process, _TERMINATE_ALLOWANCE) raise finally: - errors.cancel() + reading_stderr.cancel() + if writing_stdin is not None: + writing_stdin.cancel() def basic_auth_header(user: str, password: str) -> str: diff --git a/dev_tools/pylibs/edgtest/__init__.py b/dev_tools/pylibs/edgtest/__init__.py index 0b8ef00085..3eba4d3f02 100644 --- a/dev_tools/pylibs/edgtest/__init__.py +++ b/dev_tools/pylibs/edgtest/__init__.py @@ -306,6 +306,89 @@ def find_changes_files(run_dir: Path) -> Dict[str, Path]: changes_files[dir_entry] = changes_file return changes_files +def _remove_run_tree(path: Path) -> int: + '''Remove the directory path (of a test run) whole; return how many + .rt.tar files it held. + ''' + count = 0 + for _, _, file_names in os.walk(path): + count += sum( + 1 for file_name in file_names + if file_name.endswith(runtest.RUN_TEST_OUTPUT_SUFFIX) + ) + shutil.rmtree(path) + return count + +def prune_run_archives(run_dir: Path) -> Tuple[int, int, int]: + '''Prune the run in run_dir down to what a review of it reads (the .rt.tar + files changes.elist names; see _scan_changes_file), e.g., to keep CI's + uploads small. The replays directory is removed whole. In each + configuration, a suite with no test changes.elist names (every suite, + without a changes.elist) is removed whole: its directory, and its + suite- logs. Of the other suites, the .rt.tar files changes.elist + doesn't name are removed. Return how many .rt.tar files were kept, how + many removed, and how many suites were removed. + ''' + kept = 0 + removed = 0 + suites = 0 + for dir_entry in os.listdir(run_dir): + config_dir = run_dir / dir_entry + if not config_dir.is_dir() or config_dir.is_symlink(): + continue + if dir_entry == 'replays': + removed += _remove_run_tree(config_dir) + continue + if dir_entry in ('recording-updates', 'patched-tests'): + continue + + # The .rt.tar files changes.elist names, by suite. + named: Dict[str, Set[Path]] = {} + changes_file = config_dir / 'changes.elist' + if changes_file.is_file(): + with open(changes_file, 'r') as file_handle: + test_ids = [ + test_id for test_id in (line.strip() for line in file_handle) + if test_id != '' + ] + for test_id in test_ids: + test_path = PurePosixPath(test_id) + if (test_path.is_absolute() or '..' in test_path.parts or + len(test_path.parts) < 2): + raise ValueError(f"invalid test id in {changes_file}: {test_id!r}") + named.setdefault(test_path.parts[0], set()).add( + config_dir / test_path.with_suffix(runtest.RUN_TEST_OUTPUT_SUFFIX) + ) + + # Each suite edgy ran (and logged) in the configuration. + for log_path in config_dir.glob('suite-*.elog'): + suite = log_path.stem[len('suite-'):] + suite_dir = config_dir / suite + walkable = suite_dir.is_dir() and not suite_dir.is_symlink() + if suite not in named: + if walkable: + removed += _remove_run_tree(suite_dir) + log_path.unlink() + try: + log_path.with_suffix('.log').unlink() + except FileNotFoundError: + pass + suites += 1 + continue + if not walkable: + continue + for root, _, file_names in os.walk(suite_dir): + for file_name in file_names: + if not file_name.endswith(runtest.RUN_TEST_OUTPUT_SUFFIX): + continue + file_path = Path(root, file_name) + if file_path in named[suite]: + kept += 1 + else: + file_path.unlink() + removed += 1 + return kept, removed, suites + def describe_rt_path(rt_file_path: Path, run_dir: Path) -> Tuple[str, str]: '''Return (relative_rt_path, config_name) for an .rt.tar under the run. diff --git a/dev_tools/pylibs/edgtest/runtest/__init__.py b/dev_tools/pylibs/edgtest/runtest/__init__.py index 0308d4b651..ff0002bdb2 100644 --- a/dev_tools/pylibs/edgtest/runtest/__init__.py +++ b/dev_tools/pylibs/edgtest/runtest/__init__.py @@ -12,6 +12,7 @@ import functools import hashlib import io +import itertools import json import os import re @@ -27,8 +28,8 @@ from collections import OrderedDict from pathlib import Path, PurePath from typing import ( - Any, Callable, Dict, IO, Iterator, List, Mapping, NamedTuple, Optional, - Pattern, Set, Tuple, cast + Any, Callable, Dict, Generator, IO, Iterator, List, Mapping, NamedTuple, + Optional, Pattern, Set, Tuple, cast ) def get_default_config_name(env: Optional[Dict[str, str]] = None) -> str: @@ -1195,6 +1196,59 @@ def emit_one(commands: List[Dict[str, Any]], result.extend(emit_one(group['commands'], group['diff_lines'])) return result +# A text that is opened when (and if) it is read. +TextOpener = Callable[[], IO[str]] + +def _hash_text_stream(open_text: TextOpener) -> str: + '''``hash_diff_text`` of the text ``open_text`` opens, read in chunks.''' + digest = hashlib.sha256() + with open_text() as handle: + while True: + chunk = handle.read(64 * 1024) + if not chunk: + break + digest.update(chunk.encode('utf-8', errors = 'replace')) + return digest.hexdigest() + +def iter_collapsed_case_diff_pieces( + command_diff_entries: List[Tuple[Dict[str, Any], TextOpener]], + limit: int) -> Generator[Tuple[str, bool], None, None]: + '''The collapsed ``format_collapsed_case_diffs`` of + ``command_diff_entries`` (with their stored labels), whose diffs are + opened rather than given as lines, as ``edgutil.iter_line_pieces`` pieces + of at most ``limit`` characters (each ``# command`` comment is one piece, + however long). A diff is read only as far as the pieces are taken, so + neither a long line nor a long diff is held whole. + ''' + # First-seen order of unique diff bodies (keyed by stored hash). + groups: Dict[str, Tuple[List[Dict[str, Any]], TextOpener]] = ( + OrderedDict() + ) + for command, open_text in command_diff_entries: + group_key = command.get('diff_hash') or _hash_text_stream(open_text) + if group_key not in groups: + groups[group_key] = ([], open_text) + groups[group_key][0].append(command) + + for commands, open_text in groups.values(): + with open_text() as handle: + pieces = edgutil.iter_line_pieces(handle, limit) + # The ---/+++ header, if it has one, comes before the comments; its + # lines are short (labels), so each must be one piece: a diff whose + # first lines are longer has no header (and isn't held to look). + head = list(itertools.islice(pieces, 2)) + if (len(head) == 2 and + all(at_line_start and piece.endswith('\n') + for piece, at_line_start in head) and + head[0][0].startswith('---') and + head[1][0].startswith('+++')): + yield from head + head = [] + for command in commands: + yield f"# {case_command_from_metadata(command)}\n", True + yield from head + yield from pieces + class RtTarContents: '''Manifest of filesystem files to pack into a .rt.tar[.gz]. @@ -1349,6 +1403,16 @@ def _require_tar(self) -> tarfile.TarFile: def _read_lines(self, member_name: str) -> List[str]: return _read_tar_lines_member(self._require_tar(), member_name) + def open_text_member(self, member_name: str) -> IO[str]: + '''Open a member as text (UTF-8, errors replaced), read as it is + decompressed: nothing is held but what is read. + ''' + return io.TextIOWrapper( + _extract_tar_member(self._require_tar(), member_name), + encoding = 'utf-8', + errors = 'replace' + ) + @property def metadata(self) -> Dict[str, Any]: if self._metadata is None: @@ -1430,9 +1494,9 @@ def iter_command_diffs(self) -> Iterator[List[str]]: for _entry, diff_lines in self.iter_command_diff_entries(): yield diff_lines - def iter_command_diff_entries( - self) -> Iterator[Tuple[Dict[str, Any], List[str]]]: - '''Yield (annotation metadata, change.diff lines) for diffs. + def iter_command_diff_members( + self) -> Iterator[Tuple[Dict[str, Any], str]]: + '''Yield (annotation metadata, change.diff member name) for diffs. Command diffs use the stored command metadata. Matches diffs use a synthetic command string ``Matches for case N``. @@ -1443,21 +1507,29 @@ def iter_command_diff_entries( case.get('commands', []), start = 1 ): - diff_lines = self.command_diff(case_number, command_number) - if diff_lines is not None: - yield command, diff_lines + member = f"cases/{case_number}/{command_number}/change.diff" + if member in self.member_names: + yield command, member matches = case.get('matches') if matches is None: continue - matches_diff_lines = self.case_matches_diff(case_number) - if matches_diff_lines is None: + member = f"cases/{case_number}/matches/change.diff" + if member not in self.member_names: continue matches_entry = { 'command': case_matches_annotation(case_number), 'diff_hash': matches.get('diff_hash') } - yield matches_entry, matches_diff_lines + yield matches_entry, member + + def iter_command_diff_entries( + self) -> Iterator[Tuple[Dict[str, Any], List[str]]]: + '''Yield (annotation metadata, change.diff lines) for diffs (see + ``iter_command_diff_members``). + ''' + for entry, member in self.iter_command_diff_members(): + yield entry, self._read_lines(member) def combined_static_diff(self, *, @@ -1475,6 +1547,48 @@ def combined_static_diff(self, collapse = collapse ) + def iter_static_diff_pieces( + self, + limit: int) -> Generator[Tuple[str, bool], None, None]: + '''``combined_static_diff()`` (collapsed, with the stored labels) as + ``edgutil.iter_line_pieces`` pieces of at most ``limit`` characters, + each member read only as far as the pieces are taken. + ''' + return iter_collapsed_case_diff_pieces( + [ + (entry, functools.partial(self.open_text_member, member)) + for entry, member in self.iter_command_diff_members() + ], + limit + ) + + def iter_raw_output_pieces( + self, + limit: int) -> Generator[Tuple[str, bool], None, None]: + '''``combined_raw_output()`` as ``edgutil.iter_line_pieces`` pieces of + at most ``limit`` characters (each ``# `` header line is one piece, + however long), each member read only as far as the pieces are taken. + ''' + for case_number, case in enumerate(self.metadata.get('cases', []), + start = 1): + for command_number, command in enumerate( + case.get('commands', []), + start = 1 + ): + command_str = case_command_from_metadata(command) + if command_str: + yield f"# {command_str}\n", True + with self.open_text_member( + f"cases/{case_number}/{command_number}/output.txt" + ) as handle: + yield from edgutil.iter_line_pieces(handle, limit) + + matches_member = f"cases/{case_number}/matches/output.txt" + if matches_member in self.member_names: + yield f"# {case_matches_annotation(case_number)}\n", True + with self.open_text_member(matches_member) as handle: + yield from edgutil.iter_line_pieces(handle, limit) + def combined_raw_output(self) -> List[str]: '''Concatenate command lines from metadata with each command's body. diff --git a/dev_tools/pylibs/edgutil/__init__.py b/dev_tools/pylibs/edgutil/__init__.py index a3cb0458a9..49ce5d8e63 100644 --- a/dev_tools/pylibs/edgutil/__init__.py +++ b/dev_tools/pylibs/edgutil/__init__.py @@ -27,7 +27,9 @@ Callable, Coroutine, Dict, + IO, Iterable, + Iterator, List, Optional, Set, @@ -193,6 +195,53 @@ def __exit__(self, def eprint(msg: str) -> None: print(msg, file = sys.stderr) +def iter_line_pieces(handle: IO[str], + limit: int) -> Iterator[Tuple[str, bool]]: + '''Yield ``(piece, at_line_start)`` for the text ``handle``, a line at a + time but never holding more than ``limit`` characters of one: a longer + line comes as several pieces, only the first of which has + ``at_line_start`` set. Concatenating the pieces gives back the text + exactly. + ''' + at_line_start = True + while True: + piece = handle.readline(limit) + if not piece: + break + yield piece, at_line_start + at_line_start = piece.endswith('\n') + +class LineTooLongError(ValueError): + '''Raised by ``iter_limited_lines`` for a line over its limit.''' + + def __init__(self, line_number: int, limit: int) -> None: + super().__init__( + f"line {line_number} is longer than {limit} characters" + ) + self.line_number = line_number + self.limit = limit + +def iter_limited_lines(handle: IO[str], limit: int) -> Iterator[str]: + '''Yield each line of the text ``handle``, with its line end ('\\n') if + it has one; raise ``LineTooLongError`` for a line longer than ``limit`` + characters (its end not counted), having read only ``limit + 1`` of + them. + ''' + # Every piece up to an overlong line's first is a whole line. + for line_number, (line, _) in enumerate( + iter_line_pieces(handle, limit + 1), + start = 1 + ): + text_length = len(line) - (1 if line.endswith('\n') else 0) + if text_length > limit: + raise LineTooLongError(line_number, limit) + yield line + +# The longest line a reader of untrusted line-oriented text (a pull +# request's diff, a CI artifact's files) need hold: far more than any path, +# test name, or status line, and small enough not to matter in memory. +MAX_LINE_CHARS = 64 * 1024 + def indent_text_block(text_block: str, n_times: int = 1) -> str: indent = ' ' * n_times return re.sub(r'^(.)', f"{indent}\\1", text_block, flags = re.MULTILINE) diff --git a/dev_tools/services/acknowledg-forge/README.md b/dev_tools/services/acknowledg-forge/README.md index 1568da7961..87edb3054a 100644 --- a/dev_tools/services/acknowledg-forge/README.md +++ b/dev_tools/services/acknowledg-forge/README.md @@ -114,7 +114,7 @@ from the environment variable shown as a fallback. | `ACKNOWLEDG_FORGE_MAX_EXTRACTED_BYTES` | 2 GiB | Largest extracted artifact (including inner tarballs) | | `ACKNOWLEDG_FORGE_MAX_ARTIFACT_ENTRIES` | `500000` | Most entries in an artifact (including inner tarballs'). CI's test results reach about 170,000 | | `ACKNOWLEDG_FORGE_MAX_ARTIFACT_DIRECTORY_BYTES` | 64 MiB | Largest artifact zip central directory, which is read into memory whole (about 106 bytes per entry in CI's test results) | -| `ACKNOWLEDG_FORGE_MAX_DIFF_BYTES` | 64 MiB | Largest diff of a pull request's `tests/`, which is held in memory and copied more than once: raising it may need a higher `mem_limit` for the service ([`compose.yaml`](compose.yaml)) | +| `ACKNOWLEDG_FORGE_MAX_DIFF_BYTES` | 512 MiB | Largest diff of a pull request's `tests/`. It is streamed to disk, not held in memory, and kept with the review's rematerialized run; the builder applies all of it within `ACKNOWLEDG_FORGE_REMATERIALIZE_TIMEOUT` | | `ACKNOWLEDG_FORGE_MAX_SPARSE_PATTERNS` | `20000` | Most sparse-checkout patterns in a review's worktree (up to two per test). Checking out takes time proportional to patterns times paths | | `ACKNOWLEDG_FORGE_MAX_CONCURRENCY` | `2` | Pull requests processed at once | | `ACKNOWLEDG_FORGE_REMATERIALIZE_TIMEOUT` | `900` | Seconds `edg-rematerialize-test-run` may run | diff --git a/dev_tools/services/acknowledg-forge/SANDBOXING.md b/dev_tools/services/acknowledg-forge/SANDBOXING.md index c89d1401a9..2f64e85bb0 100644 --- a/dev_tools/services/acknowledg-forge/SANDBOXING.md +++ b/dev_tools/services/acknowledg-forge/SANDBOXING.md @@ -527,10 +527,10 @@ them. Several layers keep the service from reading through one: | --- | --- | | Artifact download | `ACKNOWLEDG_FORGE_MAX_ARTIFACT_BYTES` (256 MiB), also counting the decompressed size of inner tarballs | | Artifact entries | `ACKNOWLEDG_FORGE_MAX_ARTIFACT_ENTRIES` (500,000), counting inner tarballs' too; and its zip's central directory, `ACKNOWLEDG_FORGE_MAX_ARTIFACT_DIRECTORY_BYTES` (64 MiB), checked before zipfile reads it | -| A pull request's diff of `tests/` | `ACKNOWLEDG_FORGE_MAX_DIFF_BYTES` (64 MiB) | +| A pull request's diff of `tests/` | `ACKNOWLEDG_FORGE_MAX_DIFF_BYTES` (512 MiB) | | A review worktree's sparse-checkout patterns | `ACKNOWLEDG_FORGE_MAX_SPARSE_PATTERNS` (20,000) | | A file read for a reviewer | 64 MiB; a larger one fails the request | -| A source shown to a reviewer | only as much as can be displayed is read | +| A source shown to a reviewer | reads stop after at most 16 MiB of text plus one read piece, even for an unterminated long line; the response is marked truncated | | Each container's `/tmp` | 512 MiB tmpfs; copies made for diffing are streamed there | | Build commands | `ACKNOWLEDG_FORGE_REMATERIALIZE_TIMEOUT` (15 min) | | Reviewer commands | 2 min | diff --git a/dev_tools/services/acknowledg-forge/tests/test_display.py b/dev_tools/services/acknowledg-forge/tests/test_display.py new file mode 100644 index 0000000000..a415cb5d5e --- /dev/null +++ b/dev_tools/services/acknowledg-forge/tests/test_display.py @@ -0,0 +1,211 @@ +# Part of the EDG Compiler Project, under the Apache License v2.0 with LLVM +# Exceptions. +# See https://edgcpp.org/LICENSE.txt for license information. +# SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception + +'''Content cut for display to reviewers: whole, and line by line.''' + +import contextlib +import io +import time + +from typing import Iterator, Optional + +import pytest + +from edgacknowledg import MAX_DISPLAY_LABEL_SIZE +from edgacknowledg.budget import Budget, BudgetExceeded +from edgacknowledg.client import shared +from edgacknowledg.client.shared import ( + display_content, + display_text, + slice_source_range, + trunc_display_content, + trunc_display_label, +) + + +@pytest.fixture +def small_limits(monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.setattr(shared, 'MAX_DISPLAY_CONTENT_SIZE', 40) + monkeypatch.setattr(shared, 'MAX_DISPLAY_LINE_SIZE', 8) + + +def test_budget_counts_exactly_and_reports_overflow() -> None: + budget = Budget(5) + budget.spend(2) + budget.spend(3) + assert budget.limit == 5 + assert budget.remaining == 0 + assert not budget.exceeded + with pytest.raises(BudgetExceeded): + budget.spend(1) + assert budget.exceeded + assert budget.remaining == 0 + + budget = Budget(2) + with pytest.raises(BudgetExceeded): + budget.spend(3) + assert budget.remaining == 2 + assert budget.exceeded + + +def test_short_content_is_untouched(small_limits: None) -> None: + for content in ('', 'a', 'a\nb\n', 'a\n\n\nb', '12345678\n'): + assert trunc_display_content(content) == {'content': content} + + +def test_long_lines_are_cut_and_numbered(small_limits: None) -> None: + result = trunc_display_content('short\n123456789\nok\n' + 'x' * 20) + assert result == { + 'content': 'short\n12345678\nok\n' + 'x' * 8, + 'lines_truncated': [2, 4], + } + + +def test_long_content_is_cut_at_a_line_end(small_limits: None) -> None: + # Eight lines of six (with their ends) fit six times in 40. + lines = [f"line{n}" for n in range(8)] + result = trunc_display_content('\n'.join(lines)) + assert result == { + 'content': '\n'.join(lines[:6]), + 'truncated': True, + } + + +def test_cut_lines_count_as_cut(small_limits: None) -> None: + # Lines cut, then the whole: only lines kept are listed. + content = '\n'.join(['y' * 20] * 10) + result = trunc_display_content(content) + assert result['truncated'] + assert result['content'] == '\n'.join(['y' * 8] * 4) + assert result['lines_truncated'] == [1, 2, 3, 4] + assert len(result['content']) <= 40 + + +def test_one_line_larger_than_everything( + monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.setattr(shared, 'MAX_DISPLAY_CONTENT_SIZE', 10) + monkeypatch.setattr(shared, 'MAX_DISPLAY_LINE_SIZE', 100) + assert trunc_display_content('z' * 50 + '\nmore') == { + 'content': 'z' * 10, + 'truncated': True, + 'lines_truncated': [1], + } + + +def test_no_characters_are_lost_between_lines(small_limits: None) -> None: + # Every character of each kept line survives the line joins. + lines = ['abcdefg'] * 10 + content = trunc_display_content('\n'.join(lines))['content'] + assert all(line == 'abcdefg' for line in content.split('\n')) + + +def test_large_content_is_linear() -> None: + # Many short lines past the content limit, and a huge one, go quickly. + content = 'a\n' * 8_000_000 + 'b' * 20_000_000 + start = time.monotonic() + result = trunc_display_content(content) + assert time.monotonic() - start < 20 + assert result['truncated'] + assert 'lines_truncated' not in result + + +def test_labels_end_in_an_ellipsis() -> None: + assert trunc_display_label('PASS') == 'PASS' + fits = 'x' * MAX_DISPLAY_LABEL_SIZE + assert trunc_display_label(fits) == fits + label = trunc_display_label(fits + 'x') + assert len(label) == MAX_DISPLAY_LABEL_SIZE + assert label.endswith('\N{HORIZONTAL ELLIPSIS}') + + +def test_display_reads_only_what_fits(small_limits: None) -> None: + taken: list[str] = [] + + def pieces() -> Iterator[tuple[str, bool]]: + for n in range(1000): + taken.append(f"{n}") + yield f"line{n}\n", True + + with contextlib.closing(pieces()) as generator: + shown = display_content(generator) + assert shown['truncated'] + # Up to the first line that doesn't fit, and no more. + assert len(taken) == 7 + assert shown['content'] == '\n'.join(f"line{n}" for n in range(6)) + + +def test_display_cuts_lines_across_pieces(small_limits: None) -> None: + # A line in several pieces; then a text without a last line end, which + # the next line doesn't run into. + pieces = [ + ('abcde', True), ('fghij', False), ('k\n', False), + ('no end', True), + ('# next\n', True), + ] + assert display_content(pieces) == { + 'content': 'abcdefgh\nno end\n# next\n', + 'lines_truncated': [1], + } + + +def test_source_ranges_cut_lines(small_limits: None) -> None: + text = 'a\n' + 'b' * 20 + '\nc\n' + 'd' * 9 + '\ne\n' + # Numbered within the window. + assert slice_source_range(io.StringIO(text), 3, 1) == { + 'content': 'b' * 8 + '\nc\n' + 'd' * 8, + 'first_line': 2, + 'lines_truncated': [1, 3], + } + # Cut lines outside the window aren't listed. + assert slice_source_range(io.StringIO(text), 5, 0) == { + 'content': 'e', + 'first_line': 5, + } + # A window past the end of the file is empty, where it was asked for. + assert slice_source_range(io.StringIO(text), 9, 1) == { + 'content': '', + 'first_line': 8, + } + + +def test_source_ranges_read_no_further(small_limits: None) -> None: + reads: list[int] = [] + + class Handle(io.StringIO): + def readline(self, size: Optional[int] = -1) -> str: + assert size is not None + reads.append(size) + return super().readline(size) + + # Of each line, no more than a piece at a time; and nothing past the + # window. + handle = Handle('x' * 100 + '\ny\n' + 'z' * 100 + '\n') + shown = slice_source_range(handle, 1, 1) + assert shown == { + 'content': 'x' * 8 + '\ny', + 'first_line': 1, + 'lines_truncated': [1], + } + assert set(reads) == {shared.MAX_DISPLAY_LINE_SIZE + 1} + assert handle.read() == 'z' * 100 + '\n' + + +def test_unterminated_long_lines_stop_at_read_budget( + small_limits: None, monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.setattr(shared, '_MAX_SOURCE_READ_CHARS', 40) + text = 'x' * 1000 + handle = io.StringIO(text) + shown = display_text(handle) + assert shown == { + 'content': 'x' * 8, + 'truncated': True, + 'lines_truncated': [1], + } + assert handle.tell() <= 40 + 9 + + handle = io.StringIO(text) + shown = slice_source_range(handle, 2, 0) + assert shown == {'content': '', 'first_line': 2, 'truncated': True} + assert handle.tell() <= 40 + 9 diff --git a/dev_tools/services/acknowledg-forge/tests/test_github.py b/dev_tools/services/acknowledg-forge/tests/test_github.py index 3c333825a7..be2b08f64a 100644 --- a/dev_tools/services/acknowledg-forge/tests/test_github.py +++ b/dev_tools/services/acknowledg-forge/tests/test_github.py @@ -235,3 +235,38 @@ def handler(request: httpx.Request) -> httpx.Response: assert posted == [{'body': 'hello'}] assert await commenter.find_comment(REPO, 1, 'MARK') == '2' assert await commenter.find_comment(REPO, 1, 'OTHER') is None + + +async def test_responses_are_read_only_so_far( + monkeypatch: pytest.MonkeyPatch) -> None: + from edgacknowledg.forge import ForgeError + from edgacknowledg.forge import http as forge_http + import gzip + + def handler(request: httpx.Request) -> httpx.Response: + if request.url.path == '/small': + # Compressed, and paginated: decoded once, links kept. + return httpx.Response( + 200, + headers = { + 'content-encoding': 'gzip', + 'link': f'<{API}/small?page=2>; rel="next"', + }, + content = gzip.compress(json.dumps({'ok': True}).encode()) + ) + return httpx.Response(200, json = {'pad': 'x' * 1000}) + + client = forge_http.ForgeHttpClient( + base_url = API, + headers = {}, + transport = httpx.MockTransport(handler) + ) + try: + monkeypatch.setattr(forge_http, 'MAX_RESPONSE_BYTES', 100) + response = await client.request('GET', '/small') + assert response.json() == {'ok': True} + assert response.links['next']['url'] == f"{API}/small?page=2" + with pytest.raises(ForgeError, match = 'over 100 bytes'): + await client.get_json('/large') + finally: + await client.aclose() diff --git a/dev_tools/services/acknowledg-forge/tests/test_rematerialize.py b/dev_tools/services/acknowledg-forge/tests/test_rematerialize.py index 7123d85ca2..a6e4e5d623 100644 --- a/dev_tools/services/acknowledg-forge/tests/test_rematerialize.py +++ b/dev_tools/services/acknowledg-forge/tests/test_rematerialize.py @@ -12,8 +12,20 @@ import sys from pathlib import Path, PurePosixPath +from typing import Any + +import pytest import edgtest +import edgutil + +# (Not 'from ... import TestSourceHandler', which pytest would collect.) +import edgacknowledg.client.test as test_provider + +from edgacknowledg import RequestTags, message +from edgacknowledg.client.shared import SandboxContext + +from test_symlinks import FakeChannel DEV_TOOLS = Path(__file__).resolve().parents[3] REMATERIALIZE = DEV_TOOLS / 'bin' / 'edg-rematerialize-test-run' @@ -155,7 +167,16 @@ def static_diff(test: str) -> str: edgtest.runtest.RUN_TEST_OUTPUT_SUFFIX ) with edgtest.runtest.RunTestOutput(rt_path) as rt_output: - return ''.join(rt_output.combined_static_diff()) + text = ''.join(rt_output.combined_static_diff()) + # As pieces (for display), the same, header lines fitting in one. + for limit in (len('--- Expected Output\n'), 1000): + assert ''.join( + piece for piece, _ in rt_output.iter_static_diff_pieces(limit) + ) == text + assert ''.join( + piece for piece, _ in rt_output.iter_raw_output_pieces(limit) + ) == ''.join(rt_output.combined_raw_output()) + return text c_diff = static_diff('suite/c.sft.cpp') assert '# cpfe -DTEST_NUMBER=1 Test_name.c\n' in c_diff @@ -179,3 +200,166 @@ def static_diff(test: str) -> str: for config, changes_file in changes_files.items() for test in changes_file.read_text().split() ) + + +def _rematerialize_change(tmp_path: Path, + change: dict[str, str]) -> tuple[ + subprocess.CompletedProcess[str], Path]: + '''Run the tool on the diff ``change`` makes to a small test home; return + its result and the run's directory. + ''' + repo = tmp_path / 'repo' + repo.mkdir() + git(repo, 'init', '--quiet', '--initial-branch=main') + write_files(repo, { + 'tests/.edgy/config.json': '{}\n', + 'tests/expectations/edg_x86_64/suite.log': '', + 'tests/expectations/edg_x86_64_cp/suite.log': '', + 'tests/tests/suite/a.sft.cpp': '//type:cp\nint a;\n', + 'tests/tests/suite/.a.rto/default.1.1.txt': ( + 'cpfe -DTEST_NUMBER=1 Test_name.c\nold output\n' + ), + }) + git(repo, 'add', '-A') + git(repo, 'commit', '--quiet', '-m', 'base') + write_files(repo, change) + git(repo, 'add', '-A') + git(repo, 'commit', '--quiet', '-m', 'change') + diff_file = tmp_path / 'pr.diff' + diff_file.write_text(git( + repo, 'diff', '--no-renames', '--no-color', 'HEAD~1', 'HEAD', '--', 'tests' + )) + git(repo, 'checkout', '--quiet', 'HEAD~1') + + runs_dir = tmp_path / 'runs' + timestamp = '2026.01.02-03.04.05' + env = dict(os.environ) + env['EDG_TEST_HOME'] = str(repo / 'tests') + env['PYTHONPATH'] = str(DEV_TOOLS / 'pylibs') + env.pop('RUN_TEST_DEFAULT_CONFIG', None) + result = subprocess.run( + [sys.executable, str(REMATERIALIZE), + '--runs-dir', str(runs_dir), + '--run-timestamp', timestamp, + str(diff_file)], + cwd = repo, + env = env, + capture_output = True, + text = True + ) + return result, runs_dir / timestamp + + +def test_rematerialize_passes_huge_lines_through(tmp_path: Path) -> None: + # Far longer than a line is ever held whole; written (and patched) in + # full. + huge = 'h' * (5 * edgutil.MAX_LINE_CHARS) + result, run_dir = _rematerialize_change(tmp_path, { + 'tests/tests/suite/.a.rto/default.1.1.txt': ( + f"cpfe -DTEST_NUMBER=1 Test_name.c\n{huge}\n" + ), + }) + assert result.returncode == 0, result.stdout + result.stderr + rt_path = (run_dir / 'edg_x86_64' / 'suite/a.sft.cpp').with_suffix( + edgtest.runtest.RUN_TEST_OUTPUT_SUFFIX + ) + with edgtest.runtest.RunTestOutput(rt_path) as rt_output: + static_diff = ''.join(rt_output.combined_static_diff()) + assert f"+{huge}\n" in static_diff + + +def test_rematerialize_refuses_huge_expectation_lines(tmp_path: Path) -> None: + result, _ = _rematerialize_change(tmp_path, { + 'tests/expectations/edg_x86_64/suite.log': ( + 'a.sft.cpp:cp:-DTEST_NUMBER=1:FAIL:' + + 'r' * edgutil.MAX_LINE_CHARS + '\n' + ), + }) + assert result.returncode == 1 + assert ( + 'tests/expectations/edg_x86_64/suite.log: a changed line is longer than' + ) in result.stderr + + +async def test_curr_diff_is_shown_and_compared( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None: + result, run_dir = _rematerialize_change(tmp_path, { + 'tests/tests/suite/.a.rto/default.1.1.txt': ( + 'cpfe -DTEST_NUMBER=1 Test_name.c\nnew output\n' + ), + # Brings the other configuration into the run (which inherits the + # recording's change). + 'tests/expectations/edg_x86_64_cp/suite.log': ( + 'a.sft.cpp:cp:-DTEST_NUMBER=1:FAIL:\n' + ), + }) + assert result.returncode == 0, result.stdout + result.stderr + context = SandboxContext(tmp_path) + handler = test_provider.TestSourceHandler( + 'Test', + 'test', + context, + context.subpath(tmp_path / 'repo' / 'tests'), + context.subpath(run_dir) + ) + + async def show_curr_diff( + configs: tuple[str, ...] = ('edg_x86_64', 'edg_x86_64_cp') + ) -> dict[str, Any]: + channel = FakeChannel(message( + RequestTags.TEST_SHOW_CURR_DIFF, + value = { + 'id': '(edg_x86_64) suite/a.sft.cpp', + 'configs': list(configs), + 'reviewer_id': 'r', + } + )) + await handler.handle_request(channel) # type: ignore[arg-type] + [reply] = channel.sent + response: dict[str, Any] = reply['v']['client_response'] + return response + + # Observed against the run's (the pull request's) recordings, which it + # was made from: no difference. + response = await show_curr_diff() + assert response['content'] == '' + assert 'divergent_configs' not in response + + # Recordings changed since: the difference is shown, the same for both + # configurations (the other uses the default recording). + recordings = run_dir / 'patched-tests' / 'suite' / '.a.rto' + (recordings / 'default.1.1.txt').write_text( + 'cpfe -DTEST_NUMBER=1 Test_name.c\nnewer output\n' + ) + response = await show_curr_diff() + assert response['content'] == ( + '--- Expected Output\n' + '+++ Observed Output\n' + '# cpfe -DTEST_NUMBER=1 Test_name.c\n' + '@@ -1 +1 @@\n' + '-newer output\n' + '+new output\n' + ) + assert 'divergent_configs' not in response + + # A recording of its own makes the other configuration's differ. + (recordings / 'edg_x86_64_cp.1.1.txt').write_text( + 'cpfe -DTEST_NUMBER=1 Test_name.c\nnew output\n' + ) + response = await show_curr_diff() + assert '-newer output\n+new output' in response['content'] + assert response['divergent_configs'] == ['edg_x86_64_cp'] + + compared: list[Path] = [] + original_cmp = test_provider.filecmp.cmp + + def cmp(first: Path, second: Path, *, shallow: bool) -> bool: + compared.append(second) + return original_cmp(first, second, shallow = shallow) + + monkeypatch.setattr(test_provider.filecmp, 'cmp', cmp) + await show_curr_diff( + ('edg_x86_64', 'edg_x86_64_cp', 'edg_x86_64_cp') + ) + assert len(compared) == 2 + assert compared[0] != compared[1] diff --git a/dev_tools/services/acknowledg-forge/tests/test_safety.py b/dev_tools/services/acknowledg-forge/tests/test_safety.py index 822538d76a..c7534a4afa 100644 --- a/dev_tools/services/acknowledg-forge/tests/test_safety.py +++ b/dev_tools/services/acknowledg-forge/tests/test_safety.py @@ -17,12 +17,18 @@ import pytest +import edgutil + +from edgacknowledg.forge import workspace + from edgacknowledg.forge import processor from edgacknowledg.forge.processor import _changes_test_ids, _read_ci_metadata from edgacknowledg.forge.sandbox import Sandbox, SandboxUnavailableError from edgacknowledg.forge.workspace import ( UnsafeArchiveError, WorkspaceError, + Workspaces, + _OutputTooLarge, _central_directory_size, _communicate_bounded, changes_patterns, @@ -32,6 +38,9 @@ patterns_for_test, ) +from conftest import Origin, pull_request_info +from fakes import REPOSITORY, FakeForge + def _zip(path: Path, entries: dict[str, bytes]) -> Path: with zipfile.ZipFile(path, 'w') as archive: for name, data in entries.items(): @@ -71,7 +80,8 @@ def test_extract_caps_total_size(tmp_path: Path) -> None: archive = _zip(tmp_path / 'a.zip', {'big.txt': b'0' * 100_000}) destination = tmp_path / 'out' destination.mkdir() - with pytest.raises(UnsafeArchiveError): + with pytest.raises(UnsafeArchiveError, + match = 'artifact exceeds the extracted size limit'): extract_artifact(archive, destination, max_bytes = 10_000) @@ -226,7 +236,7 @@ def test_changes_patterns_cover_recordings_and_expectations() -> None: '-dir/b.sft.cpp: PASS', '+dir/b.sft.cpp: FAIL', ]) - patterns = changes_patterns(diff) + patterns = changes_patterns(io.StringIO(diff)) assert '/tests/tests/s/.a.rto/' in patterns assert '/tests/tests/s/a.*' in patterns assert '/tests/tests/other/dir/b.sft.cpp' in patterns @@ -243,6 +253,63 @@ def test_test_patterns_are_linear_in_the_path() -> None: ] +def test_line_pieces_are_bounded_and_lose_nothing() -> None: + text = ( + 'diff --git a/tests/tests/s/a.sft.cpp b/tests/tests/s/a.sft.cpp\n' + '+' + 'x' * 1000 + '\n\n+last' + ) + pieces = list(edgutil.iter_line_pieces(io.StringIO(text), 16)) + assert ''.join(piece for piece, _ in pieces) == text + assert all(len(piece) <= 16 for piece, _ in pieces) + # Only a line's first piece starts it. + assert [piece for piece, at_line_start in pieces if at_line_start] == [ + 'diff --git a/tes', '+xxxxxxxxxxxxxxx', '\n', '+last' + ] + + +def test_limited_lines_refuse_overlong_lines() -> None: + # Up to the limit (its end not counted), lines come whole. + text = '0123456789\nshort\n' + 'y' * 11 + '\nnever read\n' + lines = edgutil.iter_limited_lines(io.StringIO(text), 10) + assert next(lines) == '0123456789\n' + assert next(lines) == 'short\n' + with pytest.raises(edgutil.LineTooLongError, + match = 'line 3 is longer than 10 characters'): + next(lines) + # A last line without an end is checked too. + def limited(text: str) -> list[str]: + return list(edgutil.iter_limited_lines(io.StringIO(text), 10)) + + assert limited('a\n' + 'b' * 10) == ['a\n', 'b' * 10] + with pytest.raises(edgutil.LineTooLongError): + limited('a\n' + 'b' * 11) + + +def test_changes_patterns_pass_over_huge_lines_in_pieces() -> None: + # A line far over the limit, in an expectations file, names no test (and + # is never held whole); the lines after it still count. + diff = ( + 'diff --git a/tests/expectations/x/s.log b/tests/expectations/x/s.log\n' + '+++ b/tests/expectations/x/s.log\n' + '+big.sft.cpp:' + 'q' * (4 * edgutil.MAX_LINE_CHARS) + '\n' + '+ok.sft.cpp:cp:-DTEST_NUMBER=1:PASS:\n' + ) + handle = io.StringIO(diff) + real_readline = handle.readline + sizes: list[int] = [] + + def readline(size: int = -1) -> str: + piece = real_readline(size) + sizes.append(len(piece)) + return piece + + handle.readline = readline # type: ignore[method-assign] + patterns = changes_patterns(handle) + assert '/tests/tests/s/ok.sft.cpp' in patterns + assert not any('big' in pattern for pattern in patterns) + assert max(sizes) <= edgutil.MAX_LINE_CHARS + 1 + + def test_changes_patterns_skip_suites_no_suite_may_be_named() -> None: # A suite is in the patterns of every test its expectation file names. def expectations(suite: str) -> str: @@ -254,10 +321,10 @@ def expectations(suite: str) -> str: '+a.sft.cpp: PASS', ]) - patterns = changes_patterns(expectations('cwg')) + patterns = changes_patterns(io.StringIO(expectations('cwg'))) assert '/tests/tests/cwg/a.sft.cpp' in patterns for suite in ['s' * 65, 'a.b', 'a b']: - assert changes_patterns(expectations(suite)) == [ + assert changes_patterns(io.StringIO(expectations(suite))) == [ '/tests/.edgy/', '/tests/expectations/', ] @@ -276,34 +343,132 @@ def test_patterns_are_capped() -> None: *(f"+{n}.sft.cpp: FAIL" for n in range(cap)), ]) with pytest.raises(WorkspaceError, match = 'sparse-checkout patterns'): - changes_patterns(diff, max_patterns = cap) + changes_patterns(io.StringIO(diff), max_patterns = cap) # Within a higher cap. - changes_patterns(diff, max_patterns = 3 * cap) + changes_patterns(io.StringIO(diff), max_patterns = 3 * cap) async def test_git_output_can_be_bounded() -> None: - async def run(*argv: str, limit: int) -> object: - process = await asyncio.create_subprocess_exec( - *argv, - stdout = asyncio.subprocess.PIPE, - stderr = asyncio.subprocess.PIPE - ) - result = await _communicate_bounded(process, limit) - assert process.returncode is not None - return result - - assert await run('sh', '-c', 'printf out; printf err >&2', - limit = 3) == (b'out', b'err') + # Stdout goes to the sink as it comes; stderr is returned. + sunk: list[bytes] = [] + process = await asyncio.create_subprocess_exec( + 'sh', '-c', 'printf out; printf err >&2', + stdout = asyncio.subprocess.PIPE, + stderr = asyncio.subprocess.PIPE + ) + assert await _communicate_bounded(process, 3, sunk.append) == b'err' + assert process.returncode is not None + assert b''.join(sunk) == b'out' # One that would never stop is stopped (letting it clean up). process = await asyncio.create_subprocess_exec( 'yes', stdout = asyncio.subprocess.PIPE, stderr = asyncio.subprocess.PIPE ) - assert await _communicate_bounded(process, 1024 * 1024) is None + with pytest.raises(_OutputTooLarge): + await _communicate_bounded(process, 1024 * 1024, sunk.append) assert process.returncode == -signal.SIGTERM +async def test_git_stdin_is_written_and_stderr_bounded() -> None: + sunk: list[bytes] = [] + # It reads its input (larger than a pipe holds) while writing more + # stderr than is kept. + process = await asyncio.create_subprocess_exec( + 'sh', '-c', 'cat; head -c 1000000 /dev/zero | tr "\\0" e >&2', + stdin = asyncio.subprocess.PIPE, + stdout = asyncio.subprocess.PIPE, + stderr = asyncio.subprocess.PIPE + ) + data = b'pattern\n' * 100_000 + stderr = await _communicate_bounded( + process, + len(data), + sunk.append, + data + ) + assert process.returncode == 0 + assert b''.join(sunk) == data + assert stderr == b'e' * workspace._MAX_GIT_STDERR_BYTES + + +async def test_git_results_are_bounded(tmp_path: Path, + forge: FakeForge, + monkeypatch: pytest.MonkeyPatch + ) -> None: + workspaces = Workspaces(tmp_path / 'home', forge) + assert (await workspaces.git('version')).stdout.startswith('git version') + monkeypatch.setattr(workspace, '_MAX_GIT_OUTPUT_BYTES', 4) + with pytest.raises(WorkspaceError, match = 'wrote over 4 bytes'): + await workspaces.git('version') + + +async def test_diffs_are_written_as_they_come(tmp_path: Path, + origin: Origin, + forge: FakeForge) -> None: + # Longer than a chunk git's output comes in, with characters of several + # bytes across them. + lines = [f"{n}.sft.cpp: {'é€😀' * 10}" for n in range(2000)] + head = origin.push_pull_request(1, { + 'tests/expectations/x/s.log': ''.join(f"{line}\n" for line in lines), + }) + workspaces = Workspaces(tmp_path / 'home', forge) + info = pull_request_info(1, head, origin.main_sha) + head, _ = await workspaces.fetch_pull_request(info) + diff_file = tmp_path / 'pull-request.diff' + await workspaces.diff( + REPOSITORY, + origin.main_sha, + head, + ['tests/'], + diff_file + ) + diff = diff_file.read_text(encoding = 'utf-8') + assert len(diff_file.read_bytes()) > 64 * 1024 + assert '\ufffd' not in diff + added = [ + line[1:] for line in diff.splitlines() + if line.startswith('+') and not line.startswith('+++') + ] + assert added == lines + # Its patterns, read from the file. + with open(diff_file, encoding = 'utf-8') as handle: + assert '/tests/tests/s/1999.sft.cpp' in changes_patterns(handle) + + # Too long: refused. + workspaces = Workspaces(tmp_path / 'home', forge, max_diff_bytes = 1024) + with pytest.raises( + WorkspaceError, + match = r'over 1024 bytes \(ACKNOWLEDG_FORGE_MAX_DIFF_BYTES\)'): + await workspaces.diff( + REPOSITORY, + origin.main_sha, + head, + ['tests/'], + tmp_path / 'too-long.diff' + ) + + +@pytest.mark.parametrize('raw', [b'\xff\xff', b'\xe2']) +async def test_diff_limit_includes_replacement_bytes( + tmp_path: Path, + forge: FakeForge, + monkeypatch: pytest.MonkeyPatch, + raw: bytes) -> None: + workspaces = Workspaces(tmp_path / 'home', forge, max_diff_bytes = 2) + + async def git(*args: str, **kwargs: object) -> None: + sink = kwargs['stdout_sink'] + assert callable(sink) + sink(raw) + + monkeypatch.setattr(workspaces, 'git', git) + destination = tmp_path / 'expanded.diff' + with pytest.raises(WorkspaceError, match = 'over 2 bytes'): + await workspaces.diff(REPOSITORY, 'base', 'head', ['tests/'], destination) + assert destination.stat().st_size <= 2 + + async def test_cancelled_git_output_reads_stop_git() -> None: # As when the service stops while git still has output to write: left # writing to a pipe no one reads, it would never exit. @@ -312,7 +477,9 @@ async def test_cancelled_git_output_reads_stop_git() -> None: stdout = asyncio.subprocess.PIPE, stderr = asyncio.subprocess.PIPE ) - reading = asyncio.create_task(_communicate_bounded(process, 1 << 40)) + reading = asyncio.create_task( + _communicate_bounded(process, 1 << 40, lambda chunk: None) + ) await asyncio.sleep(0.2) reading.cancel() await asyncio.wait_for(asyncio.gather(reading, return_exceptions = True), 10) @@ -348,6 +515,16 @@ def test_ci_reads_are_capped(tmp_path: Path, _read_ci_metadata(metadata) +def test_ci_test_ids_skip_overlong_lines(tmp_path: Path) -> None: + elist = tmp_path / 'a.elist' + elist.write_text( + 's/a.sft.cpp\n' + + 's/' + 'x' * processor.MAX_TEST_ID_CHARS + '\n' + + 's/b.sft.cpp\n' + ) + assert _changes_test_ids([elist]) == ['s/a.sft.cpp', 's/b.sft.cpp'] + + def test_failures_patterns_ignore_escaping_ids() -> None: patterns = failures_patterns(['s/a.sft.cpp', '../../etc/passwd']) assert '/tests/tests/s/a.sft.cpp' in patterns diff --git a/dev_tools/services/acknowledg-forge/tests/test_state.py b/dev_tools/services/acknowledg-forge/tests/test_state.py index 2b45050ae7..b1149425f1 100644 --- a/dev_tools/services/acknowledg-forge/tests/test_state.py +++ b/dev_tools/services/acknowledg-forge/tests/test_state.py @@ -427,7 +427,7 @@ def test_webhooks_need_the_service_bind(tmp_path: Path) -> None: @pytest.mark.parametrize('setting, problem', [ - ('MAX_DIFF_BYTES', "git diff's output is over 1 bytes"), + ('MAX_DIFF_BYTES', 'MAX_DIFF_BYTES'), ('MAX_SPARSE_PATTERNS', 'MAX_SPARSE_PATTERNS'), ('MAX_ARTIFACT_ENTRIES', 'MAX_ARTIFACT_ENTRIES'), ('MAX_ARTIFACT_DIRECTORY_BYTES', 'MAX_ARTIFACT_DIRECTORY_BYTES'), diff --git a/dev_tools/services/acknowledg-forge/tests/test_symlinks.py b/dev_tools/services/acknowledg-forge/tests/test_symlinks.py index c10e0472f5..41be601c46 100644 --- a/dev_tools/services/acknowledg-forge/tests/test_symlinks.py +++ b/dev_tools/services/acknowledg-forge/tests/test_symlinks.py @@ -557,7 +557,6 @@ async def test_show_source_reads_only_what_is_shown( monkeypatch: pytest.MonkeyPatch) -> None: home, root = _test_tree(tmp_path, secret) (home / 'tests' / 'suite' / 'big.sft.cpp').write_text('int x;\n' * 1000) - monkeypatch.setattr(test_provider, 'MAX_DISPLAY_CONTENT_SIZE', 64) monkeypatch.setattr(shared, 'MAX_DISPLAY_CONTENT_SIZE', 64) reads: list[int] = [] real_open = shared.ConfinedPath.open @@ -566,12 +565,17 @@ def open_recording_reads(self: shared.ConfinedPath, *args: Any, **kwargs: Any) -> Any: handle = real_open(self, *args, **kwargs) - real_read = handle.read + real_readline = handle.readline + + def readline(size: int = -1) -> Any: + line = real_readline(size) + reads.append(len(line)) + return line def read(size: int = -1) -> Any: - reads.append(size) - return real_read(size) + raise AssertionError('read whole') + handle.readline = readline handle.read = read return handle @@ -594,7 +598,8 @@ def read(size: int = -1) -> Any: response = reply['v']['client_response'] assert response['truncated'] and len(response['content']) <= 64 assert response['content'].startswith('int x;\n') - assert reads == [65] + # A line at a time, and no more than one past what fits. + assert reads == [7] * 10 async def test_show_source_range(tmp_path: Path, secret: Path) -> None: @@ -625,7 +630,7 @@ async def ask(test_id: str, line: int, ctx: int) -> dict[str, Any]: # The window is returned, and the request echoed. response = await ask('suite/ten.sft.cpp', 5, 2) assert response['content'] == 'line 3\nline 4\nline 5\nline 6\nline 7' - assert response['first_line'] == 3 and response['total_lines'] == 10 + assert response['first_line'] == 3 and 'total_lines' not in response assert response['line'] == 5 and response['context'] == 2 assert 'truncated' not in response diff --git a/dev_tools/services/acknowledg/PROTOCOL.md b/dev_tools/services/acknowledg/PROTOCOL.md index b40dd98ddd..a1f6d3d77c 100644 --- a/dev_tools/services/acknowledg/PROTOCOL.md +++ b/dev_tools/services/acknowledg/PROTOCOL.md @@ -3,7 +3,7 @@ This document describes the WebSocket protocol used by the AcknowlEDG server, the Vue client, and the `edgy-review` / `edg-bench-review` / `edg-acknowledg-cli` providers. -The current protocol revision is **20**. +The current protocol revision is **21**. All frames are JSON text. Implementations live in `edgacknowledg` (`dev_tools/pylibs/edgacknowledg`) and must stay in sync with the JavaScript @@ -74,15 +74,15 @@ work can flow immediately. Providers may optionally send identity updates at any time (on dedicated sockets or nested in `batch.event`): ```json -{ "t": "meta.section.name", "r": 20, "v": "" } +{ "t": "meta.section.name", "r": 21, "v": "" } ``` ```json -{ "t": "meta.review.name", "r": 20, "v": [ ] } +{ "t": "meta.review.name", "r": 21, "v": [ ] } ``` ```json -{ "t": "meta.review.timestamp", "r": 20, "v": "" } +{ "t": "meta.review.timestamp", "r": 21, "v": "" } ``` - `meta.section.name` sets the display name for the section tab (shown in @@ -329,7 +329,7 @@ content that isn't the provider user's own, one reached through a symlink), the provider doesn't answer the request. It reports the reviewer instead: ```json -{ "t": "illegal-access", "r": 20, "v": { "reviewer_id": "" } } +{ "t": "illegal-access", "r": 21, "v": { "reviewer_id": "" } } ``` The server then closes that reviewer's socket with code **3001**, without @@ -345,7 +345,7 @@ access), it doesn't answer the request either. It tells the reviewer it failed, through the server, and keeps serving the section: ```json -{ "t": "invalid", "r": 20, "v": { "reviewer_id": "" } } +{ "t": "invalid", "r": 21, "v": { "reviewer_id": "" } } ``` The server sends `invalid` to that reviewer alone, with `v` `null` and `s` @@ -362,6 +362,19 @@ missing) and may set `truncated: true` when the body exceeds 8 MiB (`MAX_DISPLAY_CONTENT_SIZE`). Protocol messages themselves are capped at 10 MiB (`MAX_MESSAGE_SIZE`). +Each line of `content` is at most 16 Ki characters +(`MAX_DISPLAY_LINE_SIZE`). A provider cuts a longer line to that length and +lists its 1-based line number in `lines_truncated` (sorted ascending, and +omitted when no line was cut). Nothing in `content` itself marks a cut. The +reviewer UI appends `…` (U+2026) to each listed line and shows a "Some Lines +Truncated" notice. Line cuts happen before the 8 MiB cap, so `truncated` and +`lines_truncated` may both be set; `lines_truncated` lists only lines +present in `content`. + +Single-line labels (the `qualifier`, `variant`, and `status` of +`test.src.list-statuses` lines, and its `remark`) are cut to 1 Ki characters +(`MAX_DISPLAY_LABEL_SIZE`), ending in `…`. + **`test.src.list-statuses`** ```json @@ -392,7 +405,12 @@ missing) and may set `truncated: true` when the body exceeds 8 MiB **`test.src.show-diff`**, **`test.src.show-output`** ```json -{ "id": "", "content": "", "truncated": false } +{ + "id": "", + "content": "", + "truncated": false, + "lines_truncated": [1] +} ``` **`test.src.show-diff-curr`** — same as above, plus optional @@ -421,7 +439,8 @@ any Unicode name is allowed). The primary file itself is not in `files`; it is "id": "", "assoc_file": "", "content": "", - "truncated": false + "truncated": false, + "lines_truncated": [1] } ``` @@ -430,8 +449,10 @@ neither (otherwise the server answers `invalid`). Then `content` holds only lines `line - context` through `line + context` (clamped to the file, joined with `\n`, no trailing newline), `line` and `context` are echoed back so the reviewer can match the reply, and the reply adds `first_line` (the 1-based -number of the first returned line) and `total_lines`. `truncated` is not -sent. A missing file still yields `content: null`. +number of the first requested line, `content` being empty should the file +end before it). `truncated` is not sent; `lines_truncated` is, numbering +lines within `content` (1 for the line at `first_line`). A missing file +still yields `content: null`. **`bench.src.list-statuses`** @@ -451,7 +472,12 @@ sent. A missing file still yields `content: null`. **`bench.src.show-annotate`** ```json -{ "id": "", "content": "", "truncated": false } +{ + "id": "", + "content": "", + "truncated": false, + "lines_truncated": [1] +} ``` ### Editor `v` as seen by the reviewer diff --git a/dev_tools/services/acknowledg/client/src/BenchReviewView.vue b/dev_tools/services/acknowledg/client/src/BenchReviewView.vue index b414c930b4..cccff6704f 100644 --- a/dev_tools/services/acknowledg/client/src/BenchReviewView.vue +++ b/dev_tools/services/acknowledg/client/src/BenchReviewView.vue @@ -22,6 +22,7 @@ SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception } from './bench-metrics.js' import { formatBenchAnchor, parseBenchAnchor } from './review-anchor.js' import { protocolMessage } from './sockets.js' + import { displayLines } from './display-content.js' import AppButton from './AppButton.vue' import AppSelect from './AppSelect.vue' import BenchStatus from './BenchStatus.vue' @@ -47,8 +48,14 @@ SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception ] const selectedBenchmarkId = ref(null) const displayTruncated = ref(false) + // 1-based numbers of the displayed lines the provider cut. + const displayedLinesTruncated = ref([]) const displayedOutput = ref(null) + const displayedOutputLines = computed(() => { + return displayLines(displayedOutput.value, displayedLinesTruncated.value) + }) + provide('activeBenchmarkId', selectedBenchmarkId) provide('benchSortMode', sortMode) @@ -215,12 +222,14 @@ SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception } else if (message.t === 'bench.src.show-annotate') { if (message.v.id === selectedBenchmarkId.value) { displayTruncated.value = false + displayedLinesTruncated.value = [] if (message.v.content === null) { displayedOutput.value = 'Annotation unavailable!' } else { if (message.v.truncated) { displayTruncated.value = true } + displayedLinesTruncated.value = message.v.lines_truncated ?? [] displayedOutput.value = message.v.content } } @@ -409,12 +418,17 @@ SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception class="truncation-warning"> Content Too Long: Display Truncated +
+ Some Lines Truncated +
- 
{{ displayedOutput }}
+ 
{{ displayedOutputLines.join('\n') }}
{ + return displayLines(displayedOutput.value, displayedLinesTruncated.value) + }) + const displayedTestConfig = computed(() => { return displayedGroupConfigs.value[0] ?? "" }) @@ -150,7 +157,13 @@ SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception } const source = reply.content === null ? { missing: true } : - { content: reply.content, firstLine: reply.first_line } + { + content: displayLines( + reply.content, + reply.lines_truncated ?? [] + ).join('\n'), + firstLine: reply.first_line + } sourceSnippetCache.set(snippetKey(reply.assoc_file, reply.line), source) const request = sourcePopover.value.request if (sourcePopover.value.open && request !== null && @@ -444,6 +457,7 @@ SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception message.t === "test.src.show-output") { if (message.v.id === configuredIdForDisplayedTest()) { displayTruncated.value = false + displayedLinesTruncated.value = [] currDiffDivergenceNotice.value = null if (message.v.content === null) { displayedOutput.value = "Missing .rt.tar file!" @@ -451,6 +465,7 @@ SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception if (message.v.truncated) { displayTruncated.value = true } + displayedLinesTruncated.value = message.v.lines_truncated ?? [] displayedOutput.value = message.v.content } if (message.t === "test.src.show-diff-curr" && @@ -473,6 +488,7 @@ SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception } else if (message.t === "test.src.show-src") { if (message.v.id === displayedTestName.value) { displayTruncated.value = false + displayedLinesTruncated.value = [] currDiffDivergenceNotice.value = null if (message.v.content === null) { displayedOutput.value = "File not found!" @@ -480,6 +496,7 @@ SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception if (message.v.truncated) { displayTruncated.value = true } + displayedLinesTruncated.value = message.v.lines_truncated ?? [] displayedOutput.value = message.v.content } } @@ -655,6 +672,11 @@ SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception class="content-notice"> Content Too Long: Display Truncated
+
+ Some Lines Truncated +
{{ currDiffDivergenceNotice }} @@ -673,15 +695,15 @@ SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception padding-left: 0; --content-inset: 5px;"> + :text="displayedOutputLines.join('\n')" /> diff --git a/dev_tools/services/acknowledg/client/src/display-content.js b/dev_tools/services/acknowledg/client/src/display-content.js new file mode 100644 index 0000000000..242223e2a0 --- /dev/null +++ b/dev_tools/services/acknowledg/client/src/display-content.js @@ -0,0 +1,27 @@ +/* +Part of the EDG Compiler Project, under the Apache License v2.0 with LLVM +Exceptions. +See https://edgcpp.org/LICENSE.txt for license information. +SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +*/ + +// Display content as providers send it: `content`, maybe with +// `lines_truncated`, the 1-based numbers of the lines they cut (see +// MAX_DISPLAY_LINE_SIZE in PROTOCOL.md). Nothing in the content marks a cut; +// the lines are marked here. + +// U+2026 HORIZONTAL ELLIPSIS (JavaScript has no named escapes). +const TRUNCATION_MARK = '\u2026' + +// The lines of `content`, those numbered in `linesTruncated` ending in an +// ellipsis; none for null content. +export const displayLines = (content, linesTruncated) => { + if (content === null || content === undefined) { + return [] + } + const lines = content.split('\n') + for (const lineNumber of linesTruncated) { + lines[lineNumber - 1] += TRUNCATION_MARK + } + return lines +} diff --git a/dev_tools/services/acknowledg/client/src/sockets.js b/dev_tools/services/acknowledg/client/src/sockets.js index bea1823d0e..cf927dd8ef 100644 --- a/dev_tools/services/acknowledg/client/src/sockets.js +++ b/dev_tools/services/acknowledg/client/src/sockets.js @@ -5,7 +5,7 @@ See https://edgcpp.org/LICENSE.txt for license information. SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception */ -const PROTOCOL_REVISION = 20 +const PROTOCOL_REVISION = 21 const getSocketHost = () => { if (process.env.NODE_ENV === "development") { diff --git a/dev_tools/tests/test_edgtest_prune.py b/dev_tools/tests/test_edgtest_prune.py new file mode 100644 index 0000000000..6e1a54d49a --- /dev/null +++ b/dev_tools/tests/test_edgtest_prune.py @@ -0,0 +1,134 @@ +# Part of the EDG Compiler Project, under the Apache License v2.0 with LLVM +# Exceptions. +# See https://edgcpp.org/LICENSE.txt for license information. +# SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception + +'''Tests for pruning a test run's .rt.tar files (edgtest.prune_run_archives) +and edg-prune-test-run. + +Run with: python3 -m unittest discover -s dev_tools/tests +(with dev_tools/pylibs on PYTHONPATH). +''' + +import os +import subprocess +import sys +import tempfile +import unittest + +from pathlib import Path +from typing import List + +import edgtest + +from edgtest import runtest + +DEV_TOOLS_DIR = Path(__file__).resolve().parents[1] +PRUNE_TOOL_PATH = DEV_TOOLS_DIR / 'bin' / 'edg-prune-test-run' + +SUFFIX = runtest.RUN_TEST_OUTPUT_SUFFIX + +def _touch(path: Path) -> Path: + path.parent.mkdir(parents = True, exist_ok = True) + path.write_bytes(b'') + return path + +def _files(root: Path) -> List[str]: + return sorted( + Path(directory, name).relative_to(root).as_posix() + for directory, _, names in os.walk(root) + for name in names + ) + +class PruneRunArchivesTests(unittest.TestCase): + + def setUp(self) -> None: + temp_dir = tempfile.TemporaryDirectory() + self.addCleanup(temp_dir.cleanup) + self.run_dir = Path(temp_dir.name) / 'run' + # One configuration whose changes.elist names two tests of one suite + # (and one without results), with every test's results... + config = self.run_dir / 'edg_x86_64' + _touch(config / 'changes.elist').write_text( + 'suite/a.sft.cpp\nsuite/dir/b.sft.c\nsuite/none.sft.cpp\n\n' + ) + for name in ('changes.elog', 'executed.elist', 'suite-suite.elog', + 'suite-suite.log', 'suite-other.elog', 'suite-other.log'): + _touch(config / name) + for name in ('suite/a.sft', 'suite/dir/b.sft', 'suite/c.sft', + 'other/a.sft'): + _touch(config / f"{name}{SUFFIX}") + # ...one without a changes.elist... + config = self.run_dir / 'edg_x86_32' + for name in ('executed.elist', 'suite-suite.elog', 'suite-suite.log', + f"suite/a.sft{SUFFIX}"): + _touch(config / name) + # ...and the run's replays (removed whole). + _touch(self.run_dir / 'replays' / f"suite/c.sft{SUFFIX}") + + def test_keeps_only_what_changes_elist_names(self) -> None: + changes_file = self.run_dir / 'edg_x86_64' / 'changes.elist' + read = list(edgtest._scan_changes_file(changes_file)) + + self.assertEqual(edgtest.prune_run_archives(self.run_dir), (2, 4, 2)) + # Suites with no changes go whole, and unnamed results with them. + self.assertEqual(_files(self.run_dir), [ + 'edg_x86_32/executed.elist', + 'edg_x86_64/changes.elist', + 'edg_x86_64/changes.elog', + 'edg_x86_64/executed.elist', + 'edg_x86_64/suite-suite.elog', + 'edg_x86_64/suite-suite.log', + 'edg_x86_64/suite/a.sft' + SUFFIX, + 'edg_x86_64/suite/dir/b.sft' + SUFFIX, + ]) + # A review reads what it read before. + self.assertEqual(list(edgtest._scan_changes_file(changes_file)), read) + + # Pruning again changes nothing. + self.assertEqual(edgtest.prune_run_archives(self.run_dir), (2, 0, 0)) + + def test_keeps_nothing_without_changes_elist(self) -> None: + (self.run_dir / 'edg_x86_64' / 'changes.elist').unlink() + self.assertEqual(edgtest.prune_run_archives(self.run_dir), (0, 6, 3)) + self.assertEqual(_files(self.run_dir), [ + 'edg_x86_32/executed.elist', + 'edg_x86_64/changes.elog', + 'edg_x86_64/executed.elist', + ]) + + def test_rejects_invalid_test_ids(self) -> None: + changes_file = self.run_dir / 'edg_x86_64' / 'changes.elist' + for test_id in ('a.sft.cpp', '/suite/a.sft.cpp', + 'suite/../a.sft.cpp', '../suite/a.sft.cpp'): + with self.subTest(test_id = test_id): + changes_file.write_text(f"{test_id}\n") + with self.assertRaisesRegex(ValueError, 'invalid test id'): + edgtest.prune_run_archives(self.run_dir) + self.assertTrue( + (self.run_dir / 'edg_x86_64' / f"suite/a.sft{SUFFIX}").exists() + ) + + def test_command(self) -> None: + env = dict(os.environ) + env['PYTHONPATH'] = str(DEV_TOOLS_DIR / 'pylibs') + result = subprocess.run( + [sys.executable, str(PRUNE_TOOL_PATH), str(self.run_dir)], + capture_output = True, + text = True, + env = env + ) + self.assertEqual(result.returncode, 0, result.stderr) + self.assertIn('Kept 2', result.stdout) + self.assertIn('removed 4 others, and 2 suites', result.stdout) + + missing = subprocess.run( + [sys.executable, str(PRUNE_TOOL_PATH), str(self.run_dir / 'missing')], + capture_output = True, + text = True, + env = env + ) + self.assertEqual(missing.returncode, 1) + +if __name__ == '__main__': + unittest.main()