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

feat(toolkit): ensure consistent zip files #2931

Merged
merged 7 commits into from
Jun 21, 2019
Merged

feat(toolkit): ensure consistent zip files #2931

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
5056d4c
feat(toolkit): ensure consistent zip files
jogold Jun 19, 2019
cfaa40d
simplify
jogold Jun 19, 2019
8e6ce58
test that dates are reset
jogold Jun 20, 2019
379d246
equal
jogold Jun 20, 2019
4df0e13
Merge branch 'master' into consistent-zip-assets
jogold Jun 20, 2019
a8c12f6
better with streams?
jogold Jun 20, 2019
491b4c8
add comment
jogold Jun 21, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 20 additions & 7 deletions packages/aws-cdk/lib/archive.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,26 +1,39 @@
import archiver = require('archiver');
import crypto = require('crypto');
import fs = require('fs-extra');
import glob = require('glob');
import path = require('path');

export function zipDirectory(directory: string, outputFile: string): Promise<void> {
return new Promise((ok, fail) => {
const output = fs.createWriteStream(outputFile);
const archive = archiver('zip');
// The below options are needed to support following symlinks when building zip files:
// - nodir: This will prevent symlinks themselves from being copied into the zip.
// - nodir: This will prevent symlinks themselves from being copied into the zip.
// - follow: This will follow symlinks and copy the files within.
const globOptions = {
dot: true,
nodir: true,
follow: true,
cwd: directory
cwd: directory,
};
archive.glob('**', globOptions);
archive.pipe(output);
archive.finalize();
const files = glob.sync('**', globOptions); // The output here is already sorted

const output = fs.createWriteStream(outputFile);

const archive = archiver('zip');
archive.on('warning', fail);
archive.on('error', fail);
archive.pipe(output);

files.forEach(file => { // Append files serially to ensure file order
const stream = fs.createReadStream(path.join(directory, file));
archive.append(stream, {
Copy link
Contributor

Choose a reason for hiding this comment

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

Aren't NodeJS streams async? Are we 100% positive that after this statement, the stream has been fully read and added? Or are we sure that .finalize() will synchronously read and flush all streams? Do we need to await finalize()?

It concerns me that I don't understand the API fully, and the documentation doesn't seem to be very explicit about it.

Copy link
Contributor Author

@jogold jogold Jun 20, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

From the example in the doc: finalize the archive (ie we are done appending files but streams have to finish yet)

From the doc: Finalizes the instance and prevents further appending to the archive structure (queue will continue til drained).

I've successfully created zips containing multiple files of more than 50MB with this code (resulting in zip files of several hundreds of MB).

Looks like finalize() is a blocking statement.

zipDirectory returns a promise that is resolved only once we get the close event on output, this is fired only when archiver has been finalized and the output file descriptor has closed (output.once('close', () => ok());)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's also possible to call archive.file() and let the library handle the read stream creation (need to check if the sort order is preserved across zips in this case).

Copy link
Contributor Author

@jogold jogold Jun 21, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Tested archive.file() and file order is not preserved across zips. If think that the only way to go here is to use archive.append() with fs.createReadStream().

Copy link
Contributor

Choose a reason for hiding this comment

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

Cool, yes you are right, I had missed this:

     output.once('close', () => ok());

I'm just very scared of accidental asyncness where you're not expecting it :(

Thanks, will merge when the validation build finishes.

name: file,
date: new Date('1980-01-01T00:00:00.000Z'), // reset dates to get the same hash for the same content
Copy link
Contributor Author

@jogold jogold Jun 20, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

on my system specifying new Date(0) here seems to be translated to 1980-01-01T00:00:00.000Z for the files in the zip... this means that I cannot write a unit test for the date reset => setting to 1980-01-01T00:00:00.000Z

});
});

archive.finalize();

output.once('close', () => ok());
});
}
Expand Down
50 changes: 49 additions & 1 deletion packages/aws-cdk/package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 3 additions & 0 deletions packages/aws-cdk/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@
"devDependencies": {
"@types/archiver": "^3.0.0",
"@types/fs-extra": "^7.0.0",
"@types/jszip": "^3.1.6",
"@types/minimatch": "^3.0.3",
"@types/mockery": "^1.4.29",
"@types/request": "^2.48.1",
Expand All @@ -46,6 +47,7 @@
"@types/yaml": "^1.0.2",
"@types/yargs": "^13.0.0",
"cdk-build-tools": "^0.35.0",
"jszip": "^3.2.1",
"mockery": "^2.1.0",
"pkglint": "^0.35.0",
"sinon": "^7.3.2"
Expand All @@ -60,6 +62,7 @@
"colors": "^1.3.3",
"decamelize": "^3.2.0",
"fs-extra": "^8.0.1",
"glob": "^7.1.4",
"json-diff": "^0.5.4",
"minimatch": ">=3.0",
"promptly": "^3.0.3",
Expand Down
8 changes: 8 additions & 0 deletions packages/aws-cdk/test/test.archive.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { exec as _exec } from 'child_process';
import fs = require('fs-extra');
import jszip = require('jszip');
import { Test } from 'nodeunit';
import os = require('os');
import path = require('path');
Expand All @@ -24,6 +25,13 @@ export = {
test.ok(false, `extracted directory ${extractDir} differs from original ${originalDir}`);
}

// inspect the zile file to check that dates are reset
const zip = await fs.readFile(zipFile);
const zipData = await jszip.loadAsync(zip);
const dates = Object.values(zipData.files).map(file => file.date.toISOString());
test.equal(dates[0], '1980-01-01T00:00:00.000Z', 'Dates are not reset');
test.equal(new Set(dates).size, 1, 'Dates are not equal');

await fs.remove(stagingDir);
await fs.remove(extractDir);
test.done();
Expand Down
2 changes: 1 addition & 1 deletion packages/aws-cdk/tsconfig.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
"compilerOptions": {
"target":"ES2018",
"module": "commonjs",
"lib": ["es2016", "es2017.object", "es2017.string"],
"lib": ["es2016", "es2017.object", "es2017.string", "dom"],
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is needed for @types/jszip

"declaration": true,
"strict": true,
"noImplicitAny": true,
Expand Down