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.

Sign up for GitHub

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

Jump to bottom

fix: align publishing sites asset keys with Wrangler 1 #277

Merged
merged 1 commit into from
Jan 24, 2022
Merged

fix: align publishing sites asset keys with Wrangler 1 #277

merged 1 commit into from
Jan 24, 2022

Conversation

petebacondarwin
Copy link
Contributor

Builds on top of #270.

  • Use the same hashing strategy for asset keys (xxhash64)
  • Include the full path (from cwd) in the asset key
  • Match include and exclude patterns against full path (from cwd)
  • Validate that the asset key is not over 512 bytes long

@petebacondarwin petebacondarwin added the blocked Blocked on other work label Jan 21, 2022
@changeset-bot
Copy link

changeset-bot bot commented Jan 21, 2022 •
edited

🦋 Changeset detected

Latest commit: 40969cb

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

@petebacondarwin petebacondarwin force-pushed the sites-hash-key branch 2 times, most recently from 8c1c3fd to a3101a1 Compare January 21, 2022 15:13
This was referenced Jan 21, 2022
Closed
Merged
threepointone
threepointone approved these changes Jan 24, 2022
View reviewed changes
Copy link
Contributor

@threepointone threepointone left a comment

Choose a reason for hiding this comment

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

Just a couple of notes, but this looks good to me. Thank you for fleshing this out! Important for people moving projects over, I appreciate that you did the command line args too.

packages/wrangler/package.json Outdated Show resolved Hide resolved
packages/wrangler/src/config.ts Outdated Show resolved Hide resolved
packages/wrangler/src/__tests__/kv.test.ts Outdated Show resolved Hide resolved
@petebacondarwin
fix: align publishing sites asset keys with Wrangler 1
40969cb 
- Use the same hashing strategy for asset keys (xxhash64)
- Include the full path (from cwd) in the asset key
- Match include and exclude patterns against full path (from cwd)
- Validate that the asset key is not over 512 bytes long
@petebacondarwin petebacondarwin merged commit 6cc9dde into cloudflare:main Jan 24, 2022
@petebacondarwin petebacondarwin deleted the sites-hash-key branch January 24, 2022 15:11
@github-actions github-actions bot mentioned this pull request Jan 24, 2022
Merged
@chris-schra
Copy link

I'm not sure if this has to do with this merge, but I just updated 0.0.6 to 0.0.15 and npm is unable to build xxhash-addon (and that package looks a bit unmaintained, tbh):

# This file contains the result of Yarn building a package (xxhash-addon@npm:1.4.0)
# Script name: install

gyp info it worked if it ends with ok
gyp info using node-gyp@8.4.1
gyp info using node@16.9.1 | win32 | x64
gyp info find Python using Python version 3.10.1 found at "<REDACTED>>\AppData\Local\Programs\Python\Python310\python.exe"
gyp ERR! find VS 
gyp ERR! find VS msvs_version not set from command line or npm config
gyp ERR! find VS VCINSTALLDIR not set, not running in VS Command Prompt
gyp ERR! find VS could not use PowerShell to find Visual Studio 2017 or newer, try re-running with '--loglevel silly' for more details
gyp ERR! find VS looking for Visual Studio 2015
gyp ERR! find VS - not found
gyp ERR! find VS not looking for VS2013 as it is only supported up to Node.js 8
gyp ERR! find VS 
gyp ERR! find VS **************************************************************
gyp ERR! find VS You need to install the latest version of Visual Studio
gyp ERR! find VS including the "Desktop development with C++" workload.
gyp ERR! find VS For more information consult the documentation at:
gyp ERR! find VS https://github.com/nodejs/node-gyp#on-windows
gyp ERR! find VS **************************************************************
gyp ERR! find VS 
gyp ERR! configure error 
gyp ERR! stack Error: Could not find any Visual Studio installation to use
gyp ERR! stack     at VisualStudioFinder.fail (C:\WorkBench\frontend\node_modules\node-gyp\lib\find-visualstudio.js:122:47)
gyp ERR! stack     at C:\WorkBench\frontend\node_modules\node-gyp\lib\find-visualstudio.js:75:16
gyp ERR! stack     at VisualStudioFinder.findVisualStudio2013 (C:\WorkBench\frontend\node_modules\node-gyp\lib\find-visualstudio.js:363:14)
gyp ERR! stack     at C:\WorkBench\frontend\node_modules\node-gyp\lib\find-visualstudio.js:71:14
gyp ERR! stack     at C:\WorkBench\frontend\node_modules\node-gyp\lib\find-visualstudio.js:384:16
gyp ERR! stack     at C:\WorkBench\frontend\node_modules\node-gyp\lib\util.js:54:7
gyp ERR! stack     at C:\WorkBench\frontend\node_modules\node-gyp\lib\util.js:33:16
gyp ERR! stack     at ChildProcess.exithandler (node:child_process:404:5)
gyp ERR! stack     at ChildProcess.emit (node:events:394:28)
gyp ERR! stack     at maybeClose (node:internal/child_process:1064:16)
gyp ERR! System Windows_NT 10.0.19043
gyp ERR! command "C:\\Program Files\\nodejs\\node.exe" "C:\\WorkBench\\frontend\\node_modules\\node-gyp\\bin\\node-gyp.js" "rebuild" "--ensure"
gyp ERR! cwd C:\WorkBench\frontend\node_modules\xxhash-addon
gyp ERR! node -v v16.9.1
gyp ERR! node-gyp -v v8.4.1
gyp ERR! not ok

@petebacondarwin
Copy link
Contributor Author

I got the same error when trying to use the Windows 2022 Server runner in our Github Actions.
The cause is that the xxhash-addon package needs to run a postinstall node-gyp script to generate the native code that implements the hashing algorithm.

Your machine needs to have the necessary compilers installed on your machine (Unix based machines tend to have these already). For Windows you can follow the instructions here: https://github.com/nodejs/node-gyp#on-windows.

I'll have a look around and see if there is an alternative xxhash implementation we can use that doesn't require node-gyp. But when I implemented this last month, I did not find a suitable one.

In the long run, we should probably move away from xxhash and use a hashing algorithm that is supported by node.js out of the box. The downside of doing this right now is that any assets that were previously uploaded to Sites would get new hashes and so would need to be uploaded and stored again.

@petebacondarwin
Copy link
Contributor Author

Perhaps we can try https://www.npmjs.com/package/xxhash-wasm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@threepointone threepointone threepointone approved these changes

Assignees
No one assigned
Labels
blocked Blocked on other work
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@petebacondarwin @chris-schra @threepointone