From 10bd1e55dba79c2b7865c896ed0c42c2c9183e05 Mon Sep 17 00:00:00 2001 From: Jacob MacDonald <jakemac@google.com> Date: Thu, 4 May 2017 10:56:40 -0700 Subject: [PATCH] Add the DartDevcModuleTransformer (#1580) Compiles js modules with dartdevc based on `.moduleConfig` files and the corresponding linked summaries from the `LinkedSummaryTransformer`. --- lib/src/barback/asset_environment.dart | 9 +- .../dartdevc/dartdevc_module_transformer.dart | 125 ++++++++++++++++++ .../dartdevc/linked_summary_transformer.dart | 22 +-- lib/src/barback/dartdevc/module.dart | 45 +++++-- lib/src/barback/dartdevc/module_computer.dart | 13 +- .../unlinked_summary_transformer.dart | 19 +-- lib/src/barback/dartdevc/workers.dart | 4 + lib/src/command/barback.dart | 1 - lib/src/io.dart | 1 - .../dartdevc_module_transformer_test.dart | 59 +++++++++ test/barback/dartdevc/util.dart | 12 +- test/compiler/compiler_flag_test.dart | 22 ++- 12 files changed, 274 insertions(+), 58 deletions(-) create mode 100644 lib/src/barback/dartdevc/dartdevc_module_transformer.dart create mode 100644 test/barback/dartdevc/dartdevc_module_transformer_test.dart diff --git a/lib/src/barback/asset_environment.dart b/lib/src/barback/asset_environment.dart index 0ef3ba98..5cfb7805 100644 --- a/lib/src/barback/asset_environment.dart +++ b/lib/src/barback/asset_environment.dart @@ -18,6 +18,7 @@ import '../package.dart'; import '../package_graph.dart'; import '../source/cached.dart'; import '../utils.dart'; +import 'dartdevc/dartdevc_module_transformer.dart'; import 'dartdevc/linked_summary_transformer.dart'; import 'dartdevc/module_config_transformer.dart'; import 'dartdevc/unlinked_summary_transformer.dart'; @@ -177,9 +178,7 @@ class AssetEnvironment { /// Gets the built-in [Transformer]s or [AggregateTransformer]s that should be /// added to [package]. /// - /// These are returned as an [Iterable<Set>] to represent each phase (the - /// outer [Iterable]), and the transformers that should be ran in each phase ( - /// the inner [Set]). + /// Returns `null` if there are none. Iterable<Set> getBuiltInTransformers(Package package) { var transformers = <Set>[]; @@ -188,7 +187,8 @@ class AssetEnvironment { transformers.addAll([ [new ModuleConfigTransformer()], [new UnlinkedSummaryTransformer()], - [new LinkedSummaryTransformer()] + [new LinkedSummaryTransformer()], + [new DartDevcModuleTransformer(mode)], ].map((list) => list.toSet())); break; case Compiler.dart2JS: @@ -211,6 +211,7 @@ class AssetEnvironment { } } } + return transformers; } diff --git a/lib/src/barback/dartdevc/dartdevc_module_transformer.dart b/lib/src/barback/dartdevc/dartdevc_module_transformer.dart new file mode 100644 index 00000000..3c2a4495 --- /dev/null +++ b/lib/src/barback/dartdevc/dartdevc_module_transformer.dart @@ -0,0 +1,125 @@ +// Copyright (c) 2017, the Dart project authors. Please see the AUTHORS file +// for details. All rights reserved. Use of this source code is governed by a +// BSD-style license that can be found in the LICENSE file. + +import 'dart:async'; + +import 'package:barback/barback.dart'; +import 'package:bazel_worker/bazel_worker.dart'; +import 'package:path/path.dart' as p; + +import 'workers.dart'; +import 'module.dart'; +import 'module_reader.dart'; +import 'scratch_space.dart'; +import 'linked_summary_transformer.dart'; + +/// Creates dartdevc modules given [moduleConfigName] files which describe a set +/// of [Module]s. +/// +/// Linked summaries should have already been created for all modules and will +/// be read in to create the modules. +class DartDevcModuleTransformer extends Transformer { + @override + String get allowedExtensions => moduleConfigName; + + final BarbackMode mode; + + DartDevcModuleTransformer(this.mode); + + @override + Future apply(Transform transform) async { + ScratchSpace scratchSpace; + var reader = new ModuleReader(transform.readInputAsString); + var modules = await reader.readModules(transform.primaryInput.id); + try { + var allAssetIds = new Set<AssetId>(); + var summariesForModule = <ModuleId, Set<AssetId>>{}; + for (var module in modules) { + var transitiveModuleDeps = await reader.readTransitiveDeps(module); + var linkedSummaryIds = + transitiveModuleDeps.map((depId) => depId.linkedSummaryId).toSet(); + summariesForModule[module.id] = linkedSummaryIds; + allAssetIds..addAll(module.assetIds)..addAll(linkedSummaryIds); + } + // Create a single temp environment for all the modules in this package. + scratchSpace = + await ScratchSpace.create(allAssetIds, transform.readInput); + await Future.wait(modules.map((m) => _createDartdevcModule( + m, scratchSpace, summariesForModule[m.id], mode, transform))); + } finally { + scratchSpace?.delete(); + } + } +} + +/// Compiles [module] using the `dartdevc` binary from the SDK to a relative +/// path under the package that looks like `$outputDir/${module.id.name}.js`. +Future _createDartdevcModule( + Module module, + ScratchSpace scratchSpace, + Set<AssetId> linkedSummaryIds, + BarbackMode mode, + Transform transform) async { + var jsOutputFile = scratchSpace.fileFor(module.id.jsId); + var sdk_summary = p.url.join(sdkDir.path, 'lib/_internal/ddc_sdk.sum'); + var request = new WorkRequest(); + request.arguments.addAll([ + '--dart-sdk-summary=$sdk_summary', + // TODO(jakemac53): Remove when no longer needed, + // https://github.com/dart-lang/pub/issues/1583. + '--unsafe-angular2-whitelist', + '--modules=amd', + '--dart-sdk=${sdkDir.path}', + '--module-root=${scratchSpace.tempDir.path}', + '--library-root=${p.dirname(jsOutputFile.path)}', + '--summary-extension=${linkedSummaryExtension.substring(1)}', + '-o', + jsOutputFile.path, + ]); + // Add all the linked summaries as summary inputs. + for (var id in linkedSummaryIds) { + request.arguments.addAll(['-s', scratchSpace.fileFor(id).path]); + } + // Add URL mappings for all the package: files to tell DartDevc where to find + // them. + for (var id in module.assetIds) { + var uri = canonicalUriFor(id); + if (uri.startsWith('package:')) { + request.arguments + .add('--url-mapping=$uri,${scratchSpace.fileFor(id).path}'); + } + } + // And finally add all the urls to compile, using the package: path for files + // under lib and the full absolute path for other files. + request.arguments.addAll(module.assetIds.map((id) { + var uri = canonicalUriFor(id); + if (uri.startsWith('package:')) { + return uri; + } + return scratchSpace.fileFor(id).path; + })); + + var response = await dartdevcDriver.doWork(request); + + // TODO(jakemac53): Fix the ddc worker mode so it always sends back a bad + // status code if something failed. Today we just make sure there is an output + // js file to verify it was successful. + if (response.exitCode != EXIT_CODE_OK || !jsOutputFile.existsSync()) { + var message = 'Error compiling dartdevc module: ${module.id}.\n' + '${response.output}'; + // We only log warnings in debug mode for ddc modules because they don't all + // need to compile successfully, only the ones imported by an entrypoint do. + switch (mode) { + case BarbackMode.DEBUG: + transform.logger.warning(message); + break; + case BarbackMode.RELEASE: + transform.logger.error(message); + break; + } + } else { + transform.addOutput( + new Asset.fromBytes(module.id.jsId, jsOutputFile.readAsBytesSync())); + } +} diff --git a/lib/src/barback/dartdevc/linked_summary_transformer.dart b/lib/src/barback/dartdevc/linked_summary_transformer.dart index 040fc347..2980536b 100644 --- a/lib/src/barback/dartdevc/linked_summary_transformer.dart +++ b/lib/src/barback/dartdevc/linked_summary_transformer.dart @@ -6,14 +6,11 @@ import 'dart:async'; import 'package:barback/barback.dart'; import 'package:bazel_worker/bazel_worker.dart'; -import 'package:path/path.dart' as p; -import '../../io.dart'; import 'workers.dart'; import 'module.dart'; import 'module_reader.dart'; import 'temp_environment.dart'; -import 'unlinked_summary_transformer.dart'; final String linkedSummaryExtension = '.linked.sum'; @@ -39,20 +36,16 @@ class LinkedSummaryTransformer extends Transformer { var summariesForModule = <ModuleId, Set<AssetId>>{}; for (var module in modules) { var transitiveModuleDeps = await reader.readTransitiveDeps(module); - var unlinkedSummaryIds = transitiveModuleDeps.map((depId) { - assert(depId.name.isNotEmpty); - var summaryDir = depId.name.split('__').first; - return new AssetId(depId.package, - p.join(summaryDir, '${depId.name}$unlinkedSummaryExtension')); - }).toSet(); + var unlinkedSummaryIds = transitiveModuleDeps + .map((depId) => depId.unlinkedSummaryId) + .toSet(); summariesForModule[module.id] = unlinkedSummaryIds; allAssetIds..addAll(module.assetIds)..addAll(unlinkedSummaryIds); } // Create a single temp environment for all the modules in this package. tempEnv = await TempEnvironment.create(allAssetIds, transform.readInput); - var outputDir = topLevelDir(configId.path); await Future.wait(modules.map((m) => _createLinkedSummaryForModule( - m, summariesForModule[m.id], outputDir, tempEnv, transform))); + m, summariesForModule[m.id], tempEnv, transform))); } finally { tempEnv?.delete(); } @@ -62,12 +55,9 @@ class LinkedSummaryTransformer extends Transformer { Future _createLinkedSummaryForModule( Module module, Set<AssetId> unlinkedSummaryIds, - String outputDir, TempEnvironment tempEnv, Transform transform) async { - var summaryOutputId = new AssetId(module.id.package, - p.url.join(outputDir, '${module.id.name}$linkedSummaryExtension')); - var summaryOutputFile = tempEnv.fileFor(summaryOutputId); + var summaryOutputFile = tempEnv.fileFor(module.id.linkedSummaryId); var request = new WorkRequest(); request.arguments.addAll([ '--build-summary-only', @@ -92,6 +82,6 @@ Future _createLinkedSummaryForModule( '${response.output}'); } else { transform.addOutput(new Asset.fromBytes( - summaryOutputId, summaryOutputFile.readAsBytesSync())); + module.id.linkedSummaryId, summaryOutputFile.readAsBytesSync())); } } diff --git a/lib/src/barback/dartdevc/module.dart b/lib/src/barback/dartdevc/module.dart index 4577174f..4cb852a9 100644 --- a/lib/src/barback/dartdevc/module.dart +++ b/lib/src/barback/dartdevc/module.dart @@ -3,6 +3,10 @@ // BSD-style license that can be found in the LICENSE file. import 'package:barback/barback.dart'; +import 'package:path/path.dart' as p; + +import 'linked_summary_transformer.dart'; +import 'unlinked_summary_transformer.dart'; /// Serializable object that describes a single `module`. /// @@ -53,12 +57,24 @@ directDependencies: $directDependencies'''; /// Serializable identifier of a [Module]. /// /// A [Module] can only be a part of a single package, and must have a unique -/// name within that package. +/// [name] within that package. +/// +/// The [dir] is the top level directory under the package where the module +/// lives (such as `lib`, `web`, `test`, etc). class ModuleId { - final String package; + final String dir; final String name; + final String package; - const ModuleId(this.package, this.name); + AssetId get unlinkedSummaryId => + _moduleAssetWithExtension(unlinkedSummaryExtension); + + AssetId get linkedSummaryId => + _moduleAssetWithExtension(linkedSummaryExtension); + + AssetId get jsId => _moduleAssetWithExtension('.js'); + + const ModuleId(this.package, this.name, this.dir); /// Creates a [ModuleId] from [json] which should be a [List] that was created /// with [toJson]. @@ -67,22 +83,31 @@ class ModuleId { /// fields in that order. ModuleId.fromJson(List<String> json) : package = json[0], - name = json[1]; + name = json[1], + dir = json[2]; /// Serialize this [ModuleId] to a nested [List] which can be encoded with /// `JSON.encode` and then decoded later with `JSON.decode`. /// - /// The resulting [List] will have 2 values, representing the [package] and - /// [name] fields in that order. - List<String> toJson() => <String>[package, name]; + /// The resulting [List] will have 3 values, representing the [package], + /// [name], and [dir] fields in that order. + List<String> toJson() => <String>[package, name, dir]; @override - String toString() => 'ModuleId: $package|$name'; + String toString() => 'ModuleId: $package|$dir/$name'; @override bool operator ==(other) => - other is ModuleId && other.package == package && other.name == this.name; + other is ModuleId && + other.package == package && + other.name == name && + other.dir == dir; @override - int get hashCode => package.hashCode ^ name.hashCode; + int get hashCode => package.hashCode ^ name.hashCode ^ dir.hashCode; + + /// Returns an asset for this module with the given [extension]. + AssetId _moduleAssetWithExtension(String extension) { + return new AssetId(package, p.join(dir, '$name$extension')); + } } diff --git a/lib/src/barback/dartdevc/module_computer.dart b/lib/src/barback/dartdevc/module_computer.dart index 49b85040..9721866b 100644 --- a/lib/src/barback/dartdevc/module_computer.dart +++ b/lib/src/barback/dartdevc/module_computer.dart @@ -233,7 +233,8 @@ class _ModuleComputer { orElse: () => sortedNodes.first); var moduleName = p.url.split(p.withoutExtension(primaryNode.id.path)).join('__'); - var id = new ModuleId(primaryNode.id.package, moduleName); + var id = new ModuleId( + primaryNode.id.package, moduleName, topLevelDir(primaryNode.id.path)); // Expand to include all the part files of each node, these aren't // included as individual `_AssetNodes`s in `connectedComponents`. var allAssetIds = componentNodes @@ -336,8 +337,8 @@ class _ModuleComputer { // Create a new module based off the name of all entrypoints or merge into // an existing one by that name. var moduleNames = entrypointIds.map((id) => id.name).toList()..sort(); - var newModuleId = - new ModuleId(entrypointIds.first.package, moduleNames.join('\$')); + var newModuleId = new ModuleId(entrypointIds.first.package, + moduleNames.join('\$'), entrypointIds.first.dir); var newModule = modulesById.putIfAbsent( newModuleId, () => @@ -359,13 +360,11 @@ class _ModuleComputer { List<Module> _renameSharedModules(List<Module> modules) { if (modules.isEmpty) return modules; var next = 0; - // All modules and assets in those modules share a top level dir, we just - // grab it from the first one. - var moduleDir = topLevelDir(modules.first.assetIds.first.path); return modules.map((module) { if (module.id.name.contains('\$')) { return new Module( - new ModuleId(module.id.package, '${moduleDir}__shared_${next++}'), + new ModuleId(module.id.package, + '${module.id.dir}__shared_${next++}', module.id.dir), module.assetIds, module.directDependencies); } else { diff --git a/lib/src/barback/dartdevc/unlinked_summary_transformer.dart b/lib/src/barback/dartdevc/unlinked_summary_transformer.dart index bcd5c648..2bce94d1 100644 --- a/lib/src/barback/dartdevc/unlinked_summary_transformer.dart +++ b/lib/src/barback/dartdevc/unlinked_summary_transformer.dart @@ -6,9 +6,7 @@ import 'dart:async'; import 'package:barback/barback.dart'; import 'package:bazel_worker/bazel_worker.dart'; -import 'package:path/path.dart' as p; -import '../../io.dart'; import 'workers.dart'; import 'module.dart'; import 'module_reader.dart'; @@ -36,21 +34,18 @@ class UnlinkedSummaryTransformer extends Transformer { return allAssets; }); // Create a single temp environment for all the modules in this package. - scratchSpace = - await ScratchSpace.create(allAssetIds, transform.readInput); - await Future.wait(modules.map((m) => _createUnlinkedSummaryForModule( - m, topLevelDir(configId.path), scratchSpace, transform))); + scratchSpace = await ScratchSpace.create(allAssetIds, transform.readInput); + await Future.wait(modules + .map((m) => _createUnlinkedSummaryForModule(m, scratchSpace, transform))); } finally { scratchSpace?.delete(); } } } -Future _createUnlinkedSummaryForModule(Module module, String outputDir, - ScratchSpace scratchSpace, Transform transform) async { - var summaryOutputId = new AssetId(module.id.package, - p.url.join(outputDir, '${module.id.name}$unlinkedSummaryExtension')); - var summaryOutputFile = scratchSpace.fileFor(summaryOutputId); +Future _createUnlinkedSummaryForModule( + Module module, ScratchSpace scratchSpace, Transform transform) async { + var summaryOutputFile = scratchSpace.fileFor(module.id.unlinkedSummaryId); var request = new WorkRequest(); request.arguments.addAll([ '--build-summary-only', @@ -73,6 +68,6 @@ Future _createUnlinkedSummaryForModule(Module module, String outputDir, '${response.output}'); } else { transform.addOutput(new Asset.fromBytes( - summaryOutputId, summaryOutputFile.readAsBytesSync())); + module.id.unlinkedSummaryId, summaryOutputFile.readAsBytesSync())); } } diff --git a/lib/src/barback/dartdevc/workers.dart b/lib/src/barback/dartdevc/workers.dart index 383ac273..587f10a7 100644 --- a/lib/src/barback/dartdevc/workers.dart +++ b/lib/src/barback/dartdevc/workers.dart @@ -13,4 +13,8 @@ final analyzerDriver = new BazelWorkerDriver(() => Process.start( p.join(sdkDir.path, 'bin', 'dartanalyzer'), ['--build-mode', '--persistent_worker'])); +/// Manages a shared set of persistent dartdevc workers. +final dartdevcDriver = new BazelWorkerDriver(() => Process + .start(p.join(sdkDir.path, 'bin', 'dartdevc'), ['--persistent_worker'])); + final sdkDir = cli_util.getSdkDir(); diff --git a/lib/src/command/barback.dart b/lib/src/command/barback.dart index 284a9a0a..323649bc 100644 --- a/lib/src/command/barback.dart +++ b/lib/src/command/barback.dart @@ -25,7 +25,6 @@ abstract class BarbackCommand extends PubCommand { /// The build mode. BarbackMode get mode => new BarbackMode(argResults["mode"]); - /// The current compiler mode. Compiler get compiler { if (argResults.options.contains('dart2js') && diff --git a/lib/src/io.dart b/lib/src/io.dart index b51adec4..3b171123 100644 --- a/lib/src/io.dart +++ b/lib/src/io.dart @@ -1133,7 +1133,6 @@ class PubProcessResult { bool get success => exitCode == exit_codes.SUCCESS; } - /// Returns the top level directory in [uri]. /// /// Throws an [ArgumentError] if [uri] is just a filename with no directory. diff --git a/test/barback/dartdevc/dartdevc_module_transformer_test.dart b/test/barback/dartdevc/dartdevc_module_transformer_test.dart new file mode 100644 index 00000000..0bc0b205 --- /dev/null +++ b/test/barback/dartdevc/dartdevc_module_transformer_test.dart @@ -0,0 +1,59 @@ +// Copyright (c) 2017, the Dart project authors. Please see the AUTHORS file +// for details. All rights reserved. Use of this source code is governed by a +// BSD-style license that can be found in the LICENSE file. + +import 'package:scheduled_test/scheduled_test.dart'; + +import '../../descriptor.dart' as d; +import '../../test_pub.dart'; +import '../../serve/utils.dart'; + +main() { + integration("can compile js files for modules under lib and web", () { + d.dir("foo", [ + d.libPubspec("foo", "1.0.0"), + d.dir("lib", [ + d.file( + "foo.dart", + """ + String get message => 'hello'; + """) + ]), + ]).create(); + + d.dir(appPath, [ + d.appPubspec({ + "foo": {"path": "../foo"} + }), + d.dir("lib", [ + d.file( + "hello.dart", + """ +import 'package:foo/foo.dart'; + +hello() => message; +""") + ]), + d.dir("web", [ + d.file( + "main.dart", + """ +import 'package:myapp/hello.dart'; + +void main() { + print(hello()); +} +""") + ]) + ]).create(); + + pubGet(); + pubServe(args: ['--compiler', 'dartdevc']); + // Just confirm some basic things are present indicating that the module + // was compiled. The goal here is not to test dartdevc itself. + requestShouldSucceed('web__main.js', contains('main')); + requestShouldSucceed('packages/myapp/lib__hello.js', contains('hello')); + requestShouldSucceed('packages/foo/lib__foo.js', contains('message')); + endPubServe(); + }); +} diff --git a/test/barback/dartdevc/util.dart b/test/barback/dartdevc/util.dart index f885eaaa..13d976e3 100644 --- a/test/barback/dartdevc/util.dart +++ b/test/barback/dartdevc/util.dart @@ -10,6 +10,8 @@ import 'package:test/test.dart'; import 'package:pub/src/barback/dartdevc/module.dart'; import 'package:pub/src/barback/dartdevc/module_reader.dart'; +import 'package:pub/src/io.dart' as io_helpers; + // Keep incrementing ids so we don't accidentally create duplicates. int _next = 0; @@ -37,10 +39,11 @@ Set<AssetId> makeAssetIds({String package, String topLevelDir}) => new Set<AssetId>.from(new List.generate( 10, (_) => makeAssetId(package: package, topLevelDir: topLevelDir))); -ModuleId makeModuleId({String name, String package}) { +ModuleId makeModuleId({String dir, String name, String package}) { package ??= 'pkg_${_next++}'; name ??= 'name_${_next++}'; - return new ModuleId(package, name); + dir ??= 'lib'; + return new ModuleId(package, name, dir); } Set<ModuleId> makeModuleIds({String package}) => new Set<ModuleId>.from( @@ -59,7 +62,10 @@ Module makeModule( return new AssetId.parse(id); } - var id = makeModuleId(package: package, name: name); + topLevelDir ??= srcs?.isEmpty != false + ? 'lib' + : io_helpers.topLevelDir(toAssetId(srcs.first).path); + var id = makeModuleId(package: package, name: name, dir: topLevelDir); srcs ??= makeAssetIds(package: id.package, topLevelDir: topLevelDir); var assetIds = srcs.map(toAssetId).toSet(); directDependencies ??= new Set(); diff --git a/test/compiler/compiler_flag_test.dart b/test/compiler/compiler_flag_test.dart index d69c620d..28e3a0ad 100644 --- a/test/compiler/compiler_flag_test.dart +++ b/test/compiler/compiler_flag_test.dart @@ -5,6 +5,7 @@ import 'package:scheduled_test/scheduled_stream.dart'; import 'package:scheduled_test/scheduled_test.dart'; +import 'package:pub/src/barback/dartdevc/module_reader.dart'; import 'package:pub/src/exit_codes.dart'; import '../descriptor.dart' as d; @@ -17,18 +18,31 @@ main() { d.appPubspec(), d.dir("lib", [ d.file("hello.dart", "hello() => print('hello');"), - ]) + ]), + d.dir("web", [ + d.file( + "main.dart", + ''' + import 'package:myapp/hello.dart'; + + void main() => hello(); + '''), + ]), ]).create(); pubGet(); pubServe(args: ['--compiler', 'dartdevc']); requestShouldSucceed( - 'packages/$appPath/.moduleConfig', contains('lib__hello')); - // Binary response, just confirm it exists. + 'packages/$appPath/$moduleConfigName', contains('lib__hello')); + requestShouldSucceed(moduleConfigName, contains('web__main')); requestShouldSucceed('packages/$appPath/lib__hello.unlinked.sum', null); + requestShouldSucceed('web__main.unlinked.sum', null); requestShouldSucceed('packages/$appPath/lib__hello.linked.sum', null); + requestShouldSucceed('web__main.linked.sum', null); + requestShouldSucceed('packages/$appPath/lib__hello.js', contains('hello')); + requestShouldSucceed('web__main.js', contains('hello')); // TODO(jakemac53): Not implemented yet, update once available. - requestShould404('packages/$appPath/lib__hello.js'); + requestShould404('main.dart.js'); endPubServe(); }); -- GitLab