Skip to content

Conversation

@petebacondarwin
Copy link
Contributor

@petebacondarwin petebacondarwin commented Nov 15, 2023

Our use of shell-quote was causing problems on Windows where it was escaping character (such as @) by placing a backslash in front. This made Windows think that such path arguments, were at the root.

For example, npm install -D @cloudflare/workers-types was being converted to npm install -D \@cloudflare/workers-types, which resulted in errors like:

npm ERR! enoent ENOENT: no such file or directory, open 'D:\@cloudflare\workers-types\package.json'

Now we just rely directly on the Node.js spawn API to avoid any shell quoting concerns. This has resulted in a slightly less streamlined experience for people writing C3 plugins, but has the benefit that the developer doesn't have to worry about quoting spawn arguments.

Fixes #4282
Fixes #4425
Fixes #4342

Author has addressed the following:

  • Tests
    • Included
    • Not necessary because:
  • Changeset (Changeset guidelines)
    • Included
    • Not necessary because:
  • Associated docs
    • Issue(s)/PR(s):
    • Not necessary because: it is a bug fix with no changes to usage.

@petebacondarwin petebacondarwin requested a review from a team November 15, 2023 10:47
@petebacondarwin petebacondarwin requested a review from a team as a code owner November 15, 2023 10:47
@changeset-bot
Copy link

changeset-bot bot commented Nov 15, 2023

🦋 Changeset detected

Latest commit: c06c331

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

This PR includes changesets to release 1 package
Name Type
create-cloudflare 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

@petebacondarwin petebacondarwin added c3 Relating to C3 (create-cloudflare) package bug Something that isn't working labels Nov 15, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Nov 15, 2023

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/6982999562/npm-package-wrangler-4445

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

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

Or you can use npx with this latest build directly:

npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/6982999562/npm-package-wrangler-4445 dev path/to/script.js
Additional artifacts:
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/6982999562/npm-package-miniflare-4445
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/6982999562/npm-package-cloudflare-pages-shared-4445

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


wrangler@3.17.1 includes the following runtime dependencies:

Package Constraint Resolved
miniflare workspace:* 3.20231030.1
workerd 1.20231030.0 1.20231030.0
workerd --version 1.20231030.0 2023-10-30

|

Please ensure constraints are pinned, and miniflare/workerd minor versions match.

@codecov
Copy link

codecov bot commented Nov 15, 2023

Codecov Report

Merging #4445 (c06c331) into main (61ec986) will increase coverage by 0.01%.
The diff coverage is n/a.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #4445      +/-   ##
==========================================
+ Coverage   75.44%   75.45%   +0.01%     
==========================================
  Files         240      240              
  Lines       12851    12851              
  Branches     3309     3309              
==========================================
+ Hits         9695     9697       +2     
+ Misses       3156     3154       -2     

see 5 files with indirect coverage changes

@petebacondarwin petebacondarwin force-pushed the fix-c3-shell-scripts-on-windows branch from cb792c0 to c6a9957 Compare November 15, 2023 10:58
@dario-piotrowicz
Copy link
Member

@RamIdeas pinging you since you've introduces the use of shell-quote generally I think 🙂

@petebacondarwin petebacondarwin force-pushed the fix-c3-shell-scripts-on-windows branch 2 times, most recently from 4381160 to 8dbce2d Compare November 15, 2023 13:15
@penalosa penalosa linked an issue Nov 15, 2023 that may be closed by this pull request
@petebacondarwin petebacondarwin force-pushed the fix-c3-shell-scripts-on-windows branch 10 times, most recently from 301c5a4 to 36b52d5 Compare November 16, 2023 16:57
@RamIdeas
Copy link
Contributor

I don't think this is the best solution for the issue.

Enforcing the use of argv-style arrays across the codebase is a DX decision and subjective, so I'm not commenting on this specifically. Notably this only considers commands/args authored within the codebase.

So even if we enforce argv-style arrays, there are still encoding issues when accepting command strings as env vars (still end up ssplitting them by space) or stringifying the arrays for printing the commands to the terminal (still end up joining them by space)

We could (a) still use shell-quote in those few cases (i.e. much less use of the lib) or (b) we could account for the @ in shell-quote and workaround the bug (and any other bugs that arise from use of the lib) and use the lib everywhere as it currently is while keeping our DX around command strings – I would favour (b)

@petebacondarwin petebacondarwin force-pushed the fix-c3-shell-scripts-on-windows branch from 36b52d5 to e894e77 Compare November 16, 2023 17:17
Previously we only deleted Pages projects - in fact we were trying to delete
the Hono (non-Pages) Worker as a Pages project.

Now we delete the project based on its type.
- do not use bash-style environment variable setting in Docusaurus scripts
- do not run TTY interactive e2e tests on Windows
These e2e test workers and projects should be cleaned up as part of the normal test completion.
But if the test crashes they may be left orphaned.
This change ensures that we do not clean up projects to early while they are still being used.
Previously we were reusing folders after clearing them when retrying a
failed test. But this could lead to problems, especially on Windows where
clearing out the folder did not always work.
This commit uses environment variables to tell package managers to put
cached files in a local directory rather than a global shared one, which
can cause problems with race conditions when running multiple installs
at the same time.
It is a false optimization to cancel jobs that are likely to pass when
another job flakes out.
These jobs tend to flake out and are not providing much of a useful signal.
Reenable when #4241 lands and improves reliability of this test.
C3 often outputs log messages to the user of commands that are being
executed. Users tend to cut and paste these into their terminal to run
themselves. This makes sure that these are likely to just work went
pasted into their shell.
@petebacondarwin petebacondarwin force-pushed the fix-c3-shell-scripts-on-windows branch from 3ee51d5 to aed7c14 Compare November 24, 2023 16:23
@petebacondarwin petebacondarwin merged commit 652cc42 into main Nov 24, 2023
@petebacondarwin petebacondarwin deleted the fix-c3-shell-scripts-on-windows branch November 24, 2023 17:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something that isn't working c3 Relating to C3 (create-cloudflare) package

Projects

None yet

4 participants