Skip to content

Commit ea3bd56

Browse files
committed
parallel-make-check: fix warning-doc wording and escape every workflow command
Addresses review feedback: - The "minutes" header comment described the check backwards (the estimate drifting from the measured time). Reword it to match the code, which warns when the measured time lands more than +/-50% away from the estimate. - Centralize the GitHub workflow-command escaping in gh_escape() and apply it to the ::group:: title in dump() and the ::error:: summary in main(), not just warn(), so a config name or step carrying %, CR or LF cannot corrupt those commands either.
1 parent d907997 commit ea3bd56

1 file changed

Lines changed: 15 additions & 13 deletions

File tree

.github/scripts/parallel-make-check.py

Lines changed: 15 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -19,9 +19,10 @@
1919
# minutes expected duration, from the Minutes column of a previous
2020
# run's summary (default 1.0). Schedule weight only - configs
2121
# run longest-first and --shard balances shards by it; a stale
22-
# value just packs the schedule a little worse, but one that
23-
# drifts past +/-50% of the measured time draws a warning
24-
# (never a failure) so it is easy to spot and update.
22+
# value just packs the schedule a little worse, but a run
23+
# whose measured time lands more than +/-50% away from it
24+
# draws a warning (never a failure) so it is easy to spot
25+
# and update.
2526
# user_settings header staged as <builddir>/user_settings.h before
2627
# configure (path relative to the source root); pair it with
2728
# --enable-usersettings in "configure"
@@ -233,8 +234,16 @@ def privatize_dirs(bdir: Path, dirs: list[str]) -> None:
233234
shutil.copytree(SRCDIR / name, d, symlinks=True)
234235

235236

237+
def gh_escape(data: str) -> str:
238+
# Percent-encode workflow-command data (GitHub's documented encoding)
239+
# so a stray %, CR or LF - e.g. from a config name or step out of the
240+
# JSON - can't truncate the command or be parsed as a second one.
241+
return data.replace("%", "%25").replace("\r", "%0D").replace("\n", "%0A")
242+
243+
236244
def dump(title: str, path: Path) -> None:
237-
print(f"::group::{title}" if ON_GITHUB else f"==== {title} ====")
245+
# ::group:: is a workflow command; escape its title like warn() does.
246+
print(f"::group::{gh_escape(title)}" if ON_GITHUB else f"==== {title} ====")
238247
try:
239248
sys.stdout.write(path.read_text(errors="replace"))
240249
except OSError as e:
@@ -247,14 +256,7 @@ def dump(title: str, path: Path) -> None:
247256
def warn(msg: str) -> None:
248257
# GitHub surfaces ::warning:: as an annotation at the top of the run;
249258
# locally it is just a line. Informational only - never fails the run.
250-
if ON_GITHUB:
251-
# Percent-encode the command data (GitHub's documented escaping) so
252-
# a stray %, CR or LF - e.g. from a config name out of the JSON -
253-
# can't truncate the annotation or be read as a second command.
254-
msg = msg.replace("%", "%25").replace("\r", "%0D").replace("\n", "%0A")
255-
print(f"::warning::{msg}")
256-
else:
257-
print(f"WARNING: {msg}")
259+
print(f"::warning::{gh_escape(msg)}" if ON_GITHUB else f"WARNING: {msg}")
258260

259261

260262
def stale_estimate(cfg: Config, minutes: float) -> bool:
@@ -534,7 +536,7 @@ def run_one(cfg: Config) -> tuple[Config, str | None, float]:
534536
else "aborted without a recorded failure"
535537
if aborted:
536538
msg += f" ({aborted} config(s) aborted by fail-fast)"
537-
print(f"::error::{msg}" if ON_GITHUB else msg)
539+
print(f"::error::{gh_escape(msg)}" if ON_GITHUB else msg)
538540
return 1
539541
return 0
540542

0 commit comments

Comments
 (0)