Skip to content

Remove fast-crc32c dependency #3249

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

Merged
merged 2 commits into from
Mar 31, 2021
Merged

Remove fast-crc32c dependency #3249

merged 2 commits into from
Mar 31, 2021

Conversation

samtstern
Copy link
Contributor

@samtstern samtstern commented Mar 31, 2021

Description

Fixes #3247
Fixes #3239

The fast-crc32c native module required by the storage emulator has been causing problems with our Firepit builds as well as running the NPM module on ARM systems.

I searched for a suitable all-JS replacement but was not able to find one which seemed to be high quality. So instead I used Wikipedia and Stackoverflow to figure out how to implement the algorithm ourselves and remove the dependency entirely.

Scenarios Tested

I lifted 100% of the test cases from the fast-crc32c library we previously depended on and ran them against this new implementation, they all pass.

I also did an e2e test with this simple Cloud function:

const functions = require("firebase-functions");
const admin = require("firebase-admin");
admin.initializeApp();

process.env.STORAGE_EMULATOR_HOST = "http://localhost:9199";

exports.uploadFile = functions.https.onRequest(async (request, response) => {
  const res = await admin.storage().bucket().upload('content.txt', {
    destination: `${new Date().getTime()}.txt`
  });

  response.json({
    status: 'ok'
  });
});

It works fine, but if I intentionally botch the crc implementation then we get this error:

Error: The uploaded data did not match the data from the server. As a precaution, we attempted to delete the file, but it was not successful. To be sure the content is the same, you should try removing the file manually, then uploading the file again. 

Sample Commands

N/A

@samtstern samtstern requested review from abeisgoat and bkendall March 31, 2021 10:50
@google-cla google-cla bot added the cla: yes Manual indication that this has passed CLA. label Mar 31, 2021
Copy link
Contributor

@bkendall bkendall left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for taking this on.

@Brooooooklyn
Copy link

Would you like to try https://github.com/napi-rs/node-rs/tree/main/packages/crc32 , which is faster and support more platform:

node12 node14 node16
Windows x64
Windows x32
Windows arm64
macOS x64
macOS arm64
Linux x64 gnu
Linux x64 musl
Linux arm gnu
Linux arm64 gnu
Linux arm64 musl
Android arm64
FreeBSD x64

devpeerapong pushed a commit to devpeerapong/firebase-tools that referenced this pull request Dec 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Manual indication that this has passed CLA.
Projects
None yet
4 participants