Jonathan Lee (Gerrit)

unread,

Aug 20, 2025, 8:36:40 PMAug 20

to Brian Sheedy, chromium...@chromium.org, AyeAye, Chromium LUCI CQ, blink-revie...@chromium.org, blink-...@chromium.org, blink-rev...@chromium.org

Attention needed from Brian Sheedy

Jonathan Lee added 1 comment

File third_party/blink/tools/blinkpy/web_tests/models/test_run_results.py

Line 399, Patchset 4 (Latest): git = port_obj.host.git(path)

Jonathan Lee . resolved

`@memoized` still doesn't support keyword args yet unfortunately. `@functools.*cache` won't work either because it holds onto `self`, preventing GC.

Open in Gerrit

Related details

Attention is currently required from:

Brian Sheedy

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Brian Sheedy (Gerrit)

unread,

Aug 20, 2025, 8:49:26 PMAug 20

to Jonathan Lee, chromium...@chromium.org, AyeAye, Chromium LUCI CQ, blink-revie...@chromium.org, blink-...@chromium.org, blink-rev...@chromium.org

Attention needed from Jonathan Lee

Brian Sheedy added 1 comment

File third_party/blink/tools/blinkpy/web_tests/models/test_run_results.py

Line 399, Patchset 4 (Latest): git = port_obj.host.git(path)

Jonathan Lee . unresolved

`@memoized` still doesn't support keyword args yet unfortunately. `@functools.*cache` won't work either because it holds onto `self`, preventing GC.

Brian Sheedy

Perhaps a workaround could be to memoize a helper instead of the method in Host?

E.g. instead of:

```
    @memoized
    def git(self, path: os.PathLike | None = None) -> Git | None:
        git = Git(cwd=path,
                  executive=self.executive,
                  filesystem=self.filesystem,
                  platform=self.platform)
        return git if git.checkout_root else None
```

Do:

```
    def git(self, path: os.PathLike | None = None) -> Git | None:
        return _create_git(cwd=path, executive=self.executive, ...)

...

@functools.cache
def _create_git(*, **kwargs) -> Git | None:
    git = Git(**kwargs)
    return git if git.checkout_root else None
```

It would be nice to remove the Blink-specific version in favor of `functools.cache` if possible.

Open in Gerrit

Related details

Attention is currently required from:

Jonathan Lee

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review

No-Unresolved-Comments

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Jonathan Lee (Gerrit)

unread,

Aug 21, 2025, 4:36:09 PMAug 21

to Brian Sheedy, chromium...@chromium.org, AyeAye, Chromium LUCI CQ, blink-revie...@chromium.org, blink-...@chromium.org, blink-rev...@chromium.org

Attention needed from Brian Sheedy

Jonathan Lee voted and added 1 comment

Votes added by Jonathan Lee

Commit-Queue

+1

1 comment

File third_party/blink/tools/blinkpy/web_tests/models/test_run_results.py

Line 399, Patchset 4: git = port_obj.host.git(path)

Jonathan Lee . resolved

`@memoized` still doesn't support keyword args yet unfortunately. `@functools.*cache` won't work either because it holds onto `self`, preventing GC.

Brian Sheedy

Perhaps a workaround could be to memoize a helper instead of the method in Host?

E.g. instead of:

```
    @memoized
    def git(self, path: os.PathLike | None = None) -> Git | None:
        git = Git(cwd=path,
                  executive=self.executive,
                  filesystem=self.filesystem,
                  platform=self.platform)
        return git if git.checkout_root else None
```

Do:

```
    def git(self, path: os.PathLike | None = None) -> Git | None:
        return _create_git(cwd=path, executive=self.executive, ...)

...

@functools.cache
def _create_git(*, **kwargs) -> Git | None:
    git = Git(**kwargs)
    return git if git.checkout_root else None
```

It would be nice to remove the Blink-specific version in favor of `functools.cache` if possible.

Jonathan Lee

Done

Open in Gerrit

Related details

Attention is currently required from:

Brian Sheedy

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Brian Sheedy (Gerrit)

unread,

Aug 21, 2025, 4:39:17 PMAug 21

to Jonathan Lee, chromium...@chromium.org, AyeAye, Chromium LUCI CQ, blink-revie...@chromium.org, blink-...@chromium.org, blink-rev...@chromium.org

Attention needed from Jonathan Lee

Brian Sheedy voted and added 2 comments

Votes added by Brian Sheedy

Code-Review

+1

2 comments

Patchset-level comments

File-level comment, Patchset 5 (Latest):

Brian Sheedy . resolved

LGTM % nit

File third_party/blink/tools/blinkpy/common/host.py

Line 70, Patchset 5 (Latest):def _cached_git(**kwargs) -> Git:

Brian Sheedy . unresolved

Nit: Can also return None.

Open in Gerrit

Related details

Attention is currently required from:

Jonathan Lee

Submit Requirements:

Code-Coverage
Code-Owners

Code-Review
No-Unresolved-Comments

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Jonathan Lee (Gerrit)

unread,

Aug 21, 2025, 4:44:14 PMAug 21

to Brian Sheedy, chromium...@chromium.org, AyeAye, Chromium LUCI CQ, blink-revie...@chromium.org, blink-...@chromium.org, blink-rev...@chromium.org

Jonathan Lee voted and added 1 comment

Votes added by Jonathan Lee

Auto-Submit	+0
Commit-Queue	+2

1 comment

File third_party/blink/tools/blinkpy/common/host.py

Line 70, Patchset 5:def _cached_git(**kwargs) -> Git:

Brian Sheedy . resolved

Nit: Can also return None.

Jonathan Lee

Oops, done. One day, I hope to get enough of `blinkpy/` hinted to actually check these and function as more than documentation.

Open in Gerrit

Related details

Attention set is empty

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

open

diffy

Brian Sheedy (Gerrit)

unread,

Aug 21, 2025, 4:53:24 PMAug 21

to Jonathan Lee, chromium...@chromium.org, AyeAye, Chromium LUCI CQ, blink-revie...@chromium.org, blink-...@chromium.org, blink-rev...@chromium.org

Attention needed from Jonathan Lee

Brian Sheedy voted and added 1 comment

Votes added by Brian Sheedy

Code-Review

+1

1 comment

File third_party/blink/tools/blinkpy/common/host.py

Line 70, Patchset 5:def _cached_git(**kwargs) -> Git:

Brian Sheedy . resolved

Nit: Can also return None.

Jonathan Lee

Oops, done. One day, I hope to get enough of `blinkpy/` hinted to actually check these and function as more than documentation.

Brian Sheedy

I'm pretty sure the type hinting helpers I set up should allow for skipping certain files, so you could add basically every file under `blinkpy/` to that list and use it as a burndown list for what needs to have type hinting applied.

Open in Gerrit

Related details

Attention is currently required from:

Jonathan Lee

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

open

diffy

Chromium LUCI CQ (Gerrit)

unread,

Aug 21, 2025, 5:28:12 PMAug 21

to Jonathan Lee, Brian Sheedy, chromium...@chromium.org, AyeAye, blink-revie...@chromium.org, blink-...@chromium.org, blink-rev...@chromium.org

Chromium LUCI CQ submitted the change

Change information

Commit message:

[blinkpy] Support non-Git checkouts like cogfs

Most callsites can gracefully function without a `git` checkout (e.g.,
`rebaseline-cl` skips staging `*-expected.*`).

Make sure to memoize `Host.git()` so that warnings about an invalid
checkout are only logged once per `path`.

Bug: 427299613

Change-Id: I8c53751887f3b169bd3f9e918bb1f6e7c6007165

Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6866593

Commit-Queue: Jonathan Lee <jonath...@google.com>

Reviewed-by: Brian Sheedy <bsh...@chromium.org>

Cr-Commit-Position: refs/heads/main@{#1504716}

Files:

M third_party/blink/tools/blinkpy/common/checkout/git.py
M third_party/blink/tools/blinkpy/common/host.py
M third_party/blink/tools/blinkpy/style/main.py
M third_party/blink/tools/blinkpy/tool/commands/pretty_diff.py
M third_party/blink/tools/blinkpy/tool/commands/rebaseline.py
M third_party/blink/tools/blinkpy/tool/commands/rebaseline_cl.py
M third_party/blink/tools/blinkpy/tool/commands/rebaseline_cl_unittest.py
M third_party/blink/tools/blinkpy/wpt_tests/wpt_adapter.py

Change size: M

Delta: 8 files changed, 81 insertions(+), 49 deletions(-)

Branch: refs/heads/main

Submit Requirements:

Code-Review: +1 by Brian Sheedy

Open in Gerrit

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

open

diffy

satisfied_requirement

Reply all

Reply to author

Forward

[blinkpy] Support non-Git checkouts like cogfs [chromium/src : main]

Jonathan Lee (Gerrit)

Jonathan Lee added 1 comment

Related details

Brian Sheedy (Gerrit)

Brian Sheedy added 1 comment

Related details

Jonathan Lee (Gerrit)

Jonathan Lee voted and added 1 comment

Votes added by Jonathan Lee

1 comment

Related details

Brian Sheedy (Gerrit)

Brian Sheedy voted and added 2 comments

Votes added by Brian Sheedy

2 comments

Related details

Jonathan Lee (Gerrit)

Jonathan Lee voted and added 1 comment

Votes added by Jonathan Lee

1 comment

Related details

Brian Sheedy (Gerrit)

Brian Sheedy voted and added 1 comment

Votes added by Brian Sheedy

1 comment

Related details

Chromium LUCI CQ (Gerrit)

Chromium LUCI CQ submitted the change

Change information