Meta: Stop building vcpkg dependencies serially - #10963
Conversation
📝 WalkthroughWalkthroughChangesThe PR adds a vcpkg dependency builder that creates per-port Ninja nodes, manages caches and concurrency, and supports fetch and rebuild modes. Ladybird gains a Vcpkg dependency build
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant LadybirdCLI
participant build_dependencies
participant vcpkg
participant Ninja
participant run_node
participant BinaryCache
LadybirdCLI->>build_dependencies: configure dependency build
build_dependencies->>vcpkg: query dependency graph
build_dependencies->>Ninja: generate per-port build graph
Ninja->>run_node: execute dependency node
run_node->>vcpkg: install isolated port
vcpkg->>BinaryCache: restore or store package
run_node-->>Ninja: write completion stamp
build_dependencies-->>LadybirdCLI: return staged triplet
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (3)
Meta/Utils/vcpkg_deps.py (3)
266-281: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winNode inputs omit the overlay ports of the dependency closure.
Line 272 adds only the overlay port files for
name. Each node's manifest, however, builds the whole closure returned bydependency_closure. If you edit an overlay port for a dependency, that dependency's ABI hash changes, but the dependent node's.stampstays up to date and ninja does not re-run it. The dependent is then rebuilt serially by the final manifest install instead of in parallel.Extend the inputs to cover the overlay ports of every port in the closure.
♻️ Proposed change
- overlay_port = overlay_ports_dir() / name - if overlay_port.is_dir(): - inputs.extend(sorted(str(f) for f in overlay_port.rglob("*") if f.is_file())) + for closure_port in sorted(dependency_closure(graph, name)): + overlay_port = overlay_ports_dir() / closure_port + if overlay_port.is_dir(): + inputs.extend(sorted(str(f) for f in overlay_port.rglob("*") if f.is_file()))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Meta/Utils/vcpkg_deps.py` around lines 266 - 281, Update the node input construction around dependency_closure and overlay_port so each node includes files from the overlay port directories of every port in its dependency closure, not only name. Preserve the existing sorted file ordering and include these paths in the Ninja build inputs so changes to any closure dependency invalidate the dependent node.
486-487: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueA failed dry run is treated as "work to do".
subprocess.runat line 486 captures output but never checksreturncode. Ifbuild.ninjais malformed or ninja is missing,check.stdoutis empty,have_workbecomesTrue, and the real error surfaces much later from thecheck_callat line 507. Checkcheck.returncodeand report the capturedstderr.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Meta/Utils/vcpkg_deps.py` around lines 486 - 487, Update the dry-run subprocess handling in the dependency build flow to validate check.returncode before deriving have_work. When ninja fails, report the captured check.stderr and stop with an appropriate error instead of treating the failure as pending work; preserve the existing no-work detection for successful runs.
360-410: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueBind
heavyunconditionally to make the log path robust.
heavyis assigned only inside theif token_fifo ...block at line 383. Line 392 reads it. Theif held:guard at line 391 keeps this safe today, becauseheldis non-zero only when the block ran. The two variables are set in different places, so a later change to the token logic can turn this into aNameError.♻️ Proposed change
fifo_fd = None held = 0 + heavy = False token_fifo = environment.get("LADYBIRD_VCPKG_TOKEN_FIFO")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Meta/Utils/vcpkg_deps.py` around lines 360 - 410, Initialize heavy unconditionally before the token FIFO block in run_node, using a false/default value, then retain the existing weighted assignment when the token pool logic runs. Ensure the held-token logging path can always reference heavy without risking an unbound local variable.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Meta/ladybird.py`:
- Around line 194-200: The deps branch in Meta/ladybird.py lines 194-200 must
reject HostSystem.Windows before calling build_dependencies, exiting with a
message that staged dependency builds are unsupported on Windows. In
Meta/Utils/vcpkg_deps.py lines 466-468, enforce the same restriction at the
build_dependencies boundary or move the fcntl import behind a platform guard so
Windows cannot reach POSIX-only code.
In `@Meta/Utils/vcpkg_deps.py`:
- Around line 492-507: Update the ninja invocation in the token-pool setup to
always pass the resolved `tokens` job count, including when `jobs` is unset.
Replace the conditional `-j` argument handling in the `ninja_arguments`
construction while preserving the existing resolved count and environment
configuration.
- Around line 198-207: Update dependency_closure to handle dependencies missing
from graph without raising an unhandled KeyError: when processing current,
verify it exists in graph before accessing graph[current].dependencies, and
provide contextual failure handling for the absent port while preserving closure
tracking.
- Around line 169-195: Update query_dependency_graph to preserve a separator
when combining result.stdout and result.stderr, then validate that the parsed
graph is non-empty after processing the dependency-info output. Raise a clear
RuntimeError when no dependency entries are parsed, including the command-output
context needed to diagnose a format change, instead of returning an empty graph.
- Around line 102-114: Update extra_vcpkg_variables so the macOS branch emits
VCPKG_OSX_DEPLOYMENT_TARGET only when CMAKE_OSX_DEPLOYMENT_TARGET is set,
matching generate_vcpkg_toolchain_variables.cmake. Remove the unconditional 14.0
fallback and preserve the existing behavior for non-macOS platforms.
- Around line 334-357: Update acquire_build_tokens so heavy-node acquisition is
all-or-nothing and cannot leave tokens stranded while waiting for target; use a
dedicated lock to serialize the heavy acquisition path, then acquire the
required tokens before allowing other heavy nodes to contend. Preserve the
existing light-node behavior and nonblocking cap logic, and ensure the lock is
released on every exit.
---
Nitpick comments:
In `@Meta/Utils/vcpkg_deps.py`:
- Around line 266-281: Update the node input construction around
dependency_closure and overlay_port so each node includes files from the overlay
port directories of every port in its dependency closure, not only name.
Preserve the existing sorted file ordering and include these paths in the Ninja
build inputs so changes to any closure dependency invalidate the dependent node.
- Around line 486-487: Update the dry-run subprocess handling in the dependency
build flow to validate check.returncode before deriving have_work. When ninja
fails, report the captured check.stderr and stop with an appropriate error
instead of treating the failure as pending work; preserve the existing no-work
detection for successful runs.
- Around line 360-410: Initialize heavy unconditionally before the token FIFO
block in run_node, using a false/default value, then retain the existing
weighted assignment when the token pool logic runs. Ensure the held-token
logging path can always reference heavy without risking an unbound local
variable.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: f748dcfc-8c80-4710-bf28-dfea367991cf
📒 Files selected for processing (2)
Meta/Utils/vcpkg_deps.pyMeta/ladybird.py
| elif args.command == "deps": | ||
| _, build_preset_dir = configure_build_env(platform, args.preset, args.jobs) | ||
| build_vcpkg() | ||
| (cc, cxx) = pick_host_compiler(platform, args.cc, args.cxx) | ||
| build_dependencies( | ||
| platform, args.preset, build_preset_dir, cc, cxx, args.jobs, fetch_only=args.fetch, rebuild=args.rebuild | ||
| ) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
The deps command reaches POSIX-only code on Windows. build_dependencies depends on fcntl and os.mkfifo, which do not exist on Windows. configure_main guards the call with staged_deps, but the deps subcommand does not, so ./ladybird.py deps on Windows fails with ModuleNotFoundError: No module named 'fcntl'.
Meta/ladybird.py#L194-L200: add aplatform.host_system == HostSystem.Windowscheck in thedepsbranch and exit with a message that staged dependency builds are not supported on Windows.Meta/Utils/vcpkg_deps.py#L466-L468: moveimport fcntlto a guarded location, or raise an explicitRuntimeErrorat the top ofbuild_dependencieswhen the host system is Windows, so the constraint is enforced at the function boundary.
📍 Affects 2 files
Meta/ladybird.py#L194-L200(this comment)Meta/Utils/vcpkg_deps.py#L466-L468
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Meta/ladybird.py` around lines 194 - 200, The deps branch in Meta/ladybird.py
lines 194-200 must reject HostSystem.Windows before calling build_dependencies,
exiting with a message that staged dependency builds are unsupported on Windows.
In Meta/Utils/vcpkg_deps.py lines 466-468, enforce the same restriction at the
build_dependencies boundary or move the fcntl import behind a platform guard so
Windows cannot reach POSIX-only code.
| def extra_vcpkg_variables(platform: Platform, cc: str, cxx: str) -> str: | ||
| # The data generated by this must be exactly identical to the one generated by Meta/CMake/vcpkg/generate_vcpkg_toolchain_variables.cmake; | ||
| # as vcpkg will use the identity of that as part of the ABI hash. | ||
| variables = "" | ||
| if platform.host_system != HostSystem.Windows: | ||
| variables += f"set(ENV{{CC}} {cc})\n" | ||
| variables += f"set(ENV{{CXX}} {cxx})\n" | ||
| if platform.host_system == HostSystem.Linux and not os.environ.get("LAGOM_USE_LINKER"): | ||
| variables += "set(ENV{LDFLAGS} -Wl,-z,noseparate-code)\n" | ||
| if platform.host_system == HostSystem.macOS: | ||
| deployment_target = os.environ.get("CMAKE_OSX_DEPLOYMENT_TARGET") or "14.0" | ||
| variables += f"set(VCPKG_OSX_DEPLOYMENT_TARGET {deployment_target})\n" | ||
| return variables |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Compare the generated variable content with the CMake generator.
fd -t f 'generate_vcpkg_toolchain_variables.cmake' Meta | xargs -r cat -nRepository: LadybirdBrowser/ladybird
Length of output: 4547
🏁 Script executed:
#!/bin/bash
set -u
echo "== locate vcpkg_deps.py =="
fd -t f 'vcpkg_deps.py' Meta | sed "s#^\./##"
echo "== relevant vcpkg_deps.py section =="
python3 - <<'PY'
from pathlib import Path
for p in Path('Meta').rglob('vcpkg_deps.py'):
print(f"--- {p} ---")
lines = p.read_text().splitlines()
for i in range(94, 150):
if i <= len(lines):
print(f"{i:4}: {lines[i-1]}")
PY
echo "== search for extra_vcpkg_variables call =="
rg -n "extra_vcpkg_variables|build-vcpkg-variables|vcpkg_deps" Meta -SRepository: LadybirdBrowser/ladybird
Length of output: 4511
🏁 Script executed:
#!/bin/bash
set -u
python3 - <<'PY'
from pathlib import Path
cmake = Path("Meta/CMake/vcpkg/generate_vcpkg_toolchain_variables.cmake").read_text().splitlines()
python_fn = Path("Meta/Utils/vcpkg_deps.py").read_text().splitlines()[101:114] # zero-indexed lines 102-114
def cmake_lines(s):
out = []
for line in s.splitlines():
if "string(APPEND EXTRA_VCPKG_VARIABLES" in line or "file(WRITE" in line:
out.append(line.strip())
return out
print("CMake append/write block:")
for line in cmake_lines(" ".join(cmake[3:27])):
print(line)
print("\nPython extra_vcpkg_variables() body:")
for line in python_fn:
print(line)
PYRepository: LadybirdBrowser/ladybird
Length of output: 2119
Align macOS coverage between the Python and CMake variable generators.
Meta/CMake/vcpkg/generate_vcpkg_toolchain_variables.cmake writes VCPKG_OSX_DEPLOYMENT_TARGET when CMAKE_OSX_DEPLOYMENT_TARGET is set, while Meta/Utils/vcpkg_deps.py always writes it with a 14.0 fallback. If the CMake generator runs without that environment variable, the bytes written for this entry differ, which can invalidate the vcpkg ABI hash.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Meta/Utils/vcpkg_deps.py` around lines 102 - 114, Update
extra_vcpkg_variables so the macOS branch emits VCPKG_OSX_DEPLOYMENT_TARGET only
when CMAKE_OSX_DEPLOYMENT_TARGET is set, matching
generate_vcpkg_toolchain_variables.cmake. Remove the unconditional 14.0 fallback
and preserve the existing behavior for non-macOS platforms.
| if result.returncode != 0: | ||
| print(result.stdout, file=sys.stderr) | ||
| print(result.stderr, file=sys.stderr) | ||
| raise RuntimeError("vcpkg depend-info failed") | ||
|
|
||
| # NOTE: This folds host and target entries, same as what vcpkg does. | ||
| # FIXME: Investigate when cross-compilation becomes a problem this has to touch. | ||
| graph: dict[str, PortNode] = {} | ||
| for line in (result.stdout + result.stderr).splitlines(): | ||
| match = DEPEND_INFO_REGEX.match(line.strip()) | ||
| if not match: | ||
| continue | ||
| (name, features, _, dependencies) = match.groups() | ||
|
|
||
| node = graph.setdefault(name, PortNode(name)) | ||
| if features: | ||
| node.features.update(f for f in (feature.strip() for feature in features.split(",")) if f and f != "core") | ||
|
|
||
| for dependency in dependencies.split(","): | ||
| dependency = dependency.strip() | ||
| if not dependency: | ||
| continue | ||
| dependency = dependency.split("[")[0].removesuffix(":host") | ||
| if dependency != name: | ||
| node.dependencies.add(dependency) | ||
|
|
||
| return graph |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Add a guard for an empty dependency graph.
query_dependency_graph only fails on a non-zero exit code. If the depend-info --format=list output format changes, every line fails the regex and the function returns an empty graph. The build then generates an empty build.ninja, reports "no work to do", and falls back to a single serial manifest install. The parallel build is lost with no error.
Also, result.stdout + result.stderr concatenates without a separator. If stdout does not end with a newline, the last stdout line and the first stderr line merge into one unparsable line.
🛡️ Proposed guard
graph: dict[str, PortNode] = {}
- for line in (result.stdout + result.stderr).splitlines():
+ for line in (result.stdout.splitlines() + result.stderr.splitlines()):
match = DEPEND_INFO_REGEX.match(line.strip())
if not match:
continue
@@
+ if not graph:
+ raise RuntimeError("vcpkg depend-info returned no parsable dependency entries")
+
return graph📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if result.returncode != 0: | |
| print(result.stdout, file=sys.stderr) | |
| print(result.stderr, file=sys.stderr) | |
| raise RuntimeError("vcpkg depend-info failed") | |
| # NOTE: This folds host and target entries, same as what vcpkg does. | |
| # FIXME: Investigate when cross-compilation becomes a problem this has to touch. | |
| graph: dict[str, PortNode] = {} | |
| for line in (result.stdout + result.stderr).splitlines(): | |
| match = DEPEND_INFO_REGEX.match(line.strip()) | |
| if not match: | |
| continue | |
| (name, features, _, dependencies) = match.groups() | |
| node = graph.setdefault(name, PortNode(name)) | |
| if features: | |
| node.features.update(f for f in (feature.strip() for feature in features.split(",")) if f and f != "core") | |
| for dependency in dependencies.split(","): | |
| dependency = dependency.strip() | |
| if not dependency: | |
| continue | |
| dependency = dependency.split("[")[0].removesuffix(":host") | |
| if dependency != name: | |
| node.dependencies.add(dependency) | |
| return graph | |
| if result.returncode != 0: | |
| print(result.stdout, file=sys.stderr) | |
| print(result.stderr, file=sys.stderr) | |
| raise RuntimeError("vcpkg depend-info failed") | |
| # NOTE: This folds host and target entries, same as what vcpkg does. | |
| # FIXME: Investigate when cross-compilation becomes a problem this has to touch. | |
| graph: dict[str, PortNode] = {} | |
| for line in (result.stdout.splitlines() + result.stderr.splitlines()): | |
| match = DEPEND_INFO_REGEX.match(line.strip()) | |
| if not match: | |
| continue | |
| (name, features, _, dependencies) = match.groups() | |
| node = graph.setdefault(name, PortNode(name)) | |
| if features: | |
| node.features.update(f for f in (feature.strip() for feature in features.split(",")) if f and f != "core") | |
| for dependency in dependencies.split(","): | |
| dependency = dependency.strip() | |
| if not dependency: | |
| continue | |
| dependency = dependency.split("[")[0].removesuffix(":host") | |
| if dependency != name: | |
| node.dependencies.add(dependency) | |
| if not graph: | |
| raise RuntimeError("vcpkg depend-info returned no parsable dependency entries") | |
| return graph |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Meta/Utils/vcpkg_deps.py` around lines 169 - 195, Update
query_dependency_graph to preserve a separator when combining result.stdout and
result.stderr, then validate that the parsed graph is non-empty after processing
the dependency-info output. Raise a clear RuntimeError when no dependency
entries are parsed, including the command-output context needed to diagnose a
format change, instead of returning an empty graph.
| def dependency_closure(graph: dict[str, PortNode], port: str) -> set[str]: | ||
| closure: set[str] = set() | ||
| queue = [port] | ||
| while queue: | ||
| current = queue.pop() | ||
| if current in closure: | ||
| continue | ||
| closure.add(current) | ||
| queue.extend(graph[current].dependencies) | ||
| return closure |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Handle dependency names that are absent from the graph.
graph[current] raises KeyError if a dependency name appears on the right-hand side of a depend-info line but never gets its own parsed line. DEPEND_INFO_REGEX only accepts [a-zA-Z0-9-]+ for the port name, so any port whose line does not match is missing as a key while still being referenced as a dependency. The failure surfaces as an unhandled KeyError with no context.
🛡️ Proposed fix
while queue:
current = queue.pop()
if current in closure:
continue
+ if current not in graph:
+ raise RuntimeError(f"vcpkg depend-info referenced unknown port {current!r}")
closure.add(current)
queue.extend(graph[current].dependencies)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def dependency_closure(graph: dict[str, PortNode], port: str) -> set[str]: | |
| closure: set[str] = set() | |
| queue = [port] | |
| while queue: | |
| current = queue.pop() | |
| if current in closure: | |
| continue | |
| closure.add(current) | |
| queue.extend(graph[current].dependencies) | |
| return closure | |
| def dependency_closure(graph: dict[str, PortNode], port: str) -> set[str]: | |
| closure: set[str] = set() | |
| queue = [port] | |
| while queue: | |
| current = queue.pop() | |
| if current in closure: | |
| continue | |
| if current not in graph: | |
| raise RuntimeError(f"vcpkg depend-info referenced unknown port {current!r}") | |
| closure.add(current) | |
| queue.extend(graph[current].dependencies) | |
| return closure |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Meta/Utils/vcpkg_deps.py` around lines 198 - 207, Update dependency_closure
to handle dependencies missing from graph without raising an unhandled KeyError:
when processing current, verify it exists in graph before accessing
graph[current].dependencies, and provide contextual failure handling for the
absent port while preserving closure tracking.
| def acquire_build_tokens(fifo_fd: int, pool: int, heavy: bool) -> int: | ||
| # For light nodes, a single token is sufficient, so they get dispatched as soon as there's one free and run at whatever parallelism is there. | ||
| # heavy nodes need guaranteed available compute to not thrash on the build, so they hold off until there are enough tokens to guarantee a parallel build. | ||
| held = len(os.read(fifo_fd, 1)) | ||
| if not heavy: | ||
| return held | ||
|
|
||
| target = max(2, pool // 4) | ||
| while held < target: | ||
| held += len(os.read(fifo_fd, 1)) | ||
|
|
||
| cap = max(target, pool - 8) | ||
| os.set_blocking(fifo_fd, False) | ||
| try: | ||
| while held < cap: | ||
| extra = os.read(fifo_fd, cap - held) | ||
| if not extra: | ||
| break | ||
| held += len(extra) | ||
| except BlockingIOError: | ||
| pass | ||
| finally: | ||
| os.set_blocking(fifo_fd, True) | ||
| return held |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | 🏗️ Heavy lift
Heavy nodes can deadlock on partial token acquisition.
A heavy node takes one token at line 337, then blocks at line 343 until it reaches target. It never releases the tokens it already holds. If enough heavy nodes start together, every token in the pool is held by a node that is still waiting for more, and no node can make progress.
The case is reachable. With pool = 8, target = max(2, 8 // 4) = 2. If ninja starts 8 heavy nodes, each one reads a single token and then blocks forever. The situation is more likely when jobs is None: line 492 sets tokens = os.cpu_count(), while ninja's default parallelism is higher than the core count, so more nodes run than there are tokens.
Make the acquisition all-or-nothing. Return the partially held tokens and retry, or serialize heavy acquisition behind a separate lock.
🔒 One approach: release and retry instead of holding a partial set
def acquire_build_tokens(fifo_fd: int, pool: int, heavy: bool) -> int:
held = len(os.read(fifo_fd, 1))
if not heavy:
return held
target = max(2, pool // 4)
- while held < target:
- held += len(os.read(fifo_fd, 1))
+ # Drain what is available without blocking; if we cannot reach the target,
+ # give everything back so another node can make progress, then retry.
+ while held < target:
+ os.set_blocking(fifo_fd, False)
+ try:
+ while held < target:
+ extra = os.read(fifo_fd, target - held)
+ if not extra:
+ break
+ held += len(extra)
+ except BlockingIOError:
+ pass
+ finally:
+ os.set_blocking(fifo_fd, True)
+ if held < target:
+ os.write(fifo_fd, b"x" * held)
+ held = 0
+ time.sleep(0.5)
+ held = len(os.read(fifo_fd, 1))Note: this sketch needs an import time and still allows livelock under contention. A dedicated heavy-node lock, so that only one heavy node acquires tokens at a time, is the more robust fix.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Meta/Utils/vcpkg_deps.py` around lines 334 - 357, Update acquire_build_tokens
so heavy-node acquisition is all-or-nothing and cannot leave tokens stranded
while waiting for target; use a dedicated lock to serialize the heavy
acquisition path, then acquire the required tokens before allowing other heavy
nodes to contend. Preserve the existing light-node behavior and nonblocking cap
logic, and ensure the lock is released on every exit.
| tokens = int(jobs) if jobs else (os.cpu_count() or 4) | ||
| token_fifo = deps_dir / ".tokens" | ||
| token_fifo.unlink(missing_ok=True) | ||
| os.mkfifo(token_fifo) | ||
| fifo_fd = os.open(token_fifo, os.O_RDWR) | ||
| os.write(fifo_fd, b"x" * tokens) | ||
|
|
||
| ninja_environment = os.environ.copy() | ||
| ninja_environment["LADYBIRD_VCPKG_TOKEN_FIFO"] = str(token_fifo) | ||
| ninja_environment["LADYBIRD_VCPKG_TOKEN_POOL"] = str(tokens) | ||
|
|
||
| try: | ||
| ninja_arguments = ["ninja", "-C", str(deps_dir)] | ||
| if jobs: | ||
| ninja_arguments.extend(["-j", jobs]) | ||
| subprocess.check_call(ninja_arguments, env=ninja_environment) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
The ninja job count and the token pool disagree when jobs is unset.
If jobs is None, line 492 sizes the pool at os.cpu_count(), and line 504 starts ninja with no -j. Ninja then defaults to more than the core count, typically cores plus two. More nodes run concurrently than there are tokens. Every extra node blocks on the first os.read in acquire_build_tokens, and the risk of the heavy-node deadlock described on lines 334-357 rises.
Pass the resolved job count to ninja in both cases.
♻️ Proposed change
tokens = int(jobs) if jobs else (os.cpu_count() or 4)
@@
try:
- ninja_arguments = ["ninja", "-C", str(deps_dir)]
- if jobs:
- ninja_arguments.extend(["-j", jobs])
+ ninja_arguments = ["ninja", "-C", str(deps_dir), "-j", str(tokens)]
subprocess.check_call(ninja_arguments, env=ninja_environment)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| tokens = int(jobs) if jobs else (os.cpu_count() or 4) | |
| token_fifo = deps_dir / ".tokens" | |
| token_fifo.unlink(missing_ok=True) | |
| os.mkfifo(token_fifo) | |
| fifo_fd = os.open(token_fifo, os.O_RDWR) | |
| os.write(fifo_fd, b"x" * tokens) | |
| ninja_environment = os.environ.copy() | |
| ninja_environment["LADYBIRD_VCPKG_TOKEN_FIFO"] = str(token_fifo) | |
| ninja_environment["LADYBIRD_VCPKG_TOKEN_POOL"] = str(tokens) | |
| try: | |
| ninja_arguments = ["ninja", "-C", str(deps_dir)] | |
| if jobs: | |
| ninja_arguments.extend(["-j", jobs]) | |
| subprocess.check_call(ninja_arguments, env=ninja_environment) | |
| tokens = int(jobs) if jobs else (os.cpu_count() or 4) | |
| token_fifo = deps_dir / ".tokens" | |
| token_fifo.unlink(missing_ok=True) | |
| os.mkfifo(token_fifo) | |
| fifo_fd = os.open(token_fifo, os.O_RDWR) | |
| os.write(fifo_fd, b"x" * tokens) | |
| ninja_environment = os.environ.copy() | |
| ninja_environment["LADYBIRD_VCPKG_TOKEN_FIFO"] = str(token_fifo) | |
| ninja_environment["LADYBIRD_VCPKG_TOKEN_POOL"] = str(tokens) | |
| try: | |
| ninja_arguments = ["ninja", "-C", str(deps_dir), "-j", str(tokens)] | |
| subprocess.check_call(ninja_arguments, env=ninja_environment) |
🧰 Tools
🪛 ast-grep (0.45.0)
[error] 506-506: Use of unsanitized data to create processes
Context: subprocess.check_call(ninja_arguments, env=ninja_environment)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(os-system-unsanitized-data)
🪛 Ruff (0.16.0)
[error] 507-507: subprocess call: check for execution of untrusted input
(S603)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Meta/Utils/vcpkg_deps.py` around lines 492 - 507, Update the ninja invocation
in the token-pool setup to always pass the resolved `tokens` job count,
including when `jobs` is unset. Replace the conditional `-j` argument handling
in the `ninja_arguments` construction while preserving the existing resolved
count and environment configuration.
2fbbec3 to
4878c72
Compare
This is a massive waste of time if run serially, as all packages waste significant time just downloading and configuring things. Run them in parallel with a little pre-knowledge to go from 16m build time to 3m (caches all cold, all sources already downloaded for both).
4878c72 to
b59bcac
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
Meta/Utils/vcpkg_deps.py (2)
520-522: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument or guard the POSIX-only dependency.
build_dependenciesneedsfcntlat line 520 andos.mkfifoat line 557. Both are POSIX-only. The function-scope import hides this requirement from the reader and from the caller. Add a short comment or an explicit platform check at the top of the function.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Meta/Utils/vcpkg_deps.py` around lines 520 - 522, Make the POSIX-only requirement explicit at the start of build_dependencies: add a concise platform guard or documentation comment covering both fcntl and os.mkfifo before the function uses them. Keep the existing locking and FIFO behavior unchanged.
413-426: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRead
.weightonce.Lines 421-423 stat and read the same file in two steps and repeat the path expression. Also initialize
heavyoutside the block so line 432 does not depend onheldbeing non-zero for the name to be bound.♻️ Proposed refactor
fifo_fd = None held = 0 + heavy = False token_fifo = environment.get("LADYBIRD_VCPKG_TOKEN_FIFO") if token_fifo and Path(token_fifo).exists(): pool = ( int(environment.get("LADYBIRD_VCPKG_TOKEN_POOL", "0")) or (os.cpu_count() or 2) + 2 ) - heavy = (node_dir / ".weight").exists() and ( - node_dir / ".weight" - ).read_text().strip() == "heavy" + weight_path = node_dir / ".weight" + heavy = weight_path.is_file() and weight_path.read_text().strip() == "heavy"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Meta/Utils/vcpkg_deps.py` around lines 413 - 426, In the token FIFO setup around acquire_build_tokens, initialize heavy before the token_fifo conditional, then read node_dir / ".weight" once and derive whether it equals "heavy" without repeating the path or performing separate existence and read operations. Preserve the existing concurrency behavior while ensuring heavy is always defined for later use.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Meta/Utils/vcpkg_deps.py`:
- Around line 86-89: Update overlay_triplets_dir to validate the preset through
the same supported-preset handling used by the .get call near line 77, and raise
a clear error for unknown values that includes the supported preset names
instead of allowing a bare KeyError from PRESET_TRIPLET_DIRS[preset]. Preserve
the existing path construction for valid presets.
---
Nitpick comments:
In `@Meta/Utils/vcpkg_deps.py`:
- Around line 520-522: Make the POSIX-only requirement explicit at the start of
build_dependencies: add a concise platform guard or documentation comment
covering both fcntl and os.mkfifo before the function uses them. Keep the
existing locking and FIFO behavior unchanged.
- Around line 413-426: In the token FIFO setup around acquire_build_tokens,
initialize heavy before the token_fifo conditional, then read node_dir /
".weight" once and derive whether it equals "heavy" without repeating the path
or performing separate existence and read operations. Preserve the existing
concurrency behavior while ensuring heavy is always defined for later use.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 78df69f9-1aea-4ea0-ae3d-a3f546b52bbd
📒 Files selected for processing (2)
Meta/Utils/vcpkg_deps.pyMeta/ladybird.py
🚧 Files skipped from review as they are similar to previous changes (1)
- Meta/ladybird.py
| def overlay_triplets_dir(preset: str) -> Path: | ||
| return ( | ||
| LADYBIRD_SOURCE_DIR / "Meta" / "CMake" / "vcpkg" / PRESET_TRIPLET_DIRS[preset] | ||
| ) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Report an unknown preset with a clear error.
Line 77 tolerates an unknown preset through .get, but line 88 raises a bare KeyError. The user sees a traceback with no list of supported presets.
🛡️ Proposed fix
def overlay_triplets_dir(preset: str) -> Path:
+ if preset not in PRESET_TRIPLET_DIRS:
+ raise RuntimeError(
+ f"Unknown preset {preset!r}; expected one of {sorted(PRESET_TRIPLET_DIRS)}"
+ )
return (
LADYBIRD_SOURCE_DIR / "Meta" / "CMake" / "vcpkg" / PRESET_TRIPLET_DIRS[preset]
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def overlay_triplets_dir(preset: str) -> Path: | |
| return ( | |
| LADYBIRD_SOURCE_DIR / "Meta" / "CMake" / "vcpkg" / PRESET_TRIPLET_DIRS[preset] | |
| ) | |
| def overlay_triplets_dir(preset: str) -> Path: | |
| if preset not in PRESET_TRIPLET_DIRS: | |
| raise RuntimeError( | |
| f"Unknown preset {preset!r}; expected one of {sorted(PRESET_TRIPLET_DIRS)}" | |
| ) | |
| return ( | |
| LADYBIRD_SOURCE_DIR / "Meta" / "CMake" / "vcpkg" / PRESET_TRIPLET_DIRS[preset] | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Meta/Utils/vcpkg_deps.py` around lines 86 - 89, Update overlay_triplets_dir
to validate the preset through the same supported-preset handling used by the
.get call near line 77, and raise a clear error for unknown values that includes
the supported preset names instead of allowing a bare KeyError from
PRESET_TRIPLET_DIRS[preset]. Preserve the existing path construction for valid
presets.
|
Perhaps a silly question (without looking at a much of the python code) but is there pre-existing discussion on the vcpkg repo (issues or discussions) about parallelism? If there's a hidden flag to the vcpkg binary or some such it would be pretty silly to build our own graph to sort deps and build them in a safe way. |
They haven't even figured out how to download in parallel yet; there's also this discussion from 6 years ago that is "any updates" central: microsoft/vcpkg#19129 It don't exist Andrew :( |
|
That's... Surprising. Their vcpkg-tool surely has a graph of dependencies. Partitioning it into up to N moderately equally sized subgraphs shouldn't be a giant amount of effort. But Oki. I'll take a look at this to see if any of the vcpkg wrangling were doing in cmake is improperly mirrored later today then |
2 participants
Measured on two boxes (32x 9950x + 128GB@4800MT/s and 24x Ultra 9 275HX + 32GB @ 5600MT/s) plus icecc cluster with the two.