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

Open
wants to merge 53 commits into
base: main
Choose a base branch
from
Open

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

github-actions bot commented Apr 11, 2024

Copy link

github-actions bot commented Apr 11, 2024

@ghiscoding
Copy link

I'm assuming this PR should probably reference #9716

Copy link

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

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

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.

A few comments

test/js/bun/shell/commands/cp.test.ts Outdated Show resolved Hide resolved
test/js/bun/shell/commands/dirname.test.ts Outdated Show resolved Hide resolved
test/js/bun/shell/commands/exit.test.ts 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 Outdated Show resolved Hide resolved
src/shell/interpreter.zig Outdated Show resolved Hide resolved
@Jarred-Sumner
Copy link
Collaborator

@zackradisic zackradisic requested a review from gvilums May 8, 2024 10:46
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.

None yet

4 participants