-
Notifications
You must be signed in to change notification settings - Fork 960
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
Replace dependency on broken node-unzipper with native zlib #5714
Conversation
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
dbe4998
to
b1fd507
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Curious: What is this broken.zip file for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry, it was copied over from the fixtures in unzipper
and forgot to delete it since it's not used in the tests. Going to remove it.
I've also included LICENSE file from node-unzipper package since I added some of the fixtures from there - https://github.com/ZJONSSON/node-unzipper/blob/master/LICENSE . I am not an expert on license policies - is it okay to keep it in there or should I remove it?
Thanks for contributing to the firebase-tools repo !
We can force a re-download of the corrupted emulators in the cache by bumping their versions. |
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #5714 +/- ##
==========================================
+ Coverage 55.20% 55.22% +0.01%
==========================================
Files 328 329 +1
Lines 22417 22500 +83
Branches 4576 4586 +10
==========================================
+ Hits 12375 12425 +50
- Misses 8941 8971 +30
- Partials 1101 1104 +3
☔ View full report in Codecov by Sentry. |
awesome, @christhompsongoogle , in case this gets released can you keep track of bumping the version of ui emulator and pubsub emulator? |
Sure, I'd be happy to bump the versions once this is in |
cd0b797
to
8a159e2
Compare
After doing some testing on Windows it seems that there's an issue with the mkdir (see below for details). If I were to guess I think this has to do with the fact that windows uses backslash instead of forward slashes for their directory names, so
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really appreciate the contribution! Some changes to make, but this is a great start.
I also pulled this locally and did some testing on various versions of Node on a Mac w/ Intel chips, seems to work correctly AFAICT.
CHANGELOG.md
Outdated
@@ -5,3 +5,5 @@ | |||
- Lift GCF 2nd gen naming restrictions (#5690) | |||
- Fixes a bug where `ext:install` and `ext:configure` would error on extensions with no params. | |||
- Fixed an issue with Vite and Angular integrations using a obsolete NPM command (#5710) | |||
- Fix bugs with UI emulator and PubSub emulator not starting correctly (#5714) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Fix bugs with UI emulator and PubSub emulator not starting correctly (#5714) | |
- Fixes bug were emulators would not starting correctly due to corrupted ZIP files. (#5614, #5677) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rewording this a lil bit.
CHANGELOG.md
Outdated
@@ -5,3 +5,5 @@ | |||
- Lift GCF 2nd gen naming restrictions (#5690) | |||
- Fixes a bug where `ext:install` and `ext:configure` would error on extensions with no params. | |||
- Fixed an issue with Vite and Angular integrations using a obsolete NPM command (#5710) | |||
- Fix bugs with UI emulator and PubSub emulator not starting correctly (#5714) | |||
- Should resolve #5614 #5677 firebase/firebase-tools-ui#939 firebase/firebase-tools-ui#940 firebase/firebase-tools-ui#942 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Should resolve #5614 #5677 firebase/firebase-tools-ui#939 firebase/firebase-tools-ui#940 firebase/firebase-tools-ui#942 |
@@ -0,0 +1,25 @@ | |||
Copyright (c) 2012 - 2013 Near Infinity Corporation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove this file - we aren't able to accept PRs that add new licenses to this repo.
@@ -0,0 +1,165 @@ | |||
import { expect } from "chai"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This thorough testing is great! However, these seem like integration tests to me. They are longer running than is ideal for our unit tests, and they also occasionally flake due to timeouts when run locally.
I think these tests would be appropriate as integration tests. Could you separate them into a different npm command test:unzip
, and ensure that they are not run as part of npm run test
?
src/test/unzip.spec.ts
Outdated
}); | ||
|
||
after(async () => { | ||
// await fs.promises.rmdir(ZIP_TEMPORARY_PATH, { recursive: true }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like it should not be commented out
src/test/unzip.spec.ts
Outdated
before(function (done) { | ||
// eslint-disable-next-line @typescript-eslint/no-invalid-this | ||
this.timeout(5000); | ||
(async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unclear to me why we need this needs to be set up as a self executing function. Could this just be:
before(async () => {
...
}
src/test/unzip.spec.ts
Outdated
}); | ||
|
||
it("should unzip a ui emulator zip file", async function () { | ||
// eslint-disable-next-line @typescript-eslint/no-invalid-this |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can remove this linter violation by refactoring to:
it("should unzip...", async () => {}).timeout(2000);
src/test/unzip.spec.ts
Outdated
}); | ||
|
||
it("should unzip a pubsub emulator zip file", async function () { | ||
// eslint-disable-next-line @typescript-eslint/no-invalid-this |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can remove this linter violation by refactoring to:
it("should unzip...", async () => {}).timeout(2000);
Hi, unfortunately I have no Windows device, I've pushed a fix replacing |
Hi @joehan, thank you for the great points. I've addressed them. Regarding moving the integration tests for unzip into a separate test - is it okay that I've moved them under |
Here's the latest issue I encountered
The directory has a client directory but it's empty - and no server directory exists there |
Hey @christhompsongoogle, sorry for the trouble. I've pushed a new version with potential fix and additional console logs in case the fix doesn't work. Once I've manage to fix that I'll go ahead and remove those console logs. In case you have time, can you test it? I'll have my brother over at my place on Saturday so I could borrow his Windows laptop to debug this but currently I don't have access to any windows machine. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few more minor tweaks, but this is very close to LGTM.
CHANGELOG.md
Outdated
@@ -5,5 +5,4 @@ | |||
- Lift GCF 2nd gen naming restrictions (#5690) | |||
- Fixes a bug where `ext:install` and `ext:configure` would error on extensions with no params. | |||
- Fixed an issue with Vite and Angular integrations using a obsolete NPM command (#5710) | |||
- Fix bugs with UI emulator and PubSub emulator not starting correctly (#5714) | |||
- Should resolve #5614 #5677 firebase/firebase-tools-ui#939 firebase/firebase-tools-ui#940 firebase/firebase-tools-ui#942 | |||
- Fixes bug were emulators would not starting correctly due to corrupted ZIP files. (#5614, #5677) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Fixes bug were emulators would not starting correctly due to corrupted ZIP files. (#5614, #5677) | |
- Fixes bug were emulators would not start correctly due to corrupted ZIP files. (#5614, #5677) |
@@ -0,0 +1,12 @@ | |||
module.exports = (() => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you run npm run format
? This file is failing whitespace linting
src/unzip.ts
Outdated
const compressionMethod = entryHeader.readUInt16LE(8); | ||
if (compressionMethod === 0) { | ||
// Store (no compression) | ||
console.log(`Writing file: \${outputFilePath}`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please switch this to logger.debug(). That way, the logs will only appear in --debug mode.
src/unzip.ts
Outdated
|
||
const outputFilePath = path.normalize(path.join(outputDir, entry.fileName)); | ||
|
||
console.log(`Processing entry: \${entry.fileName}`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please switch this to logger.debug(). That way, the logs will only appear in --debug mode.
TODO: remove console logs after @christhompsongoogle tries it out.
@joehan I've adjusted the code based on your comments. I've also rebased on the current master. @christhompsongoogle, once you'll be retesting this, can you include a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM once @christhompsongoogle confirms that it looks good on Windows
Just retested on Windows and everything looks great. Thank you for the windows fixes ! LGTM on this one |
Hey should I remove the unnecessary debug logs? in case someone wants to debug something so that they don't see these unnecessary logs. |
I don't mind leaving them in - they'll require some formatting to satisfy lint before we merge. |
I'm having a really tough time understanding the linting rules (there's plenty of warnings popping up). Can you advise what needs to be changed before merging? |
Hey @Durisvk, I did some more testing this weekend and realized that the extensions emulator isn't working under this PR. I'm going to try to debug/fix it when I have some time this week, but if you want to take a crack at it, the easiest way to exercise the path is the integration test: FBTOOLS_TARGET_PROJECT= npm run test:extensions-emulator |
So, it seems like this implementation doesn't handle zips that have the data descriptor bit set: https://en.wikipedia.org/wiki/ZIP_(file_format)#:~:text=the%20compressed%20data.-,Data%20descriptor,-%5Bedit%5D. Incidentally, Extensions zips all have this bit set. Looking into how to add support for this now |
src/unzip.ts
Outdated
await fs.promises.mkdir(parentDir, { recursive: true }); | ||
|
||
const compressionMethod = entryHeader.readUInt16LE(8); | ||
if (compressionMethod === 0) { | ||
if (entry.compressedSize === 0 || compressionMethod === 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't love that we're using the compressedSize to make decisions - though that might legitimately be part of the header. Do you have a link to the header breakdown (or the bitFlag === 8 details) we could leave as a comment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch - this actually does not need to be here, was leftover from a different approach i tried.
@Durisvk @christhompsongoogle @joehan do you have a timeline when this fix will be available in the official NPM? Thanks! |
…#5714) * Replace dependency on broken node-unzipper with native zlib Fixes: firebase#5614 firebase#5677 firebase/firebase-tools-ui#939 firebase/firebase-tools-ui#940 * remove unused broken.zip fixture * add changelog record * fix timing out test * fix use operating system file delimiter * addressing code review comments from @joehan * better support for Windows path separators TODO: remove console logs after @christhompsongoogle tries it out. * addressing code review comments from @joehan * Add support for zip files that use signed data descriptors instead of a full entry header * Formats, fixes uneeded async/await * Remove unnecessary handling for empty files * Remove console.log --------- Co-authored-by: joehan <joehanley@google.com>
This should be available now, with firebase-tools v12.0 (and possibly 11.30 before that) |
* Replace dependency on broken node-unzipper with native zlib Fixes: #5614 #5677 firebase/firebase-tools-ui#939 firebase/firebase-tools-ui#940 * remove unused broken.zip fixture * add changelog record * fix timing out test * fix use operating system file delimiter * addressing code review comments from @joehan * better support for Windows path separators TODO: remove console logs after @christhompsongoogle tries it out. * addressing code review comments from @joehan * Add support for zip files that use signed data descriptors instead of a full entry header * Formats, fixes uneeded async/await * Remove unnecessary handling for empty files * Remove console.log --------- Co-authored-by: joehan <joehanley@google.com>
Description
Fixes:
#5614
#5677
firebase/firebase-tools-ui#939
firebase/firebase-tools-ui#940
firebase/firebase-tools-ui#942
The node-unzipper package is generating incorrect files when unpacking the
ui
andpubsub
emulator archive files (ZJONSSON/node-unzipper#271). It generates a Syntax error and the emulator fails to execute. The issue happens on the latest NodeJS 18 version.Scenarios Tested
Automated: src/test/unzip.spec.ts
Manual:
Running the command before the fix
firebase emulators:start --only database,auth,ui --import ./firebase-emulator --export-on-exit
Output:
Then running
rm -rf ~/.cache/firebase/emulators
, linking my new version offirebase-tools
and rerunning the same command again:firebase emulators:start --only database,auth,ui --import ./firebase-emulator --export-on-exit
Output:
Sample Commands
N/A
IMPORTANT ❗
Is it possible to automatically execute
rm -rf ~/.cache/firebase
for users after updating the package? If not then the fix will just not work because the corrupted cached version will be preferred.One workaround I can think of is introducing a cache versioning as a text file in the
~/.cache/firebase
folder. Whenever that number doesn't match the sourcecode or the file doesn't exist the cache is forcefully regenerated and the cache version is adjusted to match the one in the sourcecode.