Skip to content

Commit

Permalink
Revert "chore(core): allow the bundler to re-use pre-existing bundler…
Browse files Browse the repository at this point in the history
… output (#8916)" (#9037)

This logic to reuse bundler output is faulty. Once an asset is bundled, the bundle directory is moved into the cloud assembly (cdk.out) and therefore the logic that attempts to reuse the bundle directory only materializes in the case where moveSync decides to copy the directory and not just move it.

This reverts commit 31d6e65.
  • Loading branch information
Elad Ben-Israel authored Jul 13, 2020
1 parent 74f4b3d commit 2cc46d5
Show file tree
Hide file tree
Showing 5 changed files with 37 additions and 251 deletions.
1 change: 0 additions & 1 deletion packages/@aws-cdk/aws-lambda/test/integ.bundling.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
/// !cdk-integ pragma:ignore-assets
import * as path from 'path';
import { App, CfnOutput, Construct, Stack, StackProps } from '@aws-cdk/core';
import * as lambda from '../lib';
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
/// !cdk-integ pragma:ignore-assets
import * as path from 'path';
import * as iam from '@aws-cdk/aws-iam';
import { App, BundlingDockerImage, Construct, Stack, StackProps } from '@aws-cdk/core';
Expand Down
102 changes: 22 additions & 80 deletions packages/@aws-cdk/core/lib/asset-staging.ts
Original file line number Diff line number Diff line change
Expand Up @@ -88,23 +88,12 @@ export class AssetStaging extends Construct {
this.sourcePath = props.sourcePath;
this.fingerprintOptions = props;

// Determine the hash type based on the props as props.assetHashType is
// optional from a caller perspective.
const hashType = this.determineHashType(props.assetHashType, props.assetHash);

if (props.bundling) {
// Determine the source hash in advance of bundling if the asset hash type
// is source so that the bundler can opt to re-use its bundle dir.
const sourceHash = hashType === AssetHashType.SOURCE
? this.calculateHash(hashType, props.assetHash, props.bundling)
: undefined;

this.bundleDir = this.bundle(props.bundling, sourceHash);
this.assetHash = sourceHash ?? this.calculateHash(hashType, props.assetHash, props.bundling);
} else {
this.assetHash = this.calculateHash(hashType, props.assetHash);
this.bundleDir = this.bundle(props.bundling);
}

this.assetHash = this.calculateHash(props);

const stagingDisabled = this.node.tryGetContext(cxapi.DISABLE_ASSET_STAGING_CONTEXT);
if (stagingDisabled) {
this.stagedPath = this.bundleDir ?? this.sourcePath;
Expand Down Expand Up @@ -148,37 +137,15 @@ export class AssetStaging extends Construct {
}
}

/**
* @Property sourceHash The source hash of the asset. If specified, the bundler
* will attempt to re-use the bundling directory, which is based on a hash of
* both the sourceHash and options. If bundling finds a pre-existing directory,
* the bundler will return it as-is and won't regenerate the bundle.
*/
private bundle(options: BundlingOptions, sourceHash?: string): string {
private bundle(options: BundlingOptions): string {
// Temp staging directory in the working directory
const stagingTmp = path.join('.', STAGING_TMP);
fs.ensureDirSync(stagingTmp);

let bundleDir: string;
if (sourceHash) {
// When an asset hash is known in advance of bundling, bundling is done into a dedicated staging directory.
bundleDir = path.resolve(path.join(stagingTmp, 'asset-bundle-hash-' + sourceHash));

if (fs.existsSync(bundleDir)) {
// Pre-existing bundle directory. The bundle has already been generated once before, so lets provide it
// as-is to the caller.
return bundleDir;
}

fs.ensureDirSync(bundleDir);
} else {
// When the asset hash isn't known in advance, bundling is done into a temporary staging directory.

// Create temp directory for bundling inside the temp staging directory
bundleDir = path.resolve(fs.mkdtempSync(path.join(stagingTmp, 'asset-bundle-temp-')));
// Chmod the bundleDir to full access.
fs.chmodSync(bundleDir, 0o777);
}
// Create temp directory for bundling inside the temp staging directory
const bundleDir = path.resolve(fs.mkdtempSync(path.join(stagingTmp, 'asset-bundle-')));
// Chmod the bundleDir to full access.
fs.chmodSync(bundleDir, 0o777);

let user: string;
if (options.user) {
Expand Down Expand Up @@ -214,18 +181,7 @@ export class AssetStaging extends Construct {
workingDirectory: options.workingDirectory ?? AssetStaging.BUNDLING_INPUT_DIR,
});
} catch (err) {
// When bundling fails, keep the bundle output for diagnosability, but
// rename it out of the way so that the next run doesn't assume it has a
// valid bundleDir.

const bundleErrorDir = bundleDir + '-error';
if (fs.existsSync(bundleErrorDir)) {
// Remove the last bundleErrorDir.
fs.removeSync(bundleErrorDir);
}

fs.renameSync(bundleDir, bundleErrorDir);
throw new Error(`Failed to run bundling Docker image for asset ${this.node.path}, bundle output is located at ${bundleErrorDir}: ${err}`);
throw new Error(`Failed to run bundling Docker image for asset ${this.node.path}: ${err}`);
}

if (FileSystem.isEmpty(bundleDir)) {
Expand All @@ -235,49 +191,35 @@ export class AssetStaging extends Construct {
return bundleDir;
}

/**
* Determines the hash type from user-given prop values.
*
* @param assetHashType Asset hash type construct prop
* @param assetHash Asset hash given in the construct props
*/
private determineHashType(assetHashType?: AssetHashType, assetHash?: string) {
if (assetHash) {
if (assetHashType && assetHashType !== AssetHashType.CUSTOM) {
throw new Error(`Cannot specify \`${assetHashType}\` for \`assetHashType\` when \`assetHash\` is specified. Use \`CUSTOM\` or leave \`undefined\`.`);
private calculateHash(props: AssetStagingProps): string {
let hashType: AssetHashType;

if (props.assetHash) {
if (props.assetHashType && props.assetHashType !== AssetHashType.CUSTOM) {
throw new Error(`Cannot specify \`${props.assetHashType}\` for \`assetHashType\` when \`assetHash\` is specified. Use \`CUSTOM\` or leave \`undefined\`.`);
}
return AssetHashType.CUSTOM;
} else if (assetHashType) {
return assetHashType;
hashType = AssetHashType.CUSTOM;
} else if (props.assetHashType) {
hashType = props.assetHashType;
} else {
return AssetHashType.SOURCE;
hashType = AssetHashType.SOURCE;
}
}

private calculateHash(hashType: AssetHashType, assetHash?: string, bundling?: BundlingOptions): string {
switch (hashType) {
case AssetHashType.SOURCE:
const sourceHash = FileSystem.fingerprint(this.sourcePath, this.fingerprintOptions);
if (bundling) {
return crypto.createHash('sha256')
.update(JSON.stringify(bundling))
.update(sourceHash)
.digest('hex');
} else {
return sourceHash;
}
return FileSystem.fingerprint(this.sourcePath, this.fingerprintOptions);
case AssetHashType.BUNDLE:
if (!this.bundleDir) {
throw new Error('Cannot use `AssetHashType.BUNDLE` when `bundling` is not specified.');
}
return FileSystem.fingerprint(this.bundleDir, this.fingerprintOptions);
case AssetHashType.CUSTOM:
if (!assetHash) {
if (!props.assetHash) {
throw new Error('`assetHash` must be specified when `assetHashType` is set to `AssetHashType.CUSTOM`.');
}
// Hash the hash to make sure we can use it in a file/directory name.
// The resulting hash will also have the same length as for the other hash types.
return crypto.createHash('sha256').update(assetHash).digest('hex');
return crypto.createHash('sha256').update(props.assetHash).digest('hex');
default:
throw new Error('Unknown asset hash type.');
}
Expand Down
1 change: 0 additions & 1 deletion packages/@aws-cdk/core/test/docker-stub.sh
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ set -euo pipefail
# `/tmp/docker-stub.input` and accepts one of 3 commands that impact it's
# behavior.

echo "$@" >> /tmp/docker-stub.input.concat
echo "$@" > /tmp/docker-stub.input

if echo "$@" | grep "DOCKER_STUB_SUCCESS_NO_OUTPUT"; then
Expand Down
Loading

0 comments on commit 2cc46d5

Please sign in to comment.