diff --git a/lib/src/asset/file_based.dart b/lib/src/asset/file_based.dart index de53df562..8ee8a0f2a 100644 --- a/lib/src/asset/file_based.dart +++ b/lib/src/asset/file_based.dart @@ -26,14 +26,11 @@ class FileBasedAssetReader implements AssetReader { @override Future hasInput(AssetId id) async { - _checkInput(id); return _fileFor(id, packageGraph).exists(); } @override Future readAsString(AssetId id, {Encoding encoding: UTF8}) async { - _checkInput(id); - var file = await _fileFor(id, packageGraph); if (!await file.exists()) { throw new AssetNotFoundException(id); @@ -41,14 +38,6 @@ class FileBasedAssetReader implements AssetReader { return file.readAsString(encoding: encoding); } - /// Checks that [id] is a valid input, and throws an [InvalidInputException] - /// if its not. - void _checkInput(AssetId id) { - if (id.package != packageGraph.root.name && !id.path.startsWith('lib/')) { - throw new InvalidInputException(id); - } - } - /// Searches for all [AssetId]s matching [inputSet]s. Stream listAssetIds(Iterable inputSets) async* { var seenAssets = new Set(); @@ -88,10 +77,6 @@ class FileBasedAssetWriter implements AssetWriter { @override Future writeAsString(Asset asset, {Encoding encoding: UTF8}) async { - if (asset.id.package != packageGraph.root.name) { - throw new InvalidOutputException(asset); - } - var file = _fileFor(asset.id, packageGraph); await file.create(recursive: true); await file.writeAsString(asset.stringContents, encoding: encoding); diff --git a/lib/src/builder/build_step_impl.dart b/lib/src/builder/build_step_impl.dart index be799b4d3..50ab31e1c 100644 --- a/lib/src/builder/build_step_impl.dart +++ b/lib/src/builder/build_step_impl.dart @@ -10,6 +10,7 @@ import 'package:logging/logging.dart'; import '../analyzer/resolver.dart'; import '../asset/asset.dart'; +import '../asset/exceptions.dart'; import '../asset/id.dart'; import '../asset/reader.dart'; import '../asset/writer.dart'; @@ -20,6 +21,7 @@ import 'exceptions.dart'; /// A single step in the build processes. This represents a single input and /// its expected and real outputs. It also handles tracking of dependencies. class BuildStepImpl implements BuildStep { + /// Single `_resolvers` instance for all [BuildStepImpl]s static code_transformers.Resolvers _resolvers = new code_transformers.Resolvers(code_transformers.dartSdkDirectory); @@ -36,6 +38,7 @@ class BuildStepImpl implements BuildStep { _logger ??= new Logger(input.id.toString()); return _logger; } + Logger _logger; /// The actual outputs of this build step. @@ -56,8 +59,11 @@ class BuildStepImpl implements BuildStep { /// Used internally for writing files. final AssetWriter _writer; - BuildStepImpl( - this.input, Iterable expectedOutputs, this._reader, this._writer) + /// The current root package, used for input/output validation. + final String _rootPackage; + + BuildStepImpl(this.input, Iterable expectedOutputs, this._reader, + this._writer, this._rootPackage) : expectedOutputs = new List.unmodifiable(expectedOutputs) { /// The [input] is always a dependency. _dependencies.add(input.id); @@ -65,6 +71,7 @@ class BuildStepImpl implements BuildStep { /// Checks if an [Asset] by [id] exists as an input for this [BuildStep]. Future hasInput(AssetId id) { + _checkInput(id); _dependencies.add(id); return _reader.hasInput(id); } @@ -72,6 +79,7 @@ class BuildStepImpl implements BuildStep { /// Reads an [Asset] by [id] as a [String] using [encoding]. @override Future readAsString(AssetId id, {Encoding encoding: UTF8}) { + _checkInput(id); _dependencies.add(id); return _reader.readAsString(id, encoding: encoding); } @@ -83,9 +91,7 @@ class BuildStepImpl implements BuildStep { /// [expectedOutputs]. @override void writeAsString(Asset asset, {Encoding encoding: UTF8}) { - if (!expectedOutputs.any((id) => id == asset.id)) { - throw new UnexpectedOutputException(asset); - } + _checkOutput(asset); _outputs.add(asset); var done = _writer.writeAsString(asset, encoding: encoding); _outputsCompleted = _outputsCompleted.then((_) => done); @@ -101,4 +107,23 @@ class BuildStepImpl implements BuildStep { await _outputsCompleted; await _logger?.clearListeners(); } + + /// Checks that [id] is a valid input, and throws an [InvalidInputException] + /// if its not. + void _checkInput(AssetId id) { + if (id.package != _rootPackage && !id.path.startsWith('lib/')) { + throw new InvalidInputException(id); + } + } + + /// Checks that [asset] is a valid output, and throws an + /// [InvalidOutputException] or [UnexcpectedOutputException] if it's not. + void _checkOutput(Asset asset) { + if (asset.id.package != _rootPackage) { + throw new InvalidOutputException(asset); + } + if (!expectedOutputs.any((id) => id == asset.id)) { + throw new UnexpectedOutputException(asset); + } + } } diff --git a/lib/src/generate/build.dart b/lib/src/generate/build.dart index 582e0ca93..8b8cdf46f 100644 --- a/lib/src/generate/build.dart +++ b/lib/src/generate/build.dart @@ -9,6 +9,7 @@ import 'package:logging/logging.dart'; import 'package:path/path.dart' as path; import '../asset/asset.dart'; +import '../asset/exceptions.dart'; import '../asset/cache.dart'; import '../asset/file_based.dart'; import '../asset/id.dart'; @@ -131,6 +132,7 @@ Future _deletePreviousOutputs(List> phaseGroups) async { } } } + /// Once the group is done, add all outputs so they can be used in the next /// phase. for (var outputId in groupOutputIds) { @@ -146,10 +148,10 @@ Future _deletePreviousOutputs(List> phaseGroups) async { 'which already exist on disk. This is likely because the `.build` ' 'folder was deleted.'); var done = false; - while(!done) { + while (!done) { stdout.write('Delete these files (y/n) (or list them (l))?: '); var input = stdin.readLineSync(); - switch(input) { + switch (input) { case 'y': stdout.writeln('Deleting files...'); await Future.wait(conflictingOutputs.map((output) { @@ -184,7 +186,7 @@ Future _runPhases(List> phaseGroups) async { for (var builder in phase.builders) { // TODO(jakemac): Optimize, we can run all the builders in a phase // at the same time instead of sequentially. - await for (var output in _runBuilder(builder, inputs)) { + await for (var output in _runBuilder(builder, inputs, allInputs)) { groupOutputs.add(output); outputs.add(output); } @@ -251,12 +253,23 @@ bool _isValidInput(AssetId input) { } /// Runs [builder] with [inputs] as inputs. -Stream _runBuilder(Builder builder, Iterable inputs) async* { - for (var input in inputs) { +Stream _runBuilder(Builder builder, Iterable primaryInputs, + Map> allInputs) async* { + for (var input in primaryInputs) { var expectedOutputs = builder.declareOutputs(input); + /// Validate [expectedOutputs]. + for (var output in expectedOutputs) { + if (output.package != _packageGraph.root.name) { + throw new InvalidOutputException(new Asset(output, '')); + } + if (allInputs[output.package]?.contains(output) == true) { + throw new InvalidOutputException(new Asset(output, '')); + } + } + var inputAsset = new Asset(input, await _reader.readAsString(input)); - var buildStep = - new BuildStepImpl(inputAsset, expectedOutputs, _reader, _writer); + var buildStep = new BuildStepImpl( + inputAsset, expectedOutputs, _reader, _writer, _packageGraph.root.name); await builder.build(buildStep); await buildStep.complete(); for (var output in buildStep.outputs) { diff --git a/lib/src/transformer/transformer.dart b/lib/src/transformer/transformer.dart index 89017d816..29f1af0ab 100644 --- a/lib/src/transformer/transformer.dart +++ b/lib/src/transformer/transformer.dart @@ -70,7 +70,8 @@ abstract class BuilderTransformer implements Transformer, DeclaringTransformer { } // Run the build step. - var buildStep = new BuildStepImpl(input, expected, reader, writer); + var buildStep = + new BuildStepImpl(input, expected, reader, writer, input.id.package); await builder.build(buildStep); await buildStep.complete(); })); diff --git a/test/analyzer/resolver_test.dart b/test/analyzer/resolver_test.dart index 42bcc8169..2a6a71e8b 100644 --- a/test/analyzer/resolver_test.dart +++ b/test/analyzer/resolver_test.dart @@ -17,14 +17,17 @@ import '../common/common.dart'; main() { var entryPoint = makeAssetId('a|web/main.dart'); Future validateResolver( - {Map inputs, validator(Resolver), List messages: const []}) async { + {Map inputs, + validator(Resolver), + List messages: const []}) async { var writer = new InMemoryAssetWriter(); var reader = new InMemoryAssetReader(writer.assets); var assets = makeAssets(inputs); addAssets(assets.values, writer); var builder = new TestBuilder(validator); - var buildStep = new BuildStepImpl(assets[entryPoint], [], reader, writer); + var buildStep = + new BuildStepImpl(assets[entryPoint], [], reader, writer, 'a'); var logs = []; if (messages != null) { buildStep.logger.onRecord.listen(logs.add); @@ -175,8 +178,7 @@ main() { // First from the AST walker '[WARNING] a|web/main.dart: $warningMessage "/b.dart"', '[WARNING] a|web/main.dart: $warningMessage "/b.dart"', - ], - validator: (resolver) { + ], validator: (resolver) { var lib = resolver.getLibrary(entryPoint); expect(lib.importedLibraries.length, 1); }); diff --git a/test/asset/file_based_test.dart b/test/asset/file_based_test.dart index d99f38915..63e5080b8 100644 --- a/test/asset/file_based_test.dart +++ b/test/asset/file_based_test.dart @@ -26,12 +26,8 @@ main() { 'world\n'); }); - test('can only read package dependency files in the lib dir', () async { + test('can read package dependency files in the lib dir', () async { expect(await reader.readAsString(makeAssetId('a|lib/a.txt')), 'A\n'); - expect(reader.readAsString(makeAssetId('a|web/a.txt')), - throwsA(invalidInputException)); - expect(reader.readAsString(makeAssetId('a|a.txt')), - throwsA(invalidInputException)); }); test('can check for existence of any application package files', () async { @@ -46,16 +42,10 @@ main() { await reader.hasInput(makeAssetId('basic_pkg|lib/a.txt')), isFalse); }); - test('can only check for existence of package dependency files in lib', + test('can check for existence of package dependency files in lib', () async { expect(await reader.hasInput(makeAssetId('a|lib/a.txt')), isTrue); expect(await reader.hasInput(makeAssetId('a|lib/b.txt')), isFalse); - expect(reader.hasInput(makeAssetId('a|web/a.txt')), - throwsA(invalidInputException)); - expect(reader.hasInput(makeAssetId('a|a.txt')), - throwsA(invalidInputException)); - expect(reader.hasInput(makeAssetId('foo|bar.txt')), - throwsA(invalidInputException)); }); test('throws when attempting to read a non-existent file', () async { @@ -105,25 +95,5 @@ main() { await writer.delete(asset.id); expect(await file.exists(), isFalse); }); - - test('can\'t output files in package dependencies', () async { - var asset = makeAsset('a|test.txt'); - expect(writer.writeAsString(asset), throwsA(invalidOutputException)); - }); - - test('can\'t output files in arbitrary packages', () async { - var asset = makeAsset('foo|bar.txt'); - expect(writer.writeAsString(asset), throwsA(invalidOutputException)); - }); - - test('can\'t delete files in package dependencies', () async { - var id = makeAssetId('a|test.txt'); - expect(writer.delete(id), throws); - }); - - test('can\'t delete files in arbitrary dependencies', () async { - var id = makeAssetId('foo|test.txt'); - expect(writer.delete(id), throws); - }); }); } diff --git a/test/builder/build_step_impl_test.dart b/test/builder/build_step_impl_test.dart index 4560fc582..fa78e2f08 100644 --- a/test/builder/build_step_impl_test.dart +++ b/test/builder/build_step_impl_test.dart @@ -23,7 +23,8 @@ main() { reader = new StubAssetReader(); writer = new StubAssetWriter(); primary = makeAsset(); - buildStep = new BuildStepImpl(primary, [], reader, writer); + buildStep = + new BuildStepImpl(primary, [], reader, writer, primary.id.package); }); test('tracks dependencies on read', () async { @@ -53,7 +54,8 @@ main() { test('tracks outputs', () async { var a1 = makeAsset(); var a2 = makeAsset(); - buildStep = new BuildStepImpl(primary, [a1.id, a2.id], reader, writer); + buildStep = new BuildStepImpl( + primary, [a1.id, a2.id], reader, writer, primary.id.package); buildStep.writeAsString(a1); expect(buildStep.outputs, [a1]); @@ -86,6 +88,33 @@ main() { '[SEVERE] ${primary.id}: goodbye', ]); }); + + test('hasInput throws invalidInputExceptions', () async { + expect(() => buildStep.hasInput(makeAssetId('b|web/a.txt')), + throwsA(invalidInputException)); + expect(() => buildStep.hasInput(makeAssetId('b|a.txt')), + throwsA(invalidInputException)); + expect(() => buildStep.hasInput(makeAssetId('foo|bar.txt')), + throwsA(invalidInputException)); + }); + + test('readAsString throws InvalidInputExceptions', () async { + expect(() => buildStep.readAsString(makeAssetId('b|web/a.txt')), + throwsA(invalidInputException)); + expect(() => buildStep.readAsString(makeAssetId('b|a.txt')), + throwsA(invalidInputException)); + expect(() => buildStep.readAsString(makeAssetId('foo|bar.txt')), + throwsA(invalidInputException)); + }); + + test('writeAsString throws InvalidOutputExceptions', () async { + var a1 = makeAsset('b|test.txt'); + expect( + () => buildStep.writeAsString(a1), throwsA(invalidOutputException)); + var a2 = makeAsset('foo|bar.txt'); + expect( + () => buildStep.writeAsString(a2), throwsA(invalidOutputException)); + }); }); group('with in memory file system', () { @@ -109,8 +138,8 @@ main() { }); addAssets(inputs.values, writer); var outputId = new AssetId.parse('$primary.combined'); - var buildStep = new BuildStepImpl( - inputs[new AssetId.parse(primary)], [outputId], reader, writer); + var buildStep = new BuildStepImpl(inputs[new AssetId.parse(primary)], + [outputId], reader, writer, 'a'); await fileCombiner.build(buildStep); await buildStep.complete(); @@ -140,8 +169,8 @@ main() { addAssets(inputs.values, writer); var primary = makeAssetId('a|web/a.dart'); - var buildStep = - new BuildStepImpl(inputs[primary], [], reader, writer); + var buildStep = new BuildStepImpl( + inputs[primary], [], reader, writer, primary.package); var resolver = await buildStep.resolve(primary); var aLib = resolver.getLibrary(primary); diff --git a/test/generate/build_test.dart b/test/generate/build_test.dart index 5ea8792eb..99c8f4ca0 100644 --- a/test/generate/build_test.dart +++ b/test/generate/build_test.dart @@ -118,9 +118,7 @@ main() { ]; await testPhases(phases, {'b|lib/b.txt': 'b'}, outputs: {}, status: BuildStatus.Failure); - }, - skip: 'Failing, InMemoryAssetWriter doesn\'t throw. Need to handle ' - 'this in BuildStep instead of AssetWriter.'); + }); }); test('multiple phases, inputs from multiple packages', () async { @@ -168,12 +166,16 @@ main() { new Phase([new CopyBuilder()], [new InputSet('a')]), ] ]; - await testPhases(phases, {'a|lib/a.txt': 'a', 'a|lib/a.txt.copy': 'a'}, + await testPhases( + phases, + { + 'a|lib/a.txt': 'a', + 'a|lib/a.txt.copy': 'a', + 'a|.build/build_outputs.json': '[]', + }, status: BuildStatus.Failure, exceptionMatcher: invalidOutputException); - }, - skip: 'InMemoryAssetWriter doesn\'t check this, should be handled at ' - 'the BuildStep level.'); + }); }); test('tracks previous outputs in a build_outputs.json file', () async {