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.

Sign up for GitHub

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

Allow patial update to wpt manifest #47588

Merged
merged 2 commits into from
Jan 8, 2025
Merged

Allow patial update to wpt manifest #47588

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
616fb0d
Allow patially update to wpt manifest
WeizhongX Aug 13, 2024
f367c07
update per review comments
WeizhongX Nov 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions tools/manifest/manifest.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,8 @@
from copy import deepcopy
from logging import Logger
from multiprocessing import Pool
from typing import (Any, Callable, Container, Dict, IO, Iterator, Iterable, Optional, Set, Text, Tuple, Type,
Union)
from typing import (Any, Callable, Container, Dict, IO, Iterator, Iterable, List, Optional, Set, Text,
Tuple, Type, Union)

from . import jsonlib
from . import vcs
Expand Down Expand Up @@ -363,6 +363,7 @@ def load_and_update(tests_root: Text,
url_base: Text,
update: bool = True,
rebuild: bool = False,
paths_to_update: Optional[List[Text]] = None,
metadata_path: Optional[Text] = None,
cache_root: Optional[Text] = None,
working_copy: bool = True,
Expand Down Expand Up @@ -401,7 +402,7 @@ def load_and_update(tests_root: Text,
for retry in range(2):
try:
tree = vcs.get_tree(tests_root, manifest, manifest_path, cache_root,
working_copy, rebuild)
paths_to_update, working_copy, rebuild)
changed = manifest.update(tree, parallel)
break
except InvalidCacheError:
Expand Down
2 changes: 1 addition & 1 deletion tools/manifest/spec.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ def update_spec(tests_root: Text,
logger.info("Updating SPEC_MANIFEST")
try:
tree = vcs.get_tree(tests_root, manifest, manifest_path, cache_root,
working_copy, True)
None, working_copy, True)
changed = manifest.update(tree, parallel, compute_manifest_spec_items)
except InvalidCacheError:
logger.error("Manifest cache in spec.py was invalid.")
Expand Down
18 changes: 16 additions & 2 deletions tools/manifest/update.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
#!/usr/bin/env python3
import argparse
import os
from typing import Any, Optional, TYPE_CHECKING
from typing import Any, List, Optional, Text, TYPE_CHECKING

from . import manifest
from . import vcs
Expand All @@ -23,14 +23,15 @@ def update(tests_root: str,
manifest_path: Optional[str] = None,
working_copy: bool = True,
cache_root: Optional[str] = None,
paths_to_update: Optional[List[Text]] = None,
rebuild: bool = False,
parallel: bool = True
) -> bool:
logger.warning("Deprecated; use manifest.load_and_update instead")
logger.info("Updating manifest")

tree = vcs.get_tree(tests_root, manifest, manifest_path, cache_root,
working_copy, rebuild)
paths_to_update, working_copy, rebuild)
return manifest.update(tree, parallel)


Expand All @@ -42,9 +43,17 @@ def update_from_cli(**kwargs: Any) -> None:
if not kwargs["rebuild"] and kwargs["download"]:
download_from_github(path, tests_root)

paths_to_update = []
for path_to_update in kwargs["tests"]:
if path_to_update.startswith(tests_root):
paths_to_update.append(os.path.relpath(path_to_update, tests_root))
else:
logger.warning(f"{path_to_update} is not a WPT path")

manifest.load_and_update(tests_root,
path,
kwargs["url_base"],
paths_to_update=paths_to_update,
update=True,
rebuild=kwargs["rebuild"],
cache_root=kwargs["cache_root"],
Expand Down Expand Up @@ -79,6 +88,11 @@ def create_parser() -> argparse.ArgumentParser:
parser.add_argument(
"--no-parallel", dest="parallel", action="store_false",
help="Do not parallelize building the manifest")
parser.add_argument('tests',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Presumably I'm missing something, but I don't see where this command line argument is being used.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I said in the description, Additional work is needed to plug in this into the test runner. This change only make patial update in wpt manifest command possible.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Presumably I'm missing something, but I don't see where this command line argument is being used.

Line 49, above:

wpt/tools/manifest/update.py

Line 49 in b4aa684

paths_to_update=kwargs['tests'],

type=abs_path,
nargs='*',
help=('Test files or directories to update. '
'Omit to update all items under the test root.'))
return parser


Expand Down
39 changes: 28 additions & 11 deletions tools/manifest/vcs.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ def get_tree(tests_root: Text,
manifest: "Manifest",
manifest_path: Optional[Text],
cache_root: Optional[Text],
paths_to_update: Optional[List[Text]],
working_copy: bool = True,
rebuild: bool = False) -> "FileSystem":
tree = None
Expand All @@ -43,7 +44,9 @@ def get_tree(tests_root: Text,
manifest.url_base,
manifest_path=manifest_path,
cache_path=cache_root,
rebuild=rebuild)
paths_to_update=paths_to_update,
rebuild=rebuild,
)
return tree


Expand Down Expand Up @@ -91,10 +94,12 @@ def __init__(self,
tests_root: Text,
url_base: Text,
cache_path: Optional[Text],
paths_to_update: Optional[List[Text]] = None,
manifest_path: Optional[Text] = None,
rebuild: bool = False) -> None:
self.tests_root = tests_root
self.url_base = url_base
self.paths_to_update = paths_to_update or ['']
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Defaulting to [''] seems slightly surprising. Do we not want to default to [self.tests_root]?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is relevant to the other comments in __iter__. Similar to the response there, I'd like to keep this relative to the tests root.

self.ignore_cache = None
self.mtime_cache = None
tests_root_bytes = tests_root.encode("utf8")
Expand All @@ -109,17 +114,29 @@ def __init__(self,
git = GitHasher(tests_root)
self.hash_cache = git.hash_cache()

def __iter__(self) -> Iterator[Tuple[Text, Optional[Text], bool]]:
def _make_file_info(self,
path: Text,
path_stat: os.stat_result) -> Tuple[Text, Optional[Text], bool]:
mtime_cache = self.mtime_cache
for dirpath, dirnames, filenames in self.path_filter(
walk(self.tests_root.encode("utf8"))):
for filename, path_stat in filenames:
path = os.path.join(dirpath, filename).decode("utf8")
if mtime_cache is None or mtime_cache.updated(path, path_stat):
file_hash = self.hash_cache.get(path, None)
yield path, file_hash, True
else:
yield path, None, False
if mtime_cache is None or mtime_cache.updated(path, path_stat):
file_hash = self.hash_cache.get(path, None)
return path, file_hash, True
else:
return path, None, False

def __iter__(self) -> Iterator[Tuple[Text, Optional[Text], bool]]:
for path_to_update in self.paths_to_update:
path = os.path.join(self.tests_root, path_to_update)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to be joining to tests_root, or do we want to be joining to the current working directory? Certainly they'll frequently be the same, but this feels maybe slightly surprising?

From the CLI, I'd expect to be able to pass an absolute path in here and have it work — or give an error if it wasn't within the test root.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like path_to_update to be relative to tests_root.

If we want to accept absolute paths from CLI, I think we can handle that when process the command line arguments. But after that we only need to keep the part relative to tests_root, as paths out of test root can not be handled.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We eventually want to yield something that is relative to tests_root, so keep path_to_update relative to the tests_root seems more natural?

if os.path.isfile(path):
WeizhongX marked this conversation as resolved.
Show resolved Hide resolved
path_stat = os.stat(path)
yield self._make_file_info(path_to_update, path_stat)
elif os.path.isdir(path):
for dirpath, dirnames, filenames in self.path_filter(
walk(path.encode("utf8"))):
for filename, path_stat in filenames:
path = os.path.join(path_to_update,
os.path.join(dirpath, filename).decode("utf8"))
yield self._make_file_info(path, path_stat)

def dump_caches(self) -> None:
for cache in [self.mtime_cache, self.ignore_cache]:
Expand Down