-
Notifications
You must be signed in to change notification settings - Fork 1.5k
feat(.builders): store hashes of all dependency resolution inputs in metadata.json #22952
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
base: master
Are you sure you want to change the base?
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -22,6 +22,7 @@ | |||||
| RESOLUTION_DIR = REPO_DIR / '.deps' | ||||||
| LOCK_FILE_DIR = RESOLUTION_DIR / 'resolved' | ||||||
| DIRECT_DEP_FILE = REPO_DIR / 'agent_requirements.in' | ||||||
| WORKFLOW_FILE = REPO_DIR / '.github/workflows/resolve-build-deps.yaml' | ||||||
| CACHE_CONTROL = 'public, max-age=15' | ||||||
| VALID_PROJECT_NAME = re.compile(r'^([A-Z0-9]|[A-Z0-9][A-Z0-9._-]*[A-Z0-9])$', re.IGNORECASE) | ||||||
| UNNORMALIZED_PROJECT_NAME_CHARS = re.compile(r'[-_.]+') | ||||||
|
|
@@ -76,6 +77,28 @@ def hash_file(path: Path) -> str: | |||||
| return sha256(f.read()).hexdigest() | ||||||
|
|
||||||
|
|
||||||
| def hash_directory(path: Path) -> str: | ||||||
| """Compute a combined SHA256 hash of all files in a directory.""" | ||||||
| h = sha256() | ||||||
| for file_path in sorted(path.rglob('*'), key=lambda p: p.relative_to(path)): | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||||||
| if file_path.is_file(): | ||||||
|
||||||
| if file_path.is_file(): | |
| if file_path.is_file() and file_patch.suffix() != ".pyc" and not any(p.startswith('.') or p == '__pycache__' for p in file_path.parts): |
(dirty+heavy+not tested, there's probably a much better way to achieve the same)
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hash_directory includes itself in its own hash => potential instability:
when upload.py changes, hash_directory(BUILDER_DIR) will change (BUILDER_DIR = Path(__file__).parent), which might be the intended behavior, but it also means the hash of .builders includes any .pyc cache files, __pycache__/, .pytest_cache/, etc. generated during current or earlier runs.
(see comment on path.rglob('*') for a suggested fix)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is intended behavior but you're right that we can ignore some files.
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The keys 'agent_requirements.in', '.github/workflows/resolve-build-deps.yaml', '.builders' are string literals unrelated to the actual constants DIRECT_DEP_FILE, WORKFLOW_FILE, BUILDER_DIR.
If those constants are ever changed (e.g., the workflow is renamed), the metadata keys won't update and the future check comparing hashes will silently mismatch.
Consider deriving the keys from the constants:
DIRECT_DEP_FILE.relative_to(REPO_DIR).as_posix(), # 'agent_requirements.in'
WORKFLOW_FILE.relative_to(REPO_DIR).as_posix(), # '.github/workflows/resolve-build-deps.yaml'
BUILDER_DIR.relative_to(REPO_DIR).as_posix(), # '.builders'This would also validate that the constants are actually rooted under REPO_DIR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
patched_input_filesis not used in the function.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done