From 15f5a446d3854ccbd69d4f4fcf35a8040907b815 Mon Sep 17 00:00:00 2001 From: Harry Callahan Date: Thu, 16 Jul 2026 18:34:37 +0100 Subject: [PATCH 1/2] fix: Handle repos without an 'origin' remote when generating reports MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Previously, gen_results() would crash with ValueError from GitPython when the project checkout had no remote named 'origin'. This only affected developer clones — CI checkouts always have 'origin' pointing at the canonical upstream — but the crash discarded the entire report after the simulation had already run. The Markdown revision line falls back to a plain commit-sha string with no link if a remote named 'origin' is not present. Signed-off-by: Harry Callahan --- src/dvsim/report/data.py | 5 +++-- src/dvsim/sim/flow.py | 5 +++++ src/dvsim/sim/report.py | 5 ++++- src/dvsim/utils/git.py | 21 ++++++++++++++++----- tests/utils/test_git.py | 14 +++++++++----- 5 files changed, 37 insertions(+), 13 deletions(-) diff --git a/src/dvsim/report/data.py b/src/dvsim/report/data.py index 95ae6b20..39d9dd03 100644 --- a/src/dvsim/report/data.py +++ b/src/dvsim/report/data.py @@ -28,8 +28,9 @@ class IPMeta(BaseModel): """Shortened Git commit sha of the IP the tests are run against.""" branch: str """Git branch""" - url: str - """URL to where the IP can be found in git (e.g. github).""" + url: str | None + """URL to where the IP can be found in git (e.g. github). None if the local + checkout has no ``origin`` remote (typical for developer clones).""" revision_info: str | None """Optional revision info string to use for custom info instead of the above fields.""" diff --git a/src/dvsim/sim/flow.py b/src/dvsim/sim/flow.py index 8c8536b6..81309ac1 100644 --- a/src/dvsim/sim/flow.py +++ b/src/dvsim/sim/flow.py @@ -607,6 +607,11 @@ def gen_results(self, results: Sequence[CompletedJobStatus]) -> None: repo_root = Path(self.proj_root) reports_dir = Path(self.scratch_base_path) / "reports" url = git_https_url_with_commit(path=repo_root) + if url is None: + log.debug( + "no 'origin' remote in %s — report will not include an upstream source link", + repo_root, + ) build_seed = self.build_seed if not self.run_only else None try: diff --git a/src/dvsim/sim/report.py b/src/dvsim/sim/report.py index eba04395..dd05fcf9 100644 --- a/src/dvsim/sim/report.py +++ b/src/dvsim/sim/report.py @@ -243,7 +243,10 @@ def render_metadata( revision = (scope.revision_info or "").strip() if not revision: - revision = f"Github Revision: [`{scope.commit_short}`]({scope.url})" + if scope.url: + revision = f"Github Revision: [`{scope.commit_short}`]({scope.url})" + else: + revision = f"Revision: `{scope.commit_short}`" report_md += f"\n### {revision}" report_md += f"\n### Branch: {scope.branch}" diff --git a/src/dvsim/utils/git.py b/src/dvsim/utils/git.py index 996a17cf..edf716e1 100644 --- a/src/dvsim/utils/git.py +++ b/src/dvsim/utils/git.py @@ -44,8 +44,8 @@ def git_commit_hash(path: Path | None = None, *, short: bool = False) -> str: return r.head.commit.hexsha -def git_origin_url(path: Path | None = None) -> str: - """Get the git remote origin url.""" +def git_origin_url(path: Path | None = None) -> str | None: + """Get the git remote origin url, or None if no ``origin`` remote is configured.""" root = repo_root(path=path or Path.cwd()) if root is None: @@ -54,20 +54,31 @@ def git_origin_url(path: Path | None = None) -> str: r = Repo(root) - return r.remote().url + if "origin" not in [remote.name for remote in r.remotes]: + return None + return r.remote("origin").url -def git_https_url_with_commit(path: Path | None = None) -> str: + +def git_https_url_with_commit(path: Path | None = None) -> str | None: """Get an https url that references the current commit. + The link is derived from the ``origin`` remote, which in CI/regression + workflows points at the canonical upstream repository. In developer + checkouts where ``origin`` is absent or points elsewhere, return ``None`` + rather than guessing — a wrong link is worse than none. + Args: path: the path to the git repo Returns: - str containing the https url + str containing the https url, or None if no ``origin`` remote exists. """ url = git_origin_url(path=path) + if url is None: + return None + commit = git_commit_hash(path=path) url = url.removesuffix(".git") diff --git a/tests/utils/test_git.py b/tests/utils/test_git.py index 30e35689..bab4f423 100644 --- a/tests/utils/test_git.py +++ b/tests/utils/test_git.py @@ -88,11 +88,12 @@ def test_git_origin_url(tmp_path: Path) -> None: r.index.add([file]) r.index.commit("initial commit") - # Value error if called outside a git repo - assert_that( - calling(git_origin_url).with_args(tmp_path), - raises(ValueError), - ) + # None if the repo has no 'origin' remote configured + assert_that(git_origin_url(tmp_path), equal_to(None)) + + # Still None if there are remotes but none named 'origin' + r.create_remote("upstream", "git@github.com:lowRISC/other.git") + assert_that(git_origin_url(tmp_path), equal_to(None)) url = "git@github.com:lowRISC/test.git" r.create_remote("origin", url) @@ -122,6 +123,9 @@ def test_git_https_url_with_commit(tmp_path: Path, url: str, expected: str) -> N r.index.add([file]) r.index.commit("initial commit") + # Returns None when no 'origin' remote is configured. + assert_that(git_https_url_with_commit(tmp_path), equal_to(None)) + r.create_remote("origin", url) commit = r.head.commit.hexsha From e3e7bbc754f31cf3b168163c734ecf7454c7afa6 Mon Sep 17 00:00:00 2001 From: Harry Callahan Date: Thu, 16 Jul 2026 18:35:40 +0100 Subject: [PATCH 2/2] feat: Mark reports (dirty) when the worktree has uncommitted changes The revision URL in a report points at a specific commit sha, but if the working tree had modifications when the simulation ran, that sha misrepresents the code that was actually tested. Append " (dirty)" to the revision line to make this explicit. The dirty state is captured at flow init time (alongside self.commit) so it reflects the checkout when the sim started, not when the report was generated. The suffix is applied on top of both dvsim's own revision formatting and any string supplied via revision_info from downstream HJSON (e.g. OpenTitan's common_project_cfg.hjson), and is idempotent so downstream revision commands can add their own marker without producing a doubled suffix. Signed-off-by: Harry Callahan --- src/dvsim/flow/base.py | 3 ++- src/dvsim/report/data.py | 2 ++ src/dvsim/sim/flow.py | 2 ++ src/dvsim/sim/report.py | 2 ++ src/dvsim/utils/git.py | 11 +++++++++++ tests/utils/test_git.py | 31 ++++++++++++++++++++++++++++++- 6 files changed, 49 insertions(+), 2 deletions(-) diff --git a/src/dvsim/flow/base.py b/src/dvsim/flow/base.py index 4345a31b..497e8a42 100644 --- a/src/dvsim/flow/base.py +++ b/src/dvsim/flow/base.py @@ -32,7 +32,7 @@ rm_path, subst_wildcards, ) -from dvsim.utils.git import git_commit_hash +from dvsim.utils.git import git_commit_hash, git_is_dirty if TYPE_CHECKING: from dvsim.job.deploy import Deploy @@ -173,6 +173,7 @@ def __init__(self, flow_cfg_file, hjson_data, args, mk_config) -> None: proj_root = Path(self.proj_root) self.commit = git_commit_hash(path=proj_root, short=False) self.commit_short = git_commit_hash(path=proj_root, short=True) + self.dirty = git_is_dirty(path=proj_root) # Construct the path variables after variable expansion. reports_dir = Path(self.scratch_base_path) / "reports" diff --git a/src/dvsim/report/data.py b/src/dvsim/report/data.py index 39d9dd03..e385d204 100644 --- a/src/dvsim/report/data.py +++ b/src/dvsim/report/data.py @@ -26,6 +26,8 @@ class IPMeta(BaseModel): """Git commit sha of the IP the tests are run against.""" commit_short: str """Shortened Git commit sha of the IP the tests are run against.""" + dirty: bool = False + """Whether the working tree had uncommitted changes at run time.""" branch: str """Git branch""" url: str | None diff --git a/src/dvsim/sim/flow.py b/src/dvsim/sim/flow.py index 81309ac1..6dc400c4 100644 --- a/src/dvsim/sim/flow.py +++ b/src/dvsim/sim/flow.py @@ -662,6 +662,7 @@ def gen_results(self, results: Sequence[CompletedJobStatus]) -> None: variant=self.variant, commit=self.commit, commit_short=self.commit_short, + dirty=self.dirty, branch=self.branch, url=url, revision_info=self.revision, @@ -710,6 +711,7 @@ def _gen_json_results( variant=(self.variant or "").lower() or None, commit=self.commit, commit_short=self.commit_short, + dirty=self.dirty, branch=self.branch or "", url=url, revision_info=self.revision, diff --git a/src/dvsim/sim/report.py b/src/dvsim/sim/report.py index dd05fcf9..bdda6311 100644 --- a/src/dvsim/sim/report.py +++ b/src/dvsim/sim/report.py @@ -247,6 +247,8 @@ def render_metadata( revision = f"Github Revision: [`{scope.commit_short}`]({scope.url})" else: revision = f"Revision: `{scope.commit_short}`" + if scope.dirty and "(dirty)" not in revision: + revision += " (dirty)" report_md += f"\n### {revision}" report_md += f"\n### Branch: {scope.branch}" diff --git a/src/dvsim/utils/git.py b/src/dvsim/utils/git.py index edf716e1..ce537bdf 100644 --- a/src/dvsim/utils/git.py +++ b/src/dvsim/utils/git.py @@ -44,6 +44,17 @@ def git_commit_hash(path: Path | None = None, *, short: bool = False) -> str: return r.head.commit.hexsha +def git_is_dirty(path: Path | None = None) -> bool: + """Return True if the working tree has uncommitted changes to tracked files.""" + root = repo_root(path=path or Path.cwd()) + + if root is None: + log.error("no git repo found at %s", path) + raise ValueError + + return Repo(root).is_dirty() + + def git_origin_url(path: Path | None = None) -> str | None: """Get the git remote origin url, or None if no ``origin`` remote is configured.""" root = repo_root(path=path or Path.cwd()) diff --git a/tests/utils/test_git.py b/tests/utils/test_git.py index bab4f423..06bde5ce 100644 --- a/tests/utils/test_git.py +++ b/tests/utils/test_git.py @@ -10,7 +10,13 @@ from git import Repo from hamcrest import assert_that, calling, equal_to, raises -from dvsim.utils.git import git_commit_hash, git_https_url_with_commit, git_origin_url, repo_root +from dvsim.utils.git import ( + git_commit_hash, + git_https_url_with_commit, + git_is_dirty, + git_origin_url, + repo_root, +) __all__ = () @@ -72,6 +78,29 @@ def test_git_short_commit_hash(tmp_path: "Path") -> None: git_commit_hash(tmp_path, short=True), equal_to(r.git.rev_parse(r.head, short=True)) ) + @staticmethod + def test_git_is_dirty(tmp_path: Path) -> None: + """Test that git_is_dirty reflects the working tree state.""" + # Value error if called outside a git repo + assert_that( + calling(git_is_dirty).with_args(tmp_path), + raises(ValueError), + ) + + r = Repo.init(path=tmp_path) + + file = tmp_path / "a" + file.write_text("file to commit") + r.index.add([file]) + r.index.commit("initial commit") + + # Clean tree + assert_that(git_is_dirty(tmp_path), equal_to(False)) + + # Modify a tracked file — now dirty + file.write_text("changed") + assert_that(git_is_dirty(tmp_path), equal_to(True)) + @staticmethod def test_git_origin_url(tmp_path: Path) -> None: """Test that the expected git remote origin url is returned."""