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

Allow relative paths in cpAsync again #9184

Merged
merged 4 commits into from
Mar 4, 2024

Conversation

argosphil
Copy link
Contributor

@argosphil argosphil commented Mar 1, 2024

What does this PR do?

Fixes #9024 .

A recent commit in #8649 changed cpAsync to use

const fd = switch (Syscall.openatOSPath(bun.invalid_fd, src, open_flags, 0)) {

rather than

const fd = switch (Syscall.open(src, open_flags, 0)) {

The new code doesn't accept relative paths, which caused the issue.

This patch uses the CWD file descriptor instead.

  • Code changes

How did you verify your code works?

Will add tests in a bit.

There is a test.

@gvilums
Copy link
Contributor

gvilums commented Mar 1, 2024

I'm not quite sure that this is right - local testing shows that copying works just fine with relative paths. I think the issue is actually a combination of two separate issues:

  • when copying to a directory that already exists, we throw an error
  • the error we throw has a misleading error message

running

console.log(await fs.promises.cp("abc", "def", { recursive: true }));

(assuming abc exists and contains foo.txt, and def doesn't exist) works, but if def exists it fails with

EBADF: Bad file descriptor
   errno: -9
 syscall: "open"
   path: "abc"

This seems to be the error we're seeing. I don't think this is related to relative path processing, but rather the fact that we don't correctly handle the case where the directory already exists.

Node transparently does the copy if the directory already exists.

@gvilums
Copy link
Contributor

gvilums commented Mar 1, 2024

Disregard the above, I mixed up which use of openatOSPath you meant 😅

@argosphil
Copy link
Contributor Author

Disregard the above, I mixed up which use of openatOSPath you meant 😅

Thank you, I was confused myself and really shouldn't have opened this without a test. There is one now. Criticism and nitpicking appreciated :-)

@Jarred-Sumner Jarred-Sumner merged commit 4257457 into oven-sh:main Mar 4, 2024
28 of 31 checks passed
@Jarred-Sumner
Copy link
Collaborator

Thank you

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.

EBADF: Bad file descriptor. errno: -9
3 participants