From 877010c5f6dea82a843d4c05c16edb486180d99d Mon Sep 17 00:00:00 2001 From: Jerens Lensun <54782057+jerensl@users.noreply.github.com> Date: Sun, 22 Jun 2025 23:30:19 +0800 Subject: [PATCH] Mach clippy & test-tidy github inline annotation (#37294) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When the --report-ci flag is passed to ./mach clippy or ./mach test-tidy, the commands emit CI-friendly output to files in the tempy directory. These files can later be used by [GitHub Workflow Commands](https://docs.github.com/en/actions/writing-workflows/choosing-what-your-workflow-does/workflow-commands-for-github-actions#setting-an-error-message) for annotations. If the flag is not provided, the default behavior remains unchanged. Both clippy and test-tidy will limit have 10 limit annotation ⚠️ Note: For ./mach clippy --report-ci to work correctly, the Clippy command must use --message-format=json. If it's not specified, CI output will not be generated, and no warning or error will be shown. Example PR: https://github.com/jerensl/servo/pull/1/files Fixes: #37231 --------- Signed-off-by: Jerens Lensun --- .github/workflows/lint.yml | 5 +- python/servo/command_base.py | 15 ++-- python/servo/devenv_commands.py | 34 +++++++-- python/servo/testing_commands.py | 26 ++++--- python/tidy/linting_report.py | 114 +++++++++++++++++++++++++++++++ python/tidy/tidy.py | 10 ++- 6 files changed, 175 insertions(+), 29 deletions(-) create mode 100644 python/tidy/linting_report.py diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml index 375a997b13d..d2dba9d4a63 100644 --- a/.github/workflows/lint.yml +++ b/.github/workflows/lint.yml @@ -43,9 +43,8 @@ jobs: run: | sudo apt update ./mach bootstrap - # TODO: Do GitHub anotaions - name: Clippy run: | - ./mach clippy --use-crown --locked -- -- --deny warnings + ./mach clippy --use-crown --locked --github-annotations -- -- --deny warnings - name: Tidy - run: ./mach test-tidy --no-progress --all + run: ./mach test-tidy --no-progress --all --github-annotations diff --git a/python/servo/command_base.py b/python/servo/command_base.py index 3c8e6fe837e..fdc39d56c65 100644 --- a/python/servo/command_base.py +++ b/python/servo/command_base.py @@ -10,8 +10,6 @@ from __future__ import annotations import contextlib -from enum import Enum -from typing import Any, Dict, List, Optional import functools import gzip import itertools @@ -24,24 +22,23 @@ import sys import tarfile import urllib import zipfile - from dataclasses import dataclass +from enum import Enum from errno import ENOENT as NO_SUCH_FILE_OR_DIRECTORY from glob import glob from os import path from subprocess import PIPE +from typing import Any, Dict, List, Optional from xml.etree.ElementTree import XML import toml - from mach.decorators import CommandArgument, CommandArgumentGroup from mach.registrar import Registrar -from servo.platform.build_target import BuildTarget, AndroidTarget, OpenHarmonyTarget -from servo.util import download_file, get_default_cache_dir - import servo.platform import servo.util as util +from servo.platform.build_target import AndroidTarget, BuildTarget, OpenHarmonyTarget +from servo.util import download_file, get_default_cache_dir from python.servo.platform.build_target import SanitizerKind @@ -804,6 +801,7 @@ class CommandBase(object): with_debug_assertions=False, with_frame_pointer=False, use_crown=False, + capture_output=False, target_override: Optional[str] = None, **_kwargs, ): @@ -876,6 +874,9 @@ class CommandBase(object): # but uv venv on Windows only provides a `python`, not `python3`. env["PYTHON3"] = "python" + if capture_output: + return subprocess.run(["cargo", command] + args + cargo_args, env=env, capture_output=capture_output) + return call(["cargo", command] + args + cargo_args, env=env, verbose=verbose) def android_adb_path(self, env): diff --git a/python/servo/devenv_commands.py b/python/servo/devenv_commands.py index 2ccba1ee64d..0daeef231ca 100644 --- a/python/servo/devenv_commands.py +++ b/python/servo/devenv_commands.py @@ -7,20 +7,21 @@ # option. This file may not be copied, modified, or distributed # except according to those terms. -from os import path, listdir, getcwd - +import json import signal import subprocess import sys import tempfile +from os import getcwd, listdir, path from mach.decorators import ( + Command, CommandArgument, CommandProvider, - Command, ) +from tidy.linting_report import GitHubAnnotationManager -from servo.command_base import CommandBase, cd, call +from servo.command_base import CommandBase, call, cd @CommandProvider @@ -108,8 +109,14 @@ class MachCommands(CommandBase): @Command("clippy", description='Run "cargo clippy"', category="devenv") @CommandArgument("params", default=None, nargs="...", help="Command-line arguments to be passed through to clippy") + @CommandArgument( + "--github-annotations", + default=False, + action="store_true", + help="Emit the clippy warnings in the Github Actions annotations format", + ) @CommandBase.common_command_arguments(build_configuration=True, build_type=False) - def cargo_clippy(self, params, **kwargs): + def cargo_clippy(self, params, github_annotations=False, **kwargs): if not params: params = [] @@ -117,6 +124,23 @@ class MachCommands(CommandBase): self.ensure_clobbered() env = self.build_env() env["RUSTC"] = "rustc" + + if github_annotations: + if "--message-format=json" not in params: + params.insert(0, "--message-format=json") + + github_annotation_manager = GitHubAnnotationManager("clippy") + + results = self.run_cargo_build_like_command("clippy", params, env=env, capture_output=True, **kwargs) + if results.returncode == 0: + return 0 + try: + github_annotation_manager.emit_annotations_for_clippy( + [json.loads(line) for line in results.stdout.splitlines() if line.strip()] + ) + except json.JSONDecodeError: + pass + return results.returncode return self.run_cargo_build_like_command("clippy", params, env=env, **kwargs) @Command("grep", description="`git grep` for selected directories.", category="devenv") diff --git a/python/servo/testing_commands.py b/python/servo/testing_commands.py index ed717e26f74..4e19549c695 100644 --- a/python/servo/testing_commands.py +++ b/python/servo/testing_commands.py @@ -8,34 +8,32 @@ # except according to those terms. import argparse +import json import logging -import re -import sys import os import os.path as path import platform +import re import shutil import subprocess +import sys import textwrap -import json -import servo.devtools_tests -from servo.post_build_commands import PostBuildCommands +import tidy import wpt import wpt.manifestupdate import wpt.run import wpt.update - from mach.decorators import ( + Command, CommandArgument, CommandProvider, - Command, ) +import servo.devtools_tests import servo.try_parser -import tidy - from servo.command_base import BuildType, CommandBase, call, check_call +from servo.post_build_commands import PostBuildCommands from servo.util import delete SCRIPT_PATH = os.path.split(__file__)[0] @@ -254,8 +252,14 @@ class MachCommands(CommandBase): help="Check all files, and run the WPT lint in tidy, even if unchanged", ) @CommandArgument("--no-progress", default=False, action="store_true", help="Don't show progress for tidy") - def test_tidy(self, all_files, no_progress): - tidy_failed = tidy.scan(not all_files, not no_progress) + @CommandArgument( + "--github-annotations", + default=False, + action="store_true", + help="Emit tidy warnings in the Github Actions annotations format", + ) + def test_tidy(self, all_files, no_progress, github_annotations): + tidy_failed = tidy.scan(not all_files, not no_progress, github_annotations) print("\r ➤ Checking formatting of Rust files...") rustfmt_failed = format_with_rustfmt(check_only=True) diff --git a/python/tidy/linting_report.py b/python/tidy/linting_report.py new file mode 100644 index 00000000000..dcacd70ffef --- /dev/null +++ b/python/tidy/linting_report.py @@ -0,0 +1,114 @@ +# Copyright 2025 The Servo Project Developers. See the COPYRIGHT +# file at the top-level directory of this distribution. +# +# Licensed under the Apache License, Version 2.0 or the MIT license +# , at your +# option. This file may not be copied, modified, or distributed +# except according to those terms. + +from dataclasses import dataclass +from typing import Any, Literal, NotRequired + + +@dataclass +class GithubAnnotation: + file_name: str + line_start: int + line_end: int + level: Literal["notice", "warning", "error"] + title: str + message: str + column_start: NotRequired[int] + column_end: NotRequired[int] + + +class GitHubAnnotationManager: + def __init__(self, annotation_prefix: str, limit: int = 10): + self.annotation_prefix: str = annotation_prefix + self.limit: int = limit + self.total_count: int = 0 + + def clean_path(self, path: str): + return path.removeprefix("./") + + def escape(self, s: str): + return s.replace("\r", "%0D").replace("\n", "%0A") + + def emit_annotation( + self, + title: str, + message: str, + file_name: str, + line_start: int, + line_end: int | None = None, + annotation_level: str = "error", + column_start: int | None = None, + column_end: int | None = None, + ): + if self.total_count >= self.limit: + return + + if line_end is None: + line_end = line_start + + annotation: GithubAnnotation = { + "title": f"{self.annotation_prefix}: {self.escape(title)}", + "message": self.escape(message), + "file_name": self.clean_path(file_name), + "line_start": line_start, + "line_end": line_end, + "level": annotation_level, + } + + if line_start == line_end and column_start is not None and column_end is not None: + annotation["column_start"] = column_start + annotation["column_end"] = column_end + + line_info = f"line={annotation['line_start']},endLine={annotation['line_end']},title={annotation['title']}" + + column_info = "" + if "column_end" in annotation and "column_start" in annotation: + column_info = f"col={annotation['column_start']},endColumn={annotation['column_end']}," + + print( + f"::{annotation['level']} file={annotation['file_name']},{column_info}{line_info}::{annotation['message']}" + ) + + self.total_count += 1 + + def emit_annotations_for_clippy(self, data: list[dict[str, Any]]): + severenty_map: dict[str, Literal["notice", "warning", "error"]] = { + "help": "notice", + "note": "notice", + "warning": "warning", + "error": "error", + } + + for item in data: + if self.total_count >= self.limit: + break + + message = item.get("message") + if not message: + continue + + spans = message.get("spans") or [] + primary_span = next((span for span in spans if span.get("is_primary")), None) + if not primary_span: + continue + + annotation_level = severenty_map.get(message.get("level"), "error") + title = message.get("message", "") + rendered_message = message.get("rendered", "") + + self.emit_annotation( + title, + rendered_message, + primary_span["file_name"], + primary_span["line_start"], + primary_span["line_end"], + annotation_level, + primary_span["column_start"], + primary_span["column_end"], + ) diff --git a/python/tidy/tidy.py b/python/tidy/tidy.py index 0e31bcc2bf1..7d2c8529f35 100644 --- a/python/tidy/tidy.py +++ b/python/tidy/tidy.py @@ -21,10 +21,10 @@ from typing import Any, Dict, List import colorama import toml - import wpt.manifestupdate -from .licenseck import OLD_MPL, MPL, APACHE, COPYRIGHT, licenses_toml +from .licenseck import APACHE, COPYRIGHT, MPL, OLD_MPL, licenses_toml +from .linting_report import GitHubAnnotationManager TOPDIR = os.path.abspath(os.path.dirname(sys.argv[0])) WPT_PATH = os.path.join(".", "tests", "wpt") @@ -982,7 +982,8 @@ def collect_errors_for_files(files_to_check, checking_functions, line_checking_f yield (filename,) + error -def scan(only_changed_files=False, progress=False): +def scan(only_changed_files=False, progress=False, github_annotations=False): + github_annotation_manager = GitHubAnnotationManager("test-tidy") # check config file for errors config_errors = check_config_file(CONFIG_FILE_PATH) # check directories contain expected files @@ -1018,6 +1019,9 @@ def scan(only_changed_files=False, progress=False): + f"{colorama.Fore.RED}{error[2]}{colorama.Style.RESET_ALL}" ) + if github_annotations: + github_annotation_manager.emit_annotation(error[2], error[2], error[0], error[1]) + return int(error is not None)