From d7f4b064e8219a70ed8184b672b8a495c3e92f9c Mon Sep 17 00:00:00 2001 From: Tom Ritter Date: Thu, 12 Nov 2026 12:30:14 +0000 Subject: [PATCH 1/4] Return command output as text instead of decoding it at every call site. Fixes #244 The command runner now asks subprocess for UTF-8 text, replacing any bytes that aren't valid UTF-8. Most call sites decoded strictly, so output in another encoding (like the ISO-8859-1 commit behind #362) raised UnicodeDecodeError wherever it was read. --- apis/phabricator.py | 8 ++++---- apis/taskcluster.py | 4 ++-- automation.py | 2 +- components/aiprovider.py | 2 +- components/commandprovider.py | 2 +- components/commandrunner.py | 15 +++++++++------ components/hg.py | 6 +++--- components/libraryprovider.py | 2 +- components/logging.py | 4 ++-- components/mach_vendor.py | 10 +++++----- components/scmprovider.py | 18 +++++++++--------- tasktypes/vendoring.py | 4 ++-- tests/aiprovider.py | 2 +- tests/build_bug_description.py | 2 +- tests/mach_vendor.py | 4 ++-- tests/mock_commandprovider.py | 4 ++-- tests/run_command.py | 22 ++++++++++++++++++---- tests/scm.py | 2 +- 18 files changed, 65 insertions(+), 48 deletions(-) diff --git a/apis/phabricator.py b/apis/phabricator.py index 0f08d3a6..69e81823 100644 --- a/apis/phabricator.py +++ b/apis/phabricator.py @@ -47,9 +47,9 @@ def _call_conduit(self, method, params): cmd += " | arc call-conduit --conduit-uri=%s %s --" % (self.url, method) ret = self.run(cmd, shell=True) try: - result = json.loads(ret.stdout.decode()) + result = json.loads(ret.stdout) except Exception: - raise Exception("Could not decode response as JSON: %s" % ret.stdout.decode()) + raise Exception("Could not decode response as JSON: %s" % ret.stdout) if result['error']: raise ConduitError(method, result) return result['response'] @@ -90,7 +90,7 @@ def submit_to_phabricator(rev_id, retry_attempt=None): cmd.append("--") ret = self.run(cmd, clean_return=False) - too_large = ARC_DIFF_TOO_LARGE.search(ret.stdout.decode()) + too_large = ARC_DIFF_TOO_LARGE.search(ret.stdout) if too_large: raise DiffTooLargeError( "arc diff will not submit %s because its diff is %s bytes, over arc's limit of 4 MiB for a single file." @@ -100,7 +100,7 @@ def submit_to_phabricator(rev_id, retry_attempt=None): else: ret.check_returncode() - output = ret.stdout.decode() + output = ret.stdout phab_revision = False r = re.compile(self.url + "D([0-9]+)") diff --git a/apis/taskcluster.py b/apis/taskcluster.py index a69786bd..f38b7894 100644 --- a/apis/taskcluster.py +++ b/apis/taskcluster.py @@ -177,9 +177,9 @@ def submit_to_try(self, library, recursed=0): try_arguments.append("--artifact") ret = self.run(try_arguments, clean_return=False) - output = ret.stdout.decode() + output = ret.stdout if ret.returncode: - stderr = ret.stderr.decode() if getattr(ret, "stderr", None) else "" + stderr = ret.stderr if getattr(ret, "stderr", None) else "" closed = TREE_CLOSED.search(output + stderr) if closed: raise TryTreeClosedError(closed.group(1).strip(), shlex.join(try_arguments)) diff --git a/automation.py b/automation.py index f80d4f04..973e9f44 100755 --- a/automation.py +++ b/automation.py @@ -202,7 +202,7 @@ def _validate(self, config_dictionary): def run(self, library_filter=""): try: start_time = time.time() - updatebot_version = self.cmdProvider.run(["git", "log", "-1", "--oneline"], shell=False, clean_return=True).stdout.decode().strip() + updatebot_version = self.cmdProvider.run(["git", "log", "-1", "--oneline"], shell=False, clean_return=True).stdout.strip() python_version = sys.version.replace("\n", " ") self.logger.log("Running Updatebot version: {0} on Python {1}".format(updatebot_version, python_version), level=LogLevel.Info) self.logger.log("Python prefix: {0}".format(sys.prefix), level=LogLevel.Debug) diff --git a/components/aiprovider.py b/components/aiprovider.py index 3cd68635..d34f094e 100644 --- a/components/aiprovider.py +++ b/components/aiprovider.py @@ -112,7 +112,7 @@ def _write(suffix, contents): self.logger.log("Could not write AI debug output: %s" % e, level=LogLevel.Warning) def _parse_result(self, ret): - stdout = ret.stdout.decode() if isinstance(ret.stdout, bytes) else ret.stdout + stdout = ret.stdout try: data = json.loads(stdout) except ValueError: diff --git a/components/commandprovider.py b/components/commandprovider.py index a9d12171..f05d16cb 100644 --- a/components/commandprovider.py +++ b/components/commandprovider.py @@ -35,7 +35,7 @@ def run(self, args, shell=False, clean_return=True, cwd=None, stdin_path=None, t return ret def _report_traceback_in_stderr(self, args, stderr): - stderr = stderr.decode(errors="replace") if stderr else "" + stderr = stderr or "" if "Traceback (most recent call last):" not in stderr: return lines = [line for line in stderr.strip().splitlines() if line.strip()] diff --git a/components/commandrunner.py b/components/commandrunner.py index d0e9c26a..96d26538 100644 --- a/components/commandrunner.py +++ b/components/commandrunner.py @@ -45,19 +45,22 @@ def _run(args, shell, clean_return, errorlog=do_nothing, infolog=do_nothing, deb # Merge any extra env (e.g. secrets) over the inherited environment. run_env = {**os.environ, **env} if env else None try: + # Output that isn't valid UTF-8 (e.g. a commit stored in another encoding) + # has the bad bytes replaced, rather than making callers decode it. ret = subprocess.run( args, shell=shell, stdout=PIPE, stderr=PIPE, timeout=timeout, - cwd=cwd, stdin=stdin_handle or subprocess.DEVNULL, env=run_env) + cwd=cwd, stdin=stdin_handle or subprocess.DEVNULL, env=run_env, + encoding="utf-8", errors="replace") except subprocess.TimeoutExpired as e: ran_to_completion = False - stdout = e.stdout - stderr = e.stderr + # TimeoutExpired carries bytes even when the output is text + stdout = e.stdout.decode(errors="replace") if isinstance(e.stdout, bytes) else e.stdout + stderr = e.stderr.decode(errors="replace") if isinstance(e.stderr, bytes) else e.stderr exception = e else: ran_to_completion = True - # Only for the log: callers get the raw output, which may not be UTF-8 - stdout = ret.stdout.decode(errors="replace") - stderr = ret.stderr.decode(errors="replace") + stdout = ret.stdout + stderr = ret.stderr finally: if stdin_handle: stdin_handle.close() diff --git a/components/hg.py b/components/hg.py index 20826f12..94e73e89 100644 --- a/components/hg.py +++ b/components/hg.py @@ -26,7 +26,7 @@ def reset_repository(cmdrunner): else: ret = cmdrunner.run(["hg", "strip", "roots(outgoing())", "--no-backup"], clean_return=False) if ret.returncode == 255: - if "abort: empty revision set" not in ret.stderr.decode(): + if "abort: empty revision set" not in ret.stderr: ret.check_returncode() @@ -76,10 +76,10 @@ def changed_files(self, exclude=()): # deleted files, with paths relative to the root of the checkout. self.run(["hg", "addremove", "-q"]) excluded = set(os.path.normpath(p) for p in exclude) - files = self.run(["hg", "--config", "ui.relative-paths=no", "status", "--no-status"]).stdout.decode().splitlines() + files = self.run(["hg", "--config", "ui.relative-paths=no", "status", "--no-status"]).stdout.splitlines() return [f for f in files if f and os.path.normpath(f) not in excluded] @logEntryExit def diff_stats(self): ret = self.run(["hg", "diff", "--stat"]) - return ret.stdout.decode().rstrip() + return ret.stdout.rstrip() diff --git a/components/libraryprovider.py b/components/libraryprovider.py index 4516181c..84eb4e0c 100644 --- a/components/libraryprovider.py +++ b/components/libraryprovider.py @@ -176,7 +176,7 @@ def get_libraries(self, gecko_path): if self._libraries is None: libraries = [] command = ["find", gecko_path, "-type", "f", "-name", "moz.yaml"] - mozilla_central_yamls = self.run(command).stdout.decode() + mozilla_central_yamls = self.run(command).stdout mozilla_central_yamls = mozilla_central_yamls.strip().split("\n") for file in mozilla_central_yamls: diff --git a/components/logging.py b/components/logging.py index 795746a4..5e91e58d 100644 --- a/components/logging.py +++ b/components/logging.py @@ -223,13 +223,13 @@ def __init__(self, config): def _update_config(self, additional_config): version = "updatebot-" - version += _run(["git", "rev-parse", "HEAD"], shell=False, clean_return=True).stdout.decode().strip() + version += _run(["git", "rev-parse", "HEAD"], shell=False, clean_return=True).stdout.strip() environment = "" if "TASK_ID" in os.environ: environment = "taskcluster" else: - environment = _run(["hostname"], shell=False, clean_return=True).stdout.decode().strip() + environment = _run(["hostname"], shell=False, clean_return=True).stdout.strip() sentry_init( dsn=self.config['sentry_config']['url'], diff --git a/components/mach_vendor.py b/components/mach_vendor.py index 41ed17d9..5fd5e5d2 100644 --- a/components/mach_vendor.py +++ b/components/mach_vendor.py @@ -23,7 +23,7 @@ def __init__(self, config): @logEntryExit @retry def check_for_update(self, library): - result = self.run(["./mach", "vendor", "--check-for-update", library.yaml_path]).stdout.decode().strip() + result = self.run(["./mach", "vendor", "--check-for-update", library.yaml_path]).stdout.strip() if "Creating " in result and " state directory" in result: # If no ~/.mozbuild directory was present this gets output unfortunately. @@ -51,8 +51,8 @@ def vendor(self, library, revision): if ret.returncode == 0: return (VendorResult.SUCCESS, "") - msg = ret.stderr.decode().rstrip() + "\n\n" if ret.stderr else "" - msg += ret.stdout.decode().rstrip() + msg = ret.stderr.rstrip() + "\n\n" if ret.stderr else "" + msg += (ret.stdout or "").rstrip() if ret.returncode == 255: return (VendorResult.MOZBUILD_ERROR, msg) @@ -62,8 +62,8 @@ def vendor(self, library, revision): return (VendorResult.GENERAL_ERROR, msg) except Exception as e: if isinstance(e, subprocess.CalledProcessError): - msg = e.stderr.decode().rstrip() + "\n\n" if e.stderr else "" - msg += e.stdout.decode().rstrip() + msg = e.stderr.rstrip() + "\n\n" if e.stderr else "" + msg += (e.stdout or "").rstrip() else: msg = str(e) return (VendorResult.GENERAL_ERROR, msg) diff --git a/components/scmprovider.py b/components/scmprovider.py index 59639c18..182c9ace 100644 --- a/components/scmprovider.py +++ b/components/scmprovider.py @@ -66,9 +66,9 @@ def populate_details(self, repo, run): return rev_range = [self.revision + "^", self.revision] - self.parent_revision = run(["git", "rev-parse", self.revision + "^"]).stdout.decode().strip() + self.parent_revision = run(["git", "rev-parse", self.revision + "^"]).stdout.strip() - files_changed = run(["git", "diff", "--name-status"] + rev_range).stdout.decode().split("\n") + files_changed = run(["git", "diff", "--name-status"] + rev_range).stdout.split("\n") for f in files_changed: f = f.strip() if not f: @@ -84,14 +84,14 @@ def populate_details(self, repo, run): else: self.files_other.append(parts[0] + " " + parts[1]) - self.summary = run(["git", "log", "--pretty=%s", "-1", self.revision]).stdout.decode() - self.author = run(["git", "log", "--pretty=%an <%ae>", "-1", self.revision]).stdout.decode() - self.description = run(["git", "log", "--pretty=%b", "-1", self.revision]).stdout.decode() + self.summary = run(["git", "log", "--pretty=%s", "-1", self.revision]).stdout + self.author = run(["git", "log", "--pretty=%an <%ae>", "-1", self.revision]).stdout + self.description = run(["git", "log", "--pretty=%b", "-1", self.revision]).stdout # A signed commit (with GPG or SSH) has a gpgsig header. We can't verify the # signature without the signer's key, only tell whether there is one. # cat-file prints the commit as stored, which may not be UTF-8 (unlike git log, # which converts it), but the header names we look for are ASCII. - headers = run(["git", "cat-file", "commit", self.revision]).stdout.decode("utf-8", "replace").split("\n\n", 1)[0] + headers = run(["git", "cat-file", "commit", self.revision]).stdout.split("\n\n", 1)[0] self.signed = any(line.startswith(("gpgsig ", "gpgsig-sha256 ")) for line in headers.split("\n")) self.revision_link = repo_and_commit_to_url(repo, self.revision) self.populated = True @@ -241,7 +241,7 @@ def check_for_update(self, library, task, new_version, most_recent_job_version): self._ensure_checkout(library.repo_url, task.branch, library.name) # Step 1: Find the common ancestor between the commit we are and new_version - common_ancestor = self.run(["git", "merge-base", library.revision, new_version]).stdout.decode().strip() + common_ancestor = self.run(["git", "merge-base", library.revision, new_version]).stdout.strip() self.logger.log("Our common ancestor is %s." % (common_ancestor), level=LogLevel.Debug) # Step 2: Get the list of commits between the common ancestor and new_version @@ -333,7 +333,7 @@ def _is_ancestor(self, ancestor, descendant): if ret.returncode not in (0, 1): # e.g. a revision that doesn't exist upstream (any more) self.logger.log("Could not determine whether %s is an ancestor of %s: %s" % ( - ancestor, descendant, ret.stderr.decode().strip()), level=LogLevel.Warning) + ancestor, descendant, ret.stderr.strip()), level=LogLevel.Warning) return ret.returncode == 0 def _commits_between(self, revision1, revision2): @@ -342,7 +342,7 @@ def _commits_between(self, revision1, revision2): If rev1 == rev2, returns an empty list. """ ret = self.run(["git", "log", "--pretty=%H|%ai|%ci", "--no-merges", "%s..%s" % (revision1, revision2)]) - commits = [line.strip() for line in ret.stdout.decode().split("\n")] + commits = [line.strip() for line in ret.stdout.split("\n")] # Put them in order of oldest to newest commits.reverse() # Populate them into a class but don't get details just yet. diff --git a/tasktypes/vendoring.py b/tasktypes/vendoring.py index f609c651..56ebd4c0 100644 --- a/tasktypes/vendoring.py +++ b/tasktypes/vendoring.py @@ -21,8 +21,8 @@ def _patch_exception_message(e): if isinstance(e, subprocess.CalledProcessError): - msg = ("stderr:\n" + e.stderr.decode().rstrip() + "\n\n") if e.stderr else "" - msg += ("stdout:\n" + e.stdout.decode().rstrip()) if e.stdout else "" + msg = ("stderr:\n" + e.stderr.rstrip() + "\n\n") if e.stderr else "" + msg += ("stdout:\n" + e.stdout.rstrip()) if e.stdout else "" return msg return str(e) diff --git a/tests/aiprovider.py b/tests/aiprovider.py index 0ef3d40f..04efbeca 100755 --- a/tests/aiprovider.py +++ b/tests/aiprovider.py @@ -46,7 +46,7 @@ def run(self, args, shell=False, clean_return=True, cwd=None, stdin_path=None, t if self.result_file_contents is not None and cwd: with open(os.path.join(cwd, CONFLICT_RESOLUTION_RESULT_FILE), "w") as f: f.write(self.result_file_contents) - return Struct(**{"stdout": self.stdout.encode(), + return Struct(**{"stdout": self.stdout, "returncode": self.returncode, "check_returncode": lambda: None}) diff --git a/tests/build_bug_description.py b/tests/build_bug_description.py index f34a1a68..68f652d4 100644 --- a/tests/build_bug_description.py +++ b/tests/build_bug_description.py @@ -149,7 +149,7 @@ def cat_file(headers): outputs = {"cat-file": headers + "\n\nThe message\n", "rev-parse": "p0", "diff": "", "log": ""} def run(cmd): - return Struct(stdout=next(v for k, v in outputs.items() if k in cmd).encode()) + return Struct(stdout=next(v for k, v in outputs.items() if k in cmd)) c = Commit("abc1234|2020-01-01 00:00:00 +0000|2020-01-02 00:00:00 +0000") c.populate_details(GITHUB_REPO, run) return c.signed diff --git a/tests/mach_vendor.py b/tests/mach_vendor.py index 01ec7e01..d0029bb4 100644 --- a/tests/mach_vendor.py +++ b/tests/mach_vendor.py @@ -66,7 +66,7 @@ def _patch(self, returncode, **kwargs): def run(cmd, clean_return=True, **run_kwargs): calls.append(clean_return) - return subprocess.CompletedProcess(cmd, returncode, stdout=b"Could not apply foo.patch", stderr=b"") + return subprocess.CompletedProcess(cmd, returncode, stdout="Could not apply foo.patch", stderr="") vendorProvider = VendorProvider({}) vendorProvider.update_config(dict(SimpleLoggerConfig, CommandProvider=Struct(run=run))) @@ -87,7 +87,7 @@ def testPatchFailureCanBeQuiet(self): (calls, e) = self._patch(1, ignore_modified=True, log_failure=False) self.assertEqual(calls, [False]) self.assertIsInstance(e, subprocess.CalledProcessError) - self.assertEqual(e.output, b"Could not apply foo.patch") + self.assertEqual(e.output, "Could not apply foo.patch") if __name__ == "__main__": diff --git a/tests/mock_commandprovider.py b/tests/mock_commandprovider.py index e646b818..8c05339e 100644 --- a/tests/mock_commandprovider.py +++ b/tests/mock_commandprovider.py @@ -77,7 +77,7 @@ def check_returncode(): if returncode != 0: raise subprocess.CalledProcessError(returncode, argument_string) pass - return Struct(**{'stdout': - Struct(**{'decode': lambda *args, **kwargs: stdout}), + return Struct(**{'stdout': stdout, + 'stderr': "", 'returncode': returncode, 'check_returncode': check_returncode}) diff --git a/tests/run_command.py b/tests/run_command.py index e8619fc0..0ac962d0 100755 --- a/tests/run_command.py +++ b/tests/run_command.py @@ -5,6 +5,7 @@ # file, You can obtain one at http://mozilla.org/MPL/2.0/. import os +import subprocess import sys import tempfile import unittest @@ -30,13 +31,13 @@ def testEnvIsMergedOverEnviron(self): ret = self._runner().run(["bash", "-c", "echo $UPDATEBOT_TEST_VAR"], env={"UPDATEBOT_TEST_VAR": "hello"}) self.assertEqual(ret.returncode, 0) - self.assertEqual(ret.stdout.decode().strip(), "hello") + self.assertEqual(ret.stdout.strip(), "hello") def testCwd(self): with tempfile.TemporaryDirectory() as d: ret = self._runner().run(["pwd"], cwd=d) self.assertEqual(ret.returncode, 0) - self.assertEqual(os.path.realpath(ret.stdout.decode().strip()), os.path.realpath(d)) + self.assertEqual(os.path.realpath(ret.stdout.strip()), os.path.realpath(d)) def testStdinPath(self): # The contents of stdin_path are fed to the process's standard input. @@ -46,7 +47,7 @@ def testStdinPath(self): try: ret = self._runner().run(["cat"], stdin_path=stdin_path) self.assertEqual(ret.returncode, 0) - self.assertEqual(ret.stdout.decode(), "piped-in-content") + self.assertEqual(ret.stdout, "piped-in-content") finally: os.remove(stdin_path) @@ -93,7 +94,20 @@ def testNoStdinByDefault(self): os.dup2(saved_stdin, 0) os.close(saved_stdin) os.close(r) - self.assertEqual(ret.stdout.decode().strip(), "False ''") + self.assertEqual(ret.stdout.strip(), "False ''") + + def testOutputIsText(self): + ret = self._runner().run(["python3", "-c", "import sys; sys.stdout.write('caf\\u00e9'); sys.stderr.write('err')"]) + self.assertEqual(ret.stdout, "café") + self.assertEqual(ret.stderr, "err") + + def testInvalidUTF8IsReplaced(self): + ret = self._runner().run(["python3", "-c", "import sys; sys.stdout.buffer.write(b'caf\\xe9')"]) + self.assertEqual(ret.stdout, "caf�") + + def testTimeout(self): + with self.assertRaises(subprocess.TimeoutExpired): + self._runner().run(["python3", "-c", "import time; print('started', flush=True); time.sleep(30)"], timeout=1) if __name__ == '__main__': diff --git a/tests/scm.py b/tests/scm.py index 85420c27..566a2e3a 100755 --- a/tests/scm.py +++ b/tests/scm.py @@ -304,7 +304,7 @@ def run(args, **kwargs): if len(attempts) == 1: # Fail part way through, leaving a partial checkout behind os.makedirs(".git/objects") - raise subprocess.CalledProcessError(128, args, stderr=b"error: RPC failed; HTTP 502\nfatal: the remote end hung up unexpectedly") + raise subprocess.CalledProcessError(128, args, stderr="error: RPC failed; HTTP 502\nfatal: the remote end hung up unexpectedly") open("README", "w").close() scm.run = run From 1d864e39a033f971593951f831a72ea23a24ba58 Mon Sep 17 00:00:00 2001 From: Tom Ritter Date: Thu, 12 Nov 2026 12:33:40 +0000 Subject: [PATCH 2/4] Log hg status when ./mach try refuses to push because of uncommitted changes. Fixes #392 Our commits succeed before we push, so this means something was changed or deleted without being registered with hg, which is how the WebGPU CTS pushes in #392 failed (fixed by Bug 1971136's vcs-add-remove-files action). If it happens again, the log will show which files. --- apis/taskcluster.py | 8 ++++++++ tests/treeherder_api.py | 30 +++++++++++++++++++++++++++++- 2 files changed, 37 insertions(+), 1 deletion(-) diff --git a/apis/taskcluster.py b/apis/taskcluster.py index f38b7894..1e79bed1 100644 --- a/apis/taskcluster.py +++ b/apis/taskcluster.py @@ -21,6 +21,9 @@ # hghooks/mozhghooks/treeclosure.py). TREE_CLOSED = re.compile(r"\S+ is CLOSED! Reason: (.*)") TREE_STATUS_UNAVAILABLE = "Unable to check if the tree is open - treating as if CLOSED." +# mach try refuses to push with uncommitted changes ("ERROR please commit changes before +# continuing" in older versions, "error: commit changes before continuing" now) +UNCOMMITTED_CHANGES = "commit changes before continuing" class TryTreeClosedError(Exception): @@ -185,6 +188,11 @@ def submit_to_try(self, library, recursed=0): raise TryTreeClosedError(closed.group(1).strip(), shlex.join(try_arguments)) if TREE_STATUS_UNAVAILABLE in output + stderr: raise TryTreeClosedError("its tree status couldn't be checked, so the push was refused as if it were closed", shlex.join(try_arguments)) + if UNCOMMITTED_CHANGES in output + stderr: + # Our commits succeeded, so something was changed or deleted without being + # registered with hg (#392). Record what, so the cause can be found. + status = self.run(["hg", "status"], clean_return=False).stdout + self.logger.log("./mach try refused to push because of uncommitted changes. hg status shows:\n%s" % status, level=LogLevel.Error) ret.check_returncode() isNext = False diff --git a/tests/treeherder_api.py b/tests/treeherder_api.py index 41fe22dc..fe557241 100755 --- a/tests/treeherder_api.py +++ b/tests/treeherder_api.py @@ -6,6 +6,7 @@ import sys import json +import subprocess import unittest import requests @@ -14,7 +15,7 @@ sys.path.append(".") sys.path.append("..") -from components.logging import SimpleLoggerConfig +from components.logging import SimpleLoggerConfig, LogLevel from components.utilities import static_vars, Struct import components.utilities from apis.taskcluster import TaskclusterProvider @@ -173,6 +174,33 @@ def test_bug_suggestions_http_error(self): components.utilities.RETRY_TIMES_OVERRIDE, components.utilities.time.sleep = original_override, original_sleep self.assertEqual(treeherder_responses.bug_suggestions_calls - calls, 3, "Expected the request to be retried") + def test_submit_to_try_logs_uncommitted_changes(self): + commands, errors = [], [] + + def mach_try_failed(): + raise subprocess.CalledProcessError(1, "./mach try") + + def run(args, clean_return=True, **kwargs): + commands.append(args) + if args[:2] == ["./mach", "try"]: + return Struct(returncode=1, stdout="error: commit changes before continuing\n", stderr="", check_returncode=mach_try_failed) + return Struct(returncode=0, stdout="! testing/web-platform/mozilla/tests/webgpu/removed.html\n", stderr="") + provider = TaskclusterProvider({'url_treeherder': 'http://localhost:27490/', 'url_taskcluster': 'http://localhost:27490/'}) + provider.update_config(dict(SimpleLoggerConfig, CommandProvider=Struct(run=run))) + original_log = provider.logger.log + provider.logger.log = lambda *args, level=LogLevel.Info, **kwargs: (errors.append(args[0]) if level == LogLevel.Error else None, original_log(*args, level=level, **kwargs)) + original_override, original_sleep = components.utilities.RETRY_TIMES_OVERRIDE, components.utilities.time.sleep + components.utilities.RETRY_TIMES_OVERRIDE = 1 + components.utilities.time.sleep = lambda s: None + try: + with self.assertRaises(subprocess.CalledProcessError): + provider.submit_to_try(Struct(try_preset="webgpu", fuzzy_query=None, fuzzy_paths=None, artifact_build=False)) + finally: + components.utilities.RETRY_TIMES_OVERRIDE, components.utilities.time.sleep = original_override, original_sleep + self.assertIn(["hg", "status"], commands) + self.assertEqual(len([e for e in errors if "uncommitted changes" in e]), 1) + self.assertIn("removed.html", [e for e in errors if "uncommitted changes" in e][0]) + def test_retrigger_was_automated(self): # Updatebot's retrigger from a real try push, triggered with its cron task's credentials updatebot = Struct(job_type_name="Action: Retrigger", task_id="Y9uB-4roQemM36ti5Apxgw") From 71c577898dc84cd3867918e044a955750e241835 Mon Sep 17 00:00:00 2001 From: Tom Ritter Date: Thu, 12 Nov 2026 12:33:40 +0000 Subject: [PATCH 3/4] Point out license file changes at the top of the bug. Fixes #190 If an update changes moz.yaml's license-file, or a file under the vendor directory named like a license (LICENSE*, COPYING*, NOTICE*, ...), the bug description starts by listing them and asking for the license in moz.yaml to be checked. This happened in 3 of Updatebot's 439 commits in the past year, so it should rarely fire. It's a note rather than a failure: stopping the update would only leave us on older code. --- components/bugzilla.py | 8 ++++++ components/hg.py | 8 ++++++ components/libraryprovider.py | 6 ++++ tasktypes/vendoring.py | 20 +++++++++++++- test.py | 1 + tests/library.py | 24 ++++++++++++++-- tests/license_files.py | 52 +++++++++++++++++++++++++++++++++++ 7 files changed, 116 insertions(+), 3 deletions(-) create mode 100644 tests/license_files.py diff --git a/components/bugzilla.py b/components/bugzilla.py index ffa186d8..2b7dcdaf 100644 --- a/components/bugzilla.py +++ b/components/bugzilla.py @@ -32,6 +32,14 @@ def UPDATE_DETAILS_WITHOUT_COMMITS(library, commit_stats, error): %s """ % (library.repo_url, error.strip().replace("\n", "\n> "), commit_stats) + @staticmethod + def LICENSE_FILES_CHANGED(files): + return """ +This update changes license files, so please check that the license in moz.yaml is still correct: + +%s +""" % "\n".join("- " + f for f in files) + @staticmethod def UPDATE_DETAILS(num_commits, num_new_commits, commit_stats, commit_details, author_summary=""): new_commit_str = "" diff --git a/components/hg.py b/components/hg.py index 94e73e89..13b8fee4 100644 --- a/components/hg.py +++ b/components/hg.py @@ -79,6 +79,14 @@ def changed_files(self, exclude=()): files = self.run(["hg", "--config", "ui.relative-paths=no", "status", "--no-status"]).stdout.splitlines() return [f for f in files if f and os.path.normpath(f) not in excluded] + @logEntryExit + def changed_files_in(self, directory): + # Modified, added, removed, missing and untracked files under directory, with + # paths relative to the root of the checkout. Unlike changed_files, this doesn't + # change what's registered with hg. + files = self.run(["hg", "--config", "ui.relative-paths=no", "status", "--no-status", directory]).stdout.splitlines() + return [f for f in files if f] + @logEntryExit def diff_stats(self): ret = self.run(["hg", "diff", "--stat"]) diff --git a/components/libraryprovider.py b/components/libraryprovider.py index 84eb4e0c..91c6dea7 100644 --- a/components/libraryprovider.py +++ b/components/libraryprovider.py @@ -51,6 +51,8 @@ def __init__(self, dict): self.fuzzy_query = dict.get('fuzzy_query', None) self.fuzzy_paths = dict.get('fuzzy_paths', None) self.artifact_build = dict.get('artifact_build', False) + # Relative to the vendor directory, as in moz.yaml + self.license_file = dict.get('license_file') or None self.yaml_path = dict['yaml_path'] # Paths are relative to the root of the gecko checkout, like yaml_path self.vendor_directory = dict.get('vendor_directory') or os.path.dirname(self.yaml_path) @@ -225,6 +227,7 @@ def validate_library(yaml_contents, yaml_path): 'fuzzy_query': '', 'fuzzy_paths': [], 'artifact_build': False, + 'license_file': '', 'tasks': [], 'yaml_path': '', 'vendor_directory': '', @@ -247,6 +250,9 @@ def validate_library(yaml_contents, yaml_path): else: validated_library['revision'] = None + if 'origin' in library and 'license-file' in library['origin']: + validated_library['license_file'] = library['origin']['license-file'] + # Attempt to get the repository url (not required by moz.yaml) if present if 'vendoring' in library and 'url' in library['vendoring']: validated_library['repo_url'] = library['vendoring']['url'] diff --git a/tasktypes/vendoring.py b/tasktypes/vendoring.py index 56ebd4c0..1e102581 100644 --- a/tasktypes/vendoring.py +++ b/tasktypes/vendoring.py @@ -6,6 +6,8 @@ import copy import functools +import os +import re import subprocess from collections import defaultdict @@ -18,6 +20,10 @@ from components.logging import LogLevel, logEntryExit, logEntryExitNoArgs from components.hg import reset_repository, SPURIOUS_CHECK_SHELF +# Upstream's license files are usually named like these (LICENSE.txt, COPYING.LIB, +# LICENSE_FOXIT, ...); moz.yaml's license-file covers any that aren't. +LICENSE_FILE_NAME = re.compile(r"^(LICEN[CS]E|COPYING|COPYRIGHT|NOTICE|UNLICENSE)", re.IGNORECASE) + def _patch_exception_message(e): if isinstance(e, subprocess.CalledProcessError): @@ -96,6 +102,13 @@ def process_task(self, library, task): self._process_new_job(library, task, new_version, timestamp, most_recent_job) self._reset_for_new_job() + # ==================================================================== + @logEntryExit + def _changed_license_files(self, library): + license_file = os.path.normpath(os.path.join(library.vendor_directory, library.license_file)) if library.license_file else None + return [f for f in self.mercurialProvider.changed_files_in(library.vendor_directory) + if LICENSE_FILE_NAME.match(os.path.basename(f)) or os.path.normpath(f) == license_file] + # ==================================================================== @logEntryExit def _only_patches_changed(self, library): @@ -198,11 +211,15 @@ def _process_new_job(self, library, task, new_version, timestamp, most_recent_jo # File the bug ------------------------ commit_stats = self.mercurialProvider.diff_stats() + license_files = self._changed_license_files(library) + if license_files: + self.logger.log("This update changes the license files %s." % ", ".join(license_files), level=LogLevel.Info) + license_note = CommentTemplates.LICENSE_FILES_CHANGED(license_files) if license_files else "" try: all_upstream_commits, unseen_upstream_commits = self.scmProvider.check_for_update(library, task, new_version, most_recent_job.version if most_recent_job else None) if library.should_show_commit_details: author_summary = self.scmProvider.build_author_summary(all_upstream_commits) - commit_chunks = self.scmProvider.build_bug_description(all_upstream_commits, 65534 - len(commit_stats) - len(author_summary) - 220, library.repo_url, options=task.options, new_commits=unseen_upstream_commits, compare_range=(library.revision, new_version)) + commit_chunks = self.scmProvider.build_bug_description(all_upstream_commits, 65534 - len(commit_stats) - len(author_summary) - len(license_note) - 220, library.repo_url, options=task.options, new_commits=unseen_upstream_commits, compare_range=(library.revision, new_version)) else: author_summary = "" commit_chunks = [""] @@ -214,6 +231,7 @@ def _process_new_job(self, library, task, new_version, timestamp, most_recent_jo self.logger.log("Could not get the upstream commits for %s; filing the bug without them: %s" % (library.name, e), level=LogLevel.Warning) commit_chunks = [""] bug_description = CommentTemplates.UPDATE_DETAILS_WITHOUT_COMMITS(library, commit_stats, _patch_exception_message(e)) + bug_description = license_note + bug_description created_job.bugzilla_id = self.bugzillaProvider.file_bug(library, CommentTemplates.UPDATE_SUMMARY(library, new_version, timestamp), bug_description, task.cc, blocks=task.blocking) self.dbProvider.update_job_add_bug_id(created_job, created_job.bugzilla_id) diff --git a/test.py b/test.py index f26ac3ca..4f05dd44 100755 --- a/test.py +++ b/test.py @@ -25,6 +25,7 @@ "class_passing", "frequency", "mach_vendor", + "license_files", "scm", "sentry_logging" ] diff --git a/tests/library.py b/tests/library.py index 623bc953..050021ba 100755 --- a/tests/library.py +++ b/tests/library.py @@ -99,7 +99,8 @@ }, "n/a") ], "yaml_path": ".github/gecko-test/libdav1d/moz.yaml", - "vendor_directory": "third_party/dav1d" + "vendor_directory": "third_party/dav1d", + "license_file": "COPYING" }), Library({ 'name': 'libpng', @@ -118,7 +119,8 @@ 'enabled': True, }, "n/a") ], - 'yaml_path': '.github/gecko-test/libpng/moz.yaml' + 'yaml_path': '.github/gecko-test/libpng/moz.yaml', + 'license_file': 'LICENSE' }) ] @@ -240,6 +242,24 @@ def artifact_build(updatebot_extra): self.assertIs(artifact_build(" artifact-build: false"), False) self.assertIs(artifact_build(""), False) + def testLicenseFile(self): + def license_file(origin_extra): + return LibraryProvider.validate_library(""" +schema: 1 +bugzilla: + product: Core + component: "Audio/Video: Playback" +origin: + name: libfoo +%s +updatebot: + maintainer-bz: foo + maintainer-phab: bar +""" % origin_extra, "fake/path").license_file + + self.assertEqual(license_file(" license-file: docs/FTL.TXT"), "docs/FTL.TXT") + self.assertIsNone(license_file("")) + def testOnlyLinuxTasksRun(self): def task_types(tasks): return [t.type for t in LibraryProvider.validate_library(""" diff --git a/tests/license_files.py b/tests/license_files.py new file mode 100644 index 00000000..5e34a427 --- /dev/null +++ b/tests/license_files.py @@ -0,0 +1,52 @@ +#!/usr/bin/env python3 + +# This Source Code Form is subject to the terms of the Mozilla Public +# License, v. 2.0. If a copy of the MPL was not distributed with this +# file, You can obtain one at http://mozilla.org/MPL/2.0/. + +import sys +import unittest + +sys.path.append(".") +sys.path.append("..") +from components.bugzilla import CommentTemplates +from components.logging import SimpleLogger +from components.utilities import Struct +from tasktypes.vendoring import VendorTaskRunner + + +class TestChangedLicenseFiles(unittest.TestCase): + def _changed_license_files(self, changed, license_file=None): + runner = VendorTaskRunner.__new__(VendorTaskRunner) + runner.logger = SimpleLogger() + runner.mercurialProvider = Struct(changed_files_in=lambda directory: changed) + return runner._changed_license_files(Struct(vendor_directory="third_party/foo", license_file=license_file)) + + def testConventionalNames(self): + changed = [ + "third_party/foo/src/foo.c", + "third_party/foo/LICENSE", + "third_party/foo/COPYING.LIB", + "third_party/foo/fonts/LICENSE_FOXIT", + "third_party/foo/third_party/nalloc/license.txt", + "third_party/foo/NOTICE", + "third_party/foo/COPYRIGHT", + "third_party/foo/docs/licensing.md", + ] + self.assertEqual(self._changed_license_files(changed), changed[1:7]) + + def testMozYamlLicenseFile(self): + changed = ["third_party/foo/docs/FTL.TXT", "third_party/foo/docs/other.txt"] + self.assertEqual(self._changed_license_files(changed, license_file="docs/FTL.TXT"), ["third_party/foo/docs/FTL.TXT"]) + + def testNoLicenseChanges(self): + self.assertEqual(self._changed_license_files(["third_party/foo/src/foo.c"]), []) + + def testNote(self): + note = CommentTemplates.LICENSE_FILES_CHANGED(["third_party/foo/LICENSE", "third_party/foo/COPYING"]) + self.assertIn("check that the license in moz.yaml is still correct", note) + self.assertIn("- third_party/foo/LICENSE\n- third_party/foo/COPYING", note) + + +if __name__ == '__main__': + unittest.main(verbosity=0) From 5dab4a1b990eeb77d8b0cd93e19f8eff831a0d78 Mon Sep 17 00:00:00 2001 From: Tom Ritter Date: Thu, 12 Nov 2026 12:33:40 +0000 Subject: [PATCH 4/4] Test that the mock providers match the classes they mock. Fixes #104 Every method a mock defines must exist on the real class with the same signature, so a change to a provider's interface can't leave tests passing against the old one. --- test.py | 1 + tests/mock_interfaces.py | 43 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 44 insertions(+) create mode 100644 tests/mock_interfaces.py diff --git a/test.py b/test.py index 4f05dd44..43f2d742 100755 --- a/test.py +++ b/test.py @@ -26,6 +26,7 @@ "frequency", "mach_vendor", "license_files", + "mock_interfaces", "scm", "sentry_logging" ] diff --git a/tests/mock_interfaces.py b/tests/mock_interfaces.py new file mode 100644 index 00000000..d313dfe6 --- /dev/null +++ b/tests/mock_interfaces.py @@ -0,0 +1,43 @@ +#!/usr/bin/env python3 + +# This Source Code Form is subject to the terms of the Mozilla Public +# License, v. 2.0. If a copy of the MPL was not distributed with this +# file, You can obtain one at http://mozilla.org/MPL/2.0/. + +import sys +import inspect +import unittest + +sys.path.append(".") +sys.path.append("..") +from components.aiprovider import AIProvider +from components.commandprovider import CommandProvider +from components.libraryprovider import LibraryProvider +from tests.mock_aiprovider import MockAIProvider +from tests.mock_commandprovider import TestCommandProvider +from tests.mock_libraryprovider import MockLibraryProvider + +# Each mock and the class it stands in for +MOCKS = [ + (MockAIProvider, AIProvider), + (TestCommandProvider, CommandProvider), + (MockLibraryProvider, LibraryProvider), +] + + +class TestMockInterfaces(unittest.TestCase): + def testMocksMatchTheClassesTheyMock(self): + # A mock whose methods drift from the real class lets tests pass against an + # interface the code no longer has (#104). + for mock, real in MOCKS: + for name, method in vars(mock).items(): + if name.startswith("__") or not callable(method): + continue + with self.subTest(mock=mock.__name__, method=name): + self.assertTrue(hasattr(real, name), "%s.%s doesn't exist on %s" % (mock.__name__, name, real.__name__)) + self.assertEqual(inspect.signature(method), inspect.signature(getattr(real, name)), + "%s.%s has a different signature from %s.%s" % (mock.__name__, name, real.__name__, name)) + + +if __name__ == '__main__': + unittest.main(verbosity=0)