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

feat: Add cp builtin to shell for Windows #10174

Merged
merged 55 commits into from
May 9, 2024
Merged

feat: Add cp builtin to shell for Windows #10174

merged 55 commits into from
May 9, 2024

Conversation

zackradisic
Copy link
Contributor

What does this PR do?

This adds the cp shell builtin to Bun shell

Not all options are supported so it is only enabled on Windows right now

How did you verify your code works?

  • 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)

Copy link
Contributor

github-actions bot commented Apr 11, 2024

Copy link
Contributor

github-actions bot commented Apr 11, 2024

Copy link
Contributor

github-actions bot commented Apr 11, 2024

@ghiscoding
Copy link

I'm assuming this PR should probably reference #9716

Copy link
Contributor

github-actions bot commented Apr 11, 2024

@gvilums gvilums self-requested a review April 11, 2024 20:29
src/bun.js/node/node_fs.zig Outdated Show resolved Hide resolved
Copy link
Contributor

github-actions bot commented Apr 12, 2024

✅ lint failures have been resolved. Thank you.

#3330669d76aa6bdbd350821af487f3ec101f7007

@Jarred-Sumner
Copy link
Collaborator

I don't think we can ship this without cp -r

Copy link
Collaborator

@Jarred-Sumner Jarred-Sumner left a comment

Choose a reason for hiding this comment

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

This segfaults when copying directories.

That means:

  1. The automated tests are not nearly comprehensive enough
  2. You didn't manually test copying directories

Please test your code both manually and through automated means.

PS C:\bun> bun-10174 exec 'cp -R node_modules C:\temp\node_mods'
Segmentation fault at address 0xb0
???:?:?: 0x7ff6b7deebc3 in ??? (bun-10174.exe)
???:?:?: 0x7ffd56de257c in ??? (KERNEL32.DLL)
???:?:?: 0x7ffd5840aa47 in ??? (ntdll.dll)

@Jarred-Sumner
Copy link
Collaborator

many test failures

@Jarred-Sumner
Copy link
Collaborator

@zackradisic zackradisic requested a review from gvilums May 8, 2024 10:46
test/js/bun/shell/test_builder.ts Outdated Show resolved Hide resolved
test/js/bun/shell/test_builder.ts Show resolved Hide resolved
test/js/bun/shell/bunshell-default.test.ts Show resolved Hide resolved
src/shell/subproc.zig Outdated Show resolved Hide resolved
src/shell/interpreter.zig Show resolved Hide resolved
src/shell/interpreter.zig Outdated Show resolved Hide resolved
src/shell/interpreter.zig Show resolved Hide resolved
// There is only one path buffer for both paths. 2 extra bytes are the nulls at the end of each
bun.default_allocator.free(this.src.ptr[0 .. this.src.len + this.dest.len + 2]);

bun.destroy(this);
Copy link
Collaborator

Choose a reason for hiding this comment

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

not sure why this is using bun.destroy / bun.new instead of usingnamespace bun.New and ThisSingleTask.new / this.destroy

const dest = args.dest.osPath(@ptrCast(&dest_buf));

if (Environment.isWindows) {
const attributes = windows.GetFileAttributesW(src);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm surprised we don't have a helper for this

@Jarred-Sumner Jarred-Sumner merged commit 4b581b0 into main May 9, 2024
41 of 52 checks passed
@Jarred-Sumner Jarred-Sumner deleted the zack/shell-cp branch May 9, 2024 00:01
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.

4 participants