fix(global): preserve build approvals and dlx behavior#10492
fix(global): preserve build approvals and dlx behavior#10492
Conversation
preserve global workspace manifest settings and merge allowBuilds during global installs avoid dlx allow-build conflicts when dangerouslyAllowAllBuilds is enabled Closes pnpm#10121 Closes pnpm#8891
adjust the ignored builds hint to include -g for global runs add coverage for global and non-global hint rendering
|
Note These changes do not touch |
There was a problem hiding this comment.
Pull request overview
This PR fixes issues related to preserving global build approvals and enabling pnpm dlx to work with dangerouslyAllowAllBuilds. The changes ensure that global workspace manifests are not unnecessarily rewritten and that build approval configurations are properly merged rather than replaced.
Changes:
- Conditional workspace manifest updates to skip rewriting when performing global operations
- Merge existing
allowBuildsconfigurations when adding new packages with--allow-buildflag - Add global flag parameter to
IgnoredBuildsErrorto provide correct hint for global installs - Allow
pnpm dlxto function whendangerouslyAllowAllBuildsis enabled
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| pnpm/test/install/global.ts | Added tests to verify global workspace manifests are preserved and allowBuilds are merged correctly |
| pnpm/test/dlx.ts | Added test to verify dlx works with dangerouslyAllowAllBuilds enabled |
| pkg-manager/plugin-commands-installation/src/remove.ts | Added conditional check to skip workspace manifest updates for global operations |
| pkg-manager/plugin-commands-installation/src/recursive.ts | Pass global flag to IgnoredBuildsError for proper hint messages |
| pkg-manager/plugin-commands-installation/src/installDeps.ts | Added conditional checks to skip workspace manifest updates for global operations |
| pkg-manager/plugin-commands-installation/src/add.ts | Spread existing allowBuilds to preserve them when adding new packages |
| pkg-manager/core/test/install/ignoredBuildsHint.ts | Tests for global flag in IgnoredBuildsError hint messages |
| pkg-manager/core/src/install/index.ts | Updated IgnoredBuildsError to accept global option and include -g flag in hint |
| exec/plugin-commands-script-runners/test/utils/index.ts | Added dangerouslyAllowAllBuilds to test defaults |
| exec/plugin-commands-script-runners/src/dlx.ts | Handle dangerouslyAllowAllBuilds by conditionally setting allowBuilds to undefined |
| .changeset/tiny-shoes-fold.md | Changeset documentation for the fixes |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| await execPnpm(['add', '--global', '--allow-build=esbuild', 'is-positive'], { env }) | ||
|
|
||
| const updatedManifest = readYamlFile<{ allowBuilds?: Record<string, boolean> }>(workspaceManifestPath) |
There was a problem hiding this comment.
The type annotation for the generic parameter should be Record<string, boolean | string> instead of Record<string, boolean> to match the actual type of allowBuilds used throughout the codebase. While boolean values are valid, the config also supports string values for version-specific specifications.
| const updatedManifest = readYamlFile<{ allowBuilds?: Record<string, boolean> }>(workspaceManifestPath) | |
| const updatedManifest = readYamlFile<{ allowBuilds?: Record<string, boolean | string> }>(workspaceManifestPath) |
| return resolved.id | ||
| })) | ||
| const allowBuildsFromCli = opts.allowBuild ?? [] | ||
| const shouldAllowBuilds = !opts.dangerouslyAllowAllBuilds |
There was a problem hiding this comment.
The variable name shouldAllowBuilds is misleading. It's set to !opts.dangerouslyAllowAllBuilds, which means when builds should be unrestricted (dangerouslyAllowAllBuilds=true), the variable is false. Consider renaming to shouldRestrictBuilds or strictBuildRestrictions to make the logic clearer.
There was a problem hiding this comment.
this boolean name doesn't make sense.
|
waiting for an actual maintainer to tell me if I should go ahead with Copilot's suggestions 😄 |
|
any clue as to why certain tests fail on some Node versions but succeed on others ? 😅 |
zkochan
left a comment
zkochan
left a comment
There was a problem hiding this comment.
I'd expect two separate PRs but these changes don't really seem to make sense to me.
so one PR for the build approval preservation and another PR to fix the dlx + dangerouslyAllowAllBuilds issue ? just to be sure I don't misinterpret
as in the code change themselves or why this PR exists in the first place ? |
Important
Full disclosure : Most of the code here has been written by GPT 5.2 Codex.
Please review the changes to see if there is any glaring mistake or flaw in the implementation 🙏
Summary
pnpm dlxwhendangerouslyAllowAllBuildsis enabledDetails
pnpm approve-builds -g(after apnpm up -L -gfor instance), any existing entry inallowBuildswon't be overwritten or just removed from either the globalpnpm-workspace.yamlorpnpmkey in the globalpackage.jsonpnpm config set dangerouslyAllowAllBuilds truefollowed by apnpm dlxcommand, we will no longer see theCONFIG_CONFLICT_BUILT_DEPENDENCIESerrorIgnoredBuildsErrorwill indicate to runpnpm approve-buildswith the-gflag (was that a regression of v11 ? I believe it was present before in v10 iirc)Issues
Closes #10121
Closes #8891