Improves reporting#310
Conversation
✅ Deploy Preview for yarn-v6 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix prepared fixes for both issues found in the latest run.
- ✅ Fixed: Disabled skip caches build hash
- Excluded BuildSkip::Disabled from persists_build_state() to prevent caching disabled packages so builds run when scripts are re-enabled.
- ✅ Fixed: Skip-build hides disabled warnings
- Added report_disabled_build_scripts() before conditional build phase so warnings appear even with --mode=skip-build.
Or push these changes by commenting:
@cursor push 5ae5d07cb3
Preview (5ae5d07cb3)
diff --git a/packages/zpm/src/build.rs b/packages/zpm/src/build.rs
--- a/packages/zpm/src/build.rs
+++ b/packages/zpm/src/build.rs
@@ -78,7 +78,7 @@
impl BuildRequest {
fn persists_build_state(&self) -> bool {
- matches!(self.build_step, BuildStep::Commands(_) | BuildStep::Skip(BuildSkip::Disabled))
+ matches!(self.build_step, BuildStep::Commands(_))
}
pub async fn run(self, project: &Project, hash: Hash64) -> Result<ScriptResult, Error> {
@@ -88,13 +88,6 @@
},
BuildStep::Skip(BuildSkip::Disabled) => {
- crate::report::if_active(|report| {
- report.warn(format!(
- "{} lists build scripts, but its build has been explicitly disabled through configuration.",
- self.locator.to_print_string(),
- ));
- });
-
return Ok(ScriptResult::new_success());
},
diff --git a/packages/zpm/src/install.rs b/packages/zpm/src/install.rs
--- a/packages/zpm/src/install.rs
+++ b/packages/zpm/src/install.rs
@@ -961,6 +961,19 @@
}
}
+ fn report_disabled_build_scripts(&self, build_requests: &build::BuildRequests) {
+ crate::report::if_active(|report| {
+ for request in &build_requests.entries {
+ if matches!(request.build_step, build::BuildStep::Skip(build::BuildSkip::Disabled)) {
+ report.warn(format!(
+ "{} lists build scripts, but its build has been explicitly disabled through configuration.",
+ request.locator.to_print_string(),
+ ));
+ }
+ }
+ });
+ }
+
pub async fn link_and_build(mut self, project: &mut Project) -> Result<InstallResult, Error> {
self.report_package_extension_diagnostics(project).await;
@@ -1013,6 +1026,8 @@
self.install_state.packages_by_location
= link_result.packages_by_location;
+ self.report_disabled_build_scripts(&link_result.build_requests);
+
project.attach_install_state(self.install_state)?;
if !self.skip_lockfile_update {You can send follow-ups to the cloud agent here.
Reviewed by Cursor Bugbot for commit 749479e. Configure here.
| = self.get_hash(project, &req.locator); | ||
|
|
||
| if !force_rebuild { | ||
| if req.persists_build_state() && !force_rebuild { |
There was a problem hiding this comment.
Disabled skip caches build hash
High Severity
After an install with scripts disabled, a package’s disabled skip is recorded in build state using the same dependency-tree hash as a real build. Re-enabling scripts can make the next install treat that package as already built and skip running its build commands even though they never ran.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit 749479e. Configure here.
|
|
||
| let build_result | ||
| = async_section("Building the project", build_future).await?; | ||
| = async_section("Building packages", build_future).await?; |
There was a problem hiding this comment.
Skip-build hides disabled warnings
Medium Severity
Disabled-build script warnings are only emitted inside the build manager. When skip_build is set (for example --mode=skip-build), that phase is skipped entirely, so packages with build scripts and scripts disabled no longer get the warning that report_disabled_build_scripts used to print at the start of link_and_build.
Reviewed by Cursor Bugbot for commit 749479e. Configure here.
⏱️ Benchmark Resultsgatsby install-full-cold
📊 Raw benchmark data (gatsby install-full-cold)Base times: 4.512s, 4.461s, 4.520s, 4.593s, 4.584s, 4.505s, 4.500s, 4.527s, 4.520s, 4.499s, 4.499s, 4.467s, 4.379s, 4.559s, 4.489s, 4.463s, 4.588s, 4.499s, 4.538s, 4.360s, 4.546s, 4.559s, 4.545s, 4.535s, 4.507s, 4.370s, 4.540s, 4.557s, 4.520s, 4.461s Head times: 4.509s, 4.634s, 4.492s, 4.521s, 4.366s, 4.466s, 4.420s, 4.599s, 4.452s, 4.410s, 4.474s, 4.554s, 4.434s, 4.509s, 4.499s, 4.569s, 4.577s, 4.557s, 4.503s, 4.599s, 4.570s, 4.585s, 4.509s, 4.558s, 4.450s, 4.516s, 4.550s, 4.578s, 4.492s, 4.448s gatsby install-cache-only
📊 Raw benchmark data (gatsby install-cache-only)Base times: 1.350s, 1.360s, 1.345s, 1.356s, 1.364s, 1.373s, 1.368s, 1.368s, 1.365s, 1.371s, 1.353s, 1.364s, 1.367s, 1.336s, 1.363s, 1.344s, 1.356s, 1.377s, 1.374s, 1.351s, 1.339s, 1.367s, 1.361s, 1.348s, 1.363s, 1.333s, 1.348s, 1.341s, 1.361s, 1.377s Head times: 1.347s, 1.358s, 1.351s, 1.354s, 1.360s, 1.370s, 1.376s, 1.365s, 1.361s, 1.353s, 1.388s, 1.352s, 1.337s, 1.381s, 1.366s, 1.378s, 1.354s, 1.362s, 1.928s, 1.361s, 1.363s, 1.382s, 1.364s, 1.360s, 1.372s, 1.351s, 1.366s, 1.362s, 1.356s, 1.367s gatsby install-cache-and-lock (warm, with lockfile)
📊 Raw benchmark data (gatsby install-cache-and-lock (warm, with lockfile))Base times: 0.362s, 0.357s, 0.353s, 0.366s, 0.352s, 0.360s, 0.355s, 0.361s, 0.372s, 0.359s, 0.358s, 0.361s, 0.360s, 0.352s, 0.354s, 0.360s, 0.355s, 0.365s, 0.353s, 0.351s, 0.354s, 0.357s, 0.357s, 0.372s, 0.369s, 0.354s, 0.364s, 0.353s, 0.353s, 0.363s Head times: 0.363s, 0.366s, 0.352s, 0.364s, 0.366s, 0.363s, 0.369s, 0.366s, 0.359s, 0.370s, 0.377s, 0.364s, 0.362s, 0.366s, 0.368s, 0.367s, 0.375s, 0.369s, 0.361s, 0.368s, 0.371s, 0.364s, 0.373s, 0.360s, 0.362s, 0.360s, 0.370s, 0.368s, 0.364s, 0.365s |
1 participant
Our logging had two issues:
The "build skipped" messages were sent before attempting the build. As a result they were not truly part of the build, their hashes weren't stored anywhere, and they consistently showed up at every install.
Both those messages and package extension messages weren't part of any log section, making them have a rather jarring style.
The "lockfile would have changed" messages were incorrectly reporting the lockfile as being created (even when it already existed and was merely out-of-sync), and didn't show the actual changes. It made it a little difficult to figure out why changes were needed without running the command locally.
Note
Medium Risk
Changes touch install/build state persistence and immutable lockfile failure paths, which affect CI and repeat-install behavior, though logic is mostly reporting and clearer error classification.
Overview
Install reporting is tightened so build skips, extension warnings, and immutable lockfile failures read more clearly and behave correctly across repeated installs.
Build pipeline: Packages with disabled scripts (or incompatible optional deps) now enqueue a
BuildStep::Skipinstead of being dropped before the build phase. Disabled-script warnings emit during Building packages, and build state is persisted for disabled skips so the same warning does not repeat every install until arebuild. The install progress section label is Building packages.Package extensions: Extension diagnostics are collected and printed under a Package extension diagnostics section instead of as loose lines.
Immutable lockfiles:
write_lockfilecompares on-diskyarn.lockto the generated content, splits errors into creation vs modification, and attaches a colorized unified diff (viasimilar, line-capped with a timeout) under Lockfile changes before failing.Reporting API: Log attachments generalize from file paths only to titled formatted blocks (
add_log_format), used for the lockfile diff.Reviewed by Cursor Bugbot for commit 10b0047. Bugbot is set up for automated code reviews on this repo. Configure here.