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

Use copy instead of rename if temp directory is on a different device #17

Merged

Conversation

bryanwood
Copy link
Contributor

Running npx "@enhance/create@latest" ./myproject -y on Windows when the temp directory isn't on the same disk/device as the project directory fails with Error: EXDEV: cross-device link not permitted.

Using copySync instead of renameSync fixes the scenario but I'm not sure if the change in this PR is a good idea or not.

@macdonst macdonst self-requested a review July 27, 2023 14:32
Copy link
Contributor

@macdonst macdonst left a comment

Choose a reason for hiding this comment

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

Hey @bryanwood thanks for the PR. I'm glad you caught this bug. I'm away from my Windows box right now so I can't fully test this but I have left some comments on the PR that I think will get this in a state in which we can merge.

create-project.js Outdated Show resolved Hide resolved
create-project.js Outdated Show resolved Hide resolved
create-project.js Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@bryanwood bryanwood marked this pull request as ready for review July 30, 2023 02:37
@bryanwood
Copy link
Contributor Author

Have addressed the comments and learnt today that node has had APIs for doing recursive directory copying for a while now 😅.

There's another PR #18 that makes the tests run on Windows.

@macdonst
Copy link
Contributor

@bryanwood this is great, thanks for making those changes. I merged #18 into main. Can you pull those changes into this branch? That way all the tests will run clean on GitHub.

@bryanwood
Copy link
Contributor Author

Looks like cpSync is only in node >= 16.7

https://nodejs.org/api/fs.html#fscpsyncsrc-dest-options

@macdonst
Copy link
Contributor

@bryanwood I've pushed a new branch https://github.com/enhance-dev/create/tree/use-copy-when-different-filesystem-roots which has all your commits plus my own implementation of cpSync. Can you pull down the branch and test it on your machine? Seems to be working on mine and in GH Actions but I'd love for the original reporter to validate the fix. Once that's done will commit and cut a new release.

@bryanwood
Copy link
Contributor Author

Have tested this locally and it works ^^.

@macdonst macdonst merged commit 6d9ce87 into enhance-dev:main Aug 3, 2023
11 of 20 checks passed
@macdonst
Copy link
Contributor

macdonst commented Aug 3, 2023

@bryanwood thanks for the confirmation. I've pushed the code from my branch with the node 14 copy sync implementation and released an @enhance/create v3.1.3 to include the fix.

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.

2 participants