Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

manifest.GitHasher.hash_cache is broken when tests_root != repo_root #17749

Closed
Hexcles opened this issue Jul 9, 2019 · 2 comments
Closed

manifest.GitHasher.hash_cache is broken when tests_root != repo_root #17749

Hexcles opened this issue Jul 9, 2019 · 2 comments

Comments

@Hexcles
Copy link
Member

Hexcles commented Jul 9, 2019

In some setup (e.g. Chromium), the WPT root is a subdirectory of the main repository (i.e. tests_root != repo_root), in which case ./wpt manifest may fail to detect local changes. Here's my hypothesis:

In manifest.GitHasher._local_changes, a list of changed files are populated with paths relative to the repo root (by git status -z, whose output isn't affected by CWD).

def _local_changes(self):
# type: () -> Set[bytes]
"""get a set of files which have changed between HEAD and working copy"""
assert self.git is not None
changes = set() # type: Set[bytes]
cmd = [b"status", b"-z", b"--ignore-submodules=all"]
data = self.git(*cmd) # type: bytes

However, later paths relative to the tests root (gs ls-tree -r -z HEAD with CWD set to the tests root) are compared against the list generated above.

cmd = [b"ls-tree", b"-r", b"-z", b"HEAD"]
local_changes = self._local_changes()
for result in self.git(*cmd).split(b"\0")[:-1]: # type: bytes
data, rel_path = result.rsplit(b"\t", 1)
hash_cache[rel_path] = None if rel_path in local_changes else data.split(b" ", 3)[2]

As a result, nothing is considered changed and the git hash will be used as the real hash of the working tree incorrectly.

@Hexcles
Copy link
Member Author

Hexcles commented Jul 9, 2019

Easy fix: disable GitHasher when tests_root != repo_root (can be done in util.py by checking the output of git rev-parse --show-toplevel).

Otherwise, we can somehow get git status to produce relative paths (and only files changed in the current directory). Perhaps we need to use another git subcommand. Or we could manipulate the paths in Python, which I'd very much like to avoid.

Hexcles added a commit that referenced this issue Jul 11, 2019
Fixes #17749 by using `git diff --name-only --relative` instead of `git
status`. The `--relative` flag is key: it limits the changes to the
current directory and also outputs relative paths. CWD is already set to
tests_root by utils.git.
@Hexcles
Copy link
Member Author

Hexcles commented Jul 11, 2019

Fixed by #17785 .

@Hexcles Hexcles closed this as completed Jul 11, 2019
aarongable pushed a commit to chromium/chromium that referenced this issue Jul 11, 2019
Bug: web-platform-tests/wpt#17749
Fix: web-platform-tests/wpt#17785

This bug would prevent the manifest from updating when local changes are
made in the working tree, which severely affects the daily workflow of
Blink engineers (bots are not affected).

Change-Id: Icf080553a8c8626c10d7973eb21ec4214c889d16
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1697371
Reviewed-by: Luke Z <lpz@chromium.org>
Commit-Queue: Robert Ma <robertma@chromium.org>
Cr-Commit-Position: refs/heads/master@{#676570}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant