Conversation
Prepare one root eval config per command instead of rebuilding eval startup state for every file, and cache package roots inside ResolutionResult so format_package_uri no longer rebuilds them on hot paths. On release pcb build /Users/lenny/code/diodeinc/wurth --locked, progress improved from about 6.5k stderr lines in 35s to about 5.5k in 20s. At roughly the 4 minute mark the old build had emitted about 15.0k lines; this version had emitted about 27.3k. Sampling also moved the hotspot from EvalContextConfig::new/build_package_roots churn to Starlark parse/eval.
Keep a shared EvalSession for a build command and reset only per-root module state. This preserves the existing load_cache across root files so repeated stdlib and shared module loads stop being reparsed for every workspace member. On release pcb build /Users/lenny/code/diodeinc/wurth --locked, the build now finishes in 76.32s. The first 20s progressed from about 5.5k stderr lines on the previous commit to about 11.3k here. Live sampling no longer showed repeated load parsing; the next hotspot is format_package_uri path-prefix matching and symbol-file reads.
Cache a root-to-package map in ResolutionResult and format package URIs by walking parent directories instead of scanning every package root on each call. On release pcb build /Users/lenny/code/diodeinc/wurth --locked, wall time dropped from 76.32s to 26.10s. The first 20s progressed from about 11.3k stderr lines on the previous commit to about 26.2k here.
Build Performance
Measured with hyperfine. Times show median ±stddev. |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix prepared a fix for 1 of the 2 issues found in the latest run.
- ✅ Fixed: Removed manifest collection drops version corrections for import-free packages
- I restored full manifest iteration by including every workspace package
pcb.toml, so version corrections now apply even to import-free packages.
- I restored full manifest iteration by including every workspace package
Or push these changes by commenting:
@cursor push a4eb2ef987
Preview (a4eb2ef987)
diff --git a/crates/pcb-zen/src/auto_deps.rs b/crates/pcb-zen/src/auto_deps.rs
--- a/crates/pcb-zen/src/auto_deps.rs
+++ b/crates/pcb-zen/src/auto_deps.rs
@@ -52,7 +52,15 @@
let configured_kicad_versions = workspace_info.asset_dep_versions();
let index = CacheIndex::open()?;
- let mut manifests: Vec<_> = package_imports.keys().cloned().collect();
+ let mut manifest_set: HashSet<PathBuf> = package_imports.keys().cloned().collect();
+ for pkg in packages.values() {
+ let manifest_path = normalize_manifest_path(
+ &pkg.dir(&workspace_info.root).join("pcb.toml"),
+ &workspace_info.root,
+ );
+ manifest_set.insert(manifest_path);
+ }
+ let mut manifests: Vec<_> = manifest_set.into_iter().collect();
manifests.sort();
for pcb_toml_path in manifests {| if total_normalized > 0 { | ||
| workspace_info.reload()?; | ||
| if let Some(config) = root_config_update { | ||
| workspace_info.config = Some(config); |
There was a problem hiding this comment.
Stale in-memory configs after auto-deps phase
Medium Severity
Replacing workspace_info.reload() with per-package updates means packages modified by auto_deps but without branch deps won't have their in-memory pkg.config refreshed. The old reload() refreshed ALL packages from disk when any normalization occurred, catching auto_deps changes for all packages. Now only branch-pinned packages get updated. Since resolution reads pkg.config (in-memory) at line ~586, auto-added dependencies for non-branch-pinned packages are silently dropped from resolution.
Additional Locations (1)
There was a problem hiding this comment.
Bugbot Autofix determined this is a false positive.
This path is already covered because run_auto_deps still calls workspace_info.reload(), refreshing all package configs before dependency collection.
| let index = CacheIndex::open()?; | ||
| let manifests = collect_manifest_paths(workspace_root, packages, &package_imports); | ||
| let mut manifests: Vec<_> = package_imports.keys().cloned().collect(); | ||
| manifests.sort(); |
There was a problem hiding this comment.
Removed manifest collection drops version corrections for import-free packages
Low Severity
The deleted collect_manifest_paths function ensured all workspace member manifests were processed, including packages without .zen file imports. The replacement package_imports.keys().cloned().collect() only includes manifests that had scanned imports. Packages with no .zen files (or no imports) will no longer receive the workspace member version corrections performed by mutate_manifest_dependencies.
| .read_file(path) | ||
| .with_context(|| format!("failed to read {}", path.display()))?; | ||
|
|
||
| if content.trim().is_empty() { |
There was a problem hiding this comment.
curious why this is needed. I thought blank pcb.toml files should parse fine in the existing code path. is this for performance reasons as well?
There was a problem hiding this comment.
Yes not a correctness thing just performance, although I suspect this is pretty negligible
2 participants
Note
Medium Risk
Refactors dependency resolution and evaluation caching paths used by build/resolve, so mistakes could cause incorrect package root/url mapping or stale evaluation state across files. Changes are mostly internal but touch core resolution and build execution paths.
Overview
Speeds up builds in large workspaces by reusing dependency-resolution metadata and evaluation state instead of rebuilding it per file.
resolve_dependenciesnow takes an optionalinput_pathso the auto-deps scan can be scoped to a specific file/package (and callers are updated accordingly). Dependency resolution also adds cachedpackage_rootsand a reverseroot → URLmap (viaOnceLock/Arc) to avoid repeated recomputation and linear scans when formatting/resolvingpackage://URIs.Build execution is updated to prepare a shared
EvalContextConfig+EvalSession(PreparedBuildEval) and evaluate multiple.zenfiles viaeval_with_config_and_session, resetting only per-root state between runs.PcbTomlnow treats empty manifest files asdefault()configs, and tests are updated (plus a new regression test) to ensure branch pinning doesn’t clobber auto-added member deps.Written by Cursor Bugbot for commit 45f96d4. This will update automatically on new commits. Configure here.