Copilot commented on code in PR #64124:
URL: https://github.com/apache/airflow/pull/64124#discussion_r3066473358
##########
scripts/ci/prek/ts_compile_lint_ui.py:
##########
@@ -50,8 +52,20 @@
all_ts_files.append("src/vite-env.d.ts")
print("All TypeScript files:", all_ts_files)
- run_command(["pnpm", "config", "set", "store-dir", ".pnpm-store"], cwd=dir)
- run_command(["pnpm", "install", "--frozen-lockfile",
"--config.confirmModulesPurge=false"], cwd=dir)
+ hash_file = AIRFLOW_ROOT_PATH / ".build" / "ui" / "pnpm-install-hash.txt"
+ node_modules = dir / "node_modules"
+ current_hash = hashlib.sha256(
+ (dir / "package.json").read_bytes() + (dir /
"pnpm-lock.yaml").read_bytes()
+ ).hexdigest()
+ stored_hash = hash_file.read_text().strip() if hash_file.exists() else ""
+
+ if node_modules.exists() and current_hash == stored_hash:
+ print("pnpm deps unchanged — skipping install.")
+ else:
+ run_command(["pnpm", "config", "set", "store-dir", ".pnpm-store"],
cwd=dir)
+ run_command(["pnpm", "install", "--frozen-lockfile",
"--config.confirmModulesPurge=false"], cwd=dir)
+ hash_file.parent.mkdir(parents=True, exist_ok=True)
+ hash_file.write_text(current_hash)
Review Comment:
Writing the hash file non-atomically can leave a truncated/partial hash if
the process is interrupted mid-write (or if multiple runs overlap), leading to
unnecessary reinstalls or inconsistent behavior. Prefer an atomic write pattern
(write to a temp file in the same directory, then replace/rename) to make the
cache robust.
##########
scripts/ci/prek/ts_compile_lint_ui.py:
##########
@@ -50,8 +52,20 @@
all_ts_files.append("src/vite-env.d.ts")
print("All TypeScript files:", all_ts_files)
- run_command(["pnpm", "config", "set", "store-dir", ".pnpm-store"], cwd=dir)
- run_command(["pnpm", "install", "--frozen-lockfile",
"--config.confirmModulesPurge=false"], cwd=dir)
+ hash_file = AIRFLOW_ROOT_PATH / ".build" / "ui" / "pnpm-install-hash.txt"
+ node_modules = dir / "node_modules"
+ current_hash = hashlib.sha256(
+ (dir / "package.json").read_bytes() + (dir /
"pnpm-lock.yaml").read_bytes()
+ ).hexdigest()
Review Comment:
Hashing by simple byte concatenation can (in rare but real cases) produce
the same combined byte string for different `(package.json, pnpm-lock.yaml)`
pairs (e.g., if bytes are shifted between files), causing a false cache hit and
skipping a required install. To make the hash unambiguous, include a delimiter
and/or the length of each file, or hash each file separately and combine the
digests.
```suggestion
package_json_bytes = (dir / "package.json").read_bytes()
pnpm_lock_bytes = (dir / "pnpm-lock.yaml").read_bytes()
current_hash_hasher = hashlib.sha256()
current_hash_hasher.update(f"package.json:{len(package_json_bytes)}:".encode("utf-8"))
current_hash_hasher.update(package_json_bytes)
current_hash_hasher.update(f"pnpm-lock.yaml:{len(pnpm_lock_bytes)}:".encode("utf-8"))
current_hash_hasher.update(pnpm_lock_bytes)
current_hash = current_hash_hasher.hexdigest()
```
##########
scripts/ci/prek/ts_compile_lint_ui.py:
##########
@@ -50,8 +52,20 @@
all_ts_files.append("src/vite-env.d.ts")
print("All TypeScript files:", all_ts_files)
- run_command(["pnpm", "config", "set", "store-dir", ".pnpm-store"], cwd=dir)
- run_command(["pnpm", "install", "--frozen-lockfile",
"--config.confirmModulesPurge=false"], cwd=dir)
+ hash_file = AIRFLOW_ROOT_PATH / ".build" / "ui" / "pnpm-install-hash.txt"
+ node_modules = dir / "node_modules"
+ current_hash = hashlib.sha256(
+ (dir / "package.json").read_bytes() + (dir /
"pnpm-lock.yaml").read_bytes()
+ ).hexdigest()
+ stored_hash = hash_file.read_text().strip() if hash_file.exists() else ""
+
+ if node_modules.exists() and current_hash == stored_hash:
Review Comment:
`node_modules.exists()` will also be true if there’s a file (or unexpected
path type) named `node_modules`. Using `node_modules.is_dir()` (or checking
`exists()` + `is_dir()`) makes the guard more precise and avoids incorrectly
skipping installs in edge cases.
```suggestion
if node_modules.is_dir() and current_hash == stored_hash:
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
