fix: generate command with running cross-device by Cherry · Pull Request #3403 · cloudflare/workers-sdk

Cherry · 2023-06-04T18:24:31Z

What this PR solves / how to test:

When using Windows (and maybe other operating systems), a renameSync can throw a Error: EXDEV: cross-device link not permitted error when renaming across drives. This is very common on Windows installs that may have Windows on C:/, but their user files on another drive like E:/.

This PR resolves that by first trying the renameSync (will be much faster than copying files on the same device, so worth doing first), but then catching the EXDEV error and handling it specifically afterwards.

For tests, I don't really know how to properly approach testing this cross-device behaviour if I'm honest - it'd need another filesystem (maybe mocked?) on some other virtual device - and most generate tests appear to be skipped right now anyway. If that's something folks would like to see, I'm going to need some guidance/examples on how best to do that testing. I ran manual tests locally on my Windows 10 machine using generate project-name https://github.com/cloudflare/workers-sdk/templates/experimental/worker-rust, as well as in WSL and on my Macbook just to ensure the previous behaviour wasn't impacted across a few OSes, and all worked well.

Author has included the following, where applicable:

Tests
Changeset (Changeset guidelines)

Reviewer is to perform the following, as applicable:

Checked for inclusion of relevant tests
Checked for inclusion of a relevant changeset
Checked for creation of associated docs updates
Manually pulled down the changes and spot-tested

changeset-bot · 2023-06-04T18:24:33Z

🦋 Changeset detected

Latest commit: 97ad9fa

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
wrangler	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

github-actions · 2023-06-04T18:28:37Z

A wrangler prerelease is available for testing. You can install this latest build in your project with:

npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/5390610863/npm-package-wrangler-3403

You can reference the automatically updated head of this PR with:

npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/prs/3403/npm-package-wrangler-3403

Or you can use npx with this latest build directly:

npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/5390610863/npm-package-wrangler-3403 dev path/to/script.js

Additional artifacts:

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/5390610863/npm-package-cloudflare-pages-shared-3403

Note that these links will no longer work once the GitHub Actions artifact expires.

codecov · 2023-06-04T18:31:56Z

Codecov Report

Merging #3403 (97ad9fa) into main (607e032) will increase coverage by 0.03%.
The diff coverage is 77.77%.

@@            Coverage Diff             @@
##             main    #3403      +/-   ##
==========================================
+ Coverage   75.07%   75.11%   +0.03%     
==========================================
  Files         183      183              
  Lines       11083    11091       +8     
  Branches     2917     2918       +1     
==========================================
+ Hits         8321     8331      +10     
+ Misses       2762     2760       -2

Impacted Files	Coverage Δ
packages/wrangler/src/git-client.ts	`85.71% <77.77%> (+4.46%)`	⬆️

... and 1 file with indirect coverage changes

Cherry · 2023-06-09T17:09:55Z

It seems like fs.cp is used in other places in the codebase, so it should be safe to use here too. I've gone ahead and removed the original dependency on fs-extra.

petebacondarwin · 2023-06-14T19:43:23Z

packages/wrangler/src/git-client.ts

+		// @ts-expect-error non standard property on Error
+		if (err.code !== "EXDEV") {
+			logger.debug(err);
+			throw new Error(`Failed to find "${subdirectory}" in ${remote}`);


Perhaps include the original err in the new Error?

I'm happy to do this - the original code doesn't, so I assumed that was intentional, but if you think it's safe to just pass it through into the new error, can do!

Hmm. Oh I see. OK let's leave it as-is.
(Aside, I note that subdirectory could actually be undefined that would be a weird message!)

(Aside, I note that subdirectory could actually be undefined that would be a weird message!)

Yeah, it could for sure. The error handling here is pretty bare-bones. If you think it's worth refactoring here, let me know.

petebacondarwin

I note that you are skipping the two unit tests. Do they not work?

I know you said that it was hard to create a real world test for this.
I wonder if we could create a network mapped drive that points to a local folder?
https://support.microsoft.com/en-us/windows/map-a-network-drive-in-windows-29ce55d1-34e3-a7e2-4801-131475f9557d#ID0EBD=Windows_11

Cherry · 2023-06-14T19:55:25Z

I note that you are skipping the two unit tests. Do they not work?

Yeah I attempted a very dumb test by temporarily mocking renameSync to always throw the EXDEV error so the fs.cp codepath would be hit, but it didn't seem to stick. I'm not sure if runWrangler in tests is spawning a brand new process and therefore doesn't have the mocks? I'm really not sure, so left it skipped. Most tests in that file are skipped right now.

I wonder if we could create a network mapped drive that points to a local folder?
support.microsoft.com/en-us/windows/map-a-network-drive-in-windows-29ce55d1-34e3-a7e2-4801-131475f9557d#ID0EBD=Windows_11

This might work 🤔 I've never had to test anything like this in CI before. Prior libraries that have solved this seem to just handle it by naively mocking: https://github.com/andrewrk/node-mv/blob/645d8f4c783abf84204be704098fdd41f36ab195/test/test.js#L14

This is in use throughout other parts of the codebase, so it should be safe

petebacondarwin · 2023-06-27T13:46:51Z

packages/wrangler/src/git-client.ts

+		// likely on a different filesystem, so we need to copy instead of rename
+		// and then remove the original directory
+		try {
+			fs.cpSync(templatePath, targetDirectory);


We actually need a recursive option on this command too, which is why the tests were failing.
I have fixed this up and rebased.

petebacondarwin

Assuming CI is happy, I am happy.

Cherry · 2023-06-27T13:58:06Z

Thanks Pete! 🎉

Cherry mentioned this pull request Jun 10, 2023

🐛 BUG: wrangler generate sometimes fails due to cross-device renaming #2850

Closed

Cherry force-pushed the fix/cross-device-generate branch from f06ebbc to f406cfa Compare June 11, 2023 20:27

petebacondarwin reviewed Jun 14, 2023

View reviewed changes

Cherry mentioned this pull request Jun 15, 2023

chore: update root /templates to use wrangler generate instead of create-cloudflare@v1 #3452

Merged

2 tasks

petebacondarwin self-requested a review June 27, 2023 12:03

Cherry added 5 commits June 27, 2023 13:12

fix: generate command with running cross-device

a125c94

chore: add changeset

c203d94

chore: use fs.csp instead of copySync from fs-extra

a42628b

This is in use throughout other parts of the codebase, so it should be safe

chore: add basic mocked test

6d101f4

chore: add debug logs

97ad9fa

petebacondarwin reviewed Jun 27, 2023

View reviewed changes

petebacondarwin force-pushed the fix/cross-device-generate branch from 7dc55c7 to 97ad9fa Compare June 27, 2023 13:47

petebacondarwin requested review from a team as code owners June 27, 2023 13:47

petebacondarwin approved these changes Jun 27, 2023

View reviewed changes

petebacondarwin merged commit 8d1521e into cloudflare:main Jun 27, 2023

github-actions bot mentioned this pull request Jun 26, 2023

Version Packages #3503

Merged

Conversation

Cherry commented Jun 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

changeset-bot bot commented Jun 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🦋 Changeset detected

Uh oh!

github-actions bot commented Jun 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Jun 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Cherry commented Jun 9, 2023

Uh oh!

petebacondarwin Jun 14, 2023

Choose a reason for hiding this comment

Uh oh!

Cherry Jun 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

petebacondarwin Jun 14, 2023

Choose a reason for hiding this comment

Uh oh!

Cherry Jun 14, 2023

Choose a reason for hiding this comment

Uh oh!

petebacondarwin left a comment

Choose a reason for hiding this comment

Uh oh!

Cherry commented Jun 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

petebacondarwin Jun 27, 2023

Choose a reason for hiding this comment

Uh oh!

petebacondarwin left a comment

Choose a reason for hiding this comment

Uh oh!

Cherry commented Jun 27, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Cherry commented Jun 4, 2023 •

edited

Loading

changeset-bot bot commented Jun 4, 2023 •

edited

Loading

github-actions bot commented Jun 4, 2023 •

edited

Loading

codecov bot commented Jun 4, 2023 •

edited

Loading

Cherry Jun 14, 2023 •

edited

Loading

Cherry commented Jun 14, 2023 •

edited

Loading