Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(install): lifecycle script changes #8943

Merged
merged 40 commits into from
Mar 8, 2024

Conversation

dylan-conway
Copy link
Member

@dylan-conway dylan-conway commented Feb 16, 2024

What does this PR do?

  • Breaking change: if trustedDependencies exists in package.json, bun install will no longer pull from the default list and will only run scripts from the trustedDependencies list. If the list is empty, then no scripts will run. The default trusted dependencies list is only used when trustedDependencies does not exist in package.json.

  • --trust. This flags tells bun add to automatically trust the dependency and all it's dependencies, running their lifecycle scripts and adding to trustedDependencies in package.json.

  • warning for skipped lifecycle scripts

Screenshot 2024-03-06 at 11 26 43 PM

Also included in this pr is bun pm trusted. This command without any args will print the current trusted and untrusted dependencies with scripts:
Screenshot 2024-03-07 at 2 25 56 AM

If you provide package names, they will be added to trustedDependencies and their scripts will run. In the example above, running bun pm trust es5-ext will add this to your package.json:

...
trustedDependencies: [
    "es5-ext"
]
...

You can also pass --all to trust all available untrusted dependencies, and --default to list the default trusted dependencies list.

The breaking changes in this pr will be disabled until v1.1.0.

How did you verify your code works?

new and existing tests

Copy link
Contributor

github-actions bot commented Feb 16, 2024

@dylan-conway 1 files with test failures on bun-darwin-aarch64:

View test output

#1f33830bb01114b03d1e481f21269a852316922b

Copy link
Contributor

github-actions bot commented Feb 16, 2024

@dylan-conway 1 files with test failures on linux-x64:

View test output

#1f33830bb01114b03d1e481f21269a852316922b

Copy link
Contributor

github-actions bot commented Feb 16, 2024

@dylan-conway 1 files with test failures on linux-x64-baseline:

View test output

#1f33830bb01114b03d1e481f21269a852316922b

Copy link
Contributor

github-actions bot commented Feb 16, 2024

Copy link
Contributor

github-actions bot commented Feb 16, 2024

❌🪟 @dylan-conway, there are 12 test regressions on Windows x86_64

  • test\cli\install\bun-upgrade.test.ts
  • test\cli\install\migration\migrate.test.ts
  • test\cli\run\transpiler-cache.test.ts
  • test\js\bun\dns\resolve-dns.test.ts
  • test\js\bun\http\fetch-file-upload.test.ts
  • test\js\bun\http\bun-server.test.ts
  • test\js\bun\shell\shelloutput.test.ts
  • test\js\bun\shell\throw.test.ts
  • test\js\node\dns\node-dns.test.js
  • test\js\node\process\process-args.test.js
  • test\js\web\fetch\body-stream-excess.test.ts
  • test\js\web\fetch\body-stream.test.ts

Full Test Output

@dylan-conway dylan-conway marked this pull request as ready for review March 7, 2024 11:01
src/install/install.zig Outdated Show resolved Hide resolved
src/install/install.zig Outdated Show resolved Hide resolved
src/js_ast.zig Outdated
@@ -1458,6 +1458,23 @@ pub const E = struct {

return array;
}

/// debug.assert the array only contains strings before calling
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you make this an actual debug assert for this function. or uh, i see you do a runtime check for that... so is it an assertion or? i'd like osmething better than this comment

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved the debug assert inside this function. Also did the same for alphabetizeProperties. Originally I had the runtime check for extra safety

// 0 - package from old lockfile, needs update
// 1 - does not have install scripts
// 2 - has install scripts
__has_install_script: u8,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

instead of a magic number, can this be an enum(u8). it would also be nice to add this into the comment on line 4709 when we bump the lockfile version, we should reorder this to:

also proper documentation comments are /// but that is less relevant.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

switched to enum


await rm(join(packageDir, "node_modules", "electron", "preinstall.txt"), { force: true });

// lockfile should save evenn though there are no changes to trustedDependencies due to
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// lockfile should save evenn though there are no changes to trustedDependencies due to
// lockfile should save even though there are no changes to trustedDependencies due to

else => return,
else => {
if (comptime Environment.allow_assert) {
@panic("bad");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bad

Co-authored-by: Jarred Sumner <[email protected]>
@Jarred-Sumner Jarred-Sumner merged commit d37fbbd into main Mar 8, 2024
26 of 31 checks passed
@Jarred-Sumner Jarred-Sumner deleted the dylan/lifecycle-script-changes branch March 8, 2024 03:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bun install doesn't support file:. path in package.json dependencies
3 participants