-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
install: Detect cross-FS cache directory and find best one #12126
base: main
Are you sure you want to change the base?
Conversation
Co-authored-by: Dylan Conway <[email protected]>
Co-authored-by: Jarred Sumner <[email protected]> Co-authored-by: Dylan Conway <[email protected]>
…ixes in markdown syntax (#12025) Co-authored-by: dave caruso <[email protected]>
Co-authored-by: paperdave <[email protected]>
Co-authored-by: dylan-conway <[email protected]>
Co-authored-by: zackradisic <[email protected]> Co-authored-by: Jarred Sumner <[email protected]>
Co-authored-by: dylan-conway <[email protected]>
Co-authored-by: Jarred Sumner <[email protected]> Co-authored-by: dave caruso <[email protected]>
tmpname: [:0]const u8, | ||
) bool { | ||
// Make sure cachedir and cwd are in the same filesystem | ||
std.posix.renameatZ(dir_with_file_to_rename.fd, tmpname, dir_to_rename_file_to.fd, tmpname) catch return false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
None of these should be using std.posix
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and this doesn't test they're in the same filesystem. This tests it doesn't throw an error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and it shouldn't be in bun.sys unless it's reusable elsewhere
/// Invariants: | ||
/// | ||
/// - node_modules_folder_path is absolute path with no relative syntax | ||
pub fn findBestDirectoryInSameFileSystem( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can go in the package manager code
we don't need to do this elsewhere afaict
❌ @zackradisic, your commit has failing tests :( 💪 1 failing tests Darwin AARCH64
💻 1 failing tests Darwin x64 baseline
💻 1 failing tests Darwin x64
🐧💪 1 failing tests Linux AARCH64
🐧🖥 1 failing tests Linux x64 baseline
🐧🖥 1 failing tests Linux x64
🪟💻 3 failing tests Windows x64 baseline
🪟💻 3 failing tests Windows x64
|
What does this PR do?
WIP need tests
Right now in the package manager, on initialization, we select an install cache directory and a temp directory that are on the same filesystem. This is because copy/moving files between filesystems is slow.
However, we don't ensure that the cache directory and the user's project directory (directory where
package.json
is) are on the same file system. If this is the case, thenbun install
becomes very slow.This PR detects if the cache directory is on another filesystem from the project directory. If that is the case, then we either: