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

Bun install files permission #14468

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

AielloChan
Copy link
Contributor

What does this PR do?

Try fix #14467

  • Documentation or TypeScript types (it's okay to leave the rest blank in this case)
  • Code changes

How did you verify your code works?

I just test it on my local machine. I'll write a test for it.

  • I checked the lifetime of memory allocated to verify it's (1) freed and (2) only freed when it should be
  • I included a test for the new code, or an existing test covers it
  • JSValue used outside outside of the stack is either wrapped in a JSC.Strong or is JSValueProtect'ed
  • I wrote TypeScript/JavaScript tests and they pass locally (bun-debug test test-file-name.test)

@AielloChan AielloChan force-pushed the fix/bun-install-files-permission branch 2 times, most recently from 00028ed to a3f3a35 Compare October 10, 2024 14:07
@AielloChan AielloChan force-pushed the fix/bun-install-files-permission branch from a3f3a35 to b9c0aee Compare October 10, 2024 16:36
@Jarred-Sumner
Copy link
Collaborator

Jarred-Sumner commented Oct 10, 2024

Thank you for highlighting that this is an issue - but this approach is going to cause a meaningful performance regression by adding an extra loop over all the files. We can make this work without regressing on performance by setting the mode during extraction without adding an extra pass over the input. I'll defer to @dylan-conway on this for what the behavior should be to align with npm/pnpm/yarn

@AielloChan
Copy link
Contributor Author

AielloChan commented Oct 11, 2024

Thank you for highlighting that this is an issue - but this approach is going to cause a meaningful performance regression by adding an extra loop over all the files. We can make this work without regressing on performance by setting the mode during extraction without adding an extra pass over the input. I'll defer to @dylan-conway on this for what the behavior should be to align with npm/pnpm/yarn

Oh, that will be great! I'm new to zig and bun, just try to fix my issue. Know that chmod at unpack stage will be the best practice, but I don't know how to do it, so just try to implement with another approach.

Wish we can ASAP to fix this, cause I can't use bun to install packages, thx ❤️.

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 ensure all files has at least read permission
2 participants