From fc1063082b88074674d7ad4eedbda3232b7a214f Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Sat, 23 Dec 2017 13:58:41 -0800 Subject: [PATCH 1/8] WIP. --- build_runner/lib/src/generate/lock_file.dart | 26 ++++++++++++++++++++ 1 file changed, 26 insertions(+) create mode 100644 build_runner/lib/src/generate/lock_file.dart diff --git a/build_runner/lib/src/generate/lock_file.dart b/build_runner/lib/src/generate/lock_file.dart new file mode 100644 index 000000000..2c2fac88d --- /dev/null +++ b/build_runner/lib/src/generate/lock_file.dart @@ -0,0 +1,26 @@ +// 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_graph/package_graph.dart'; + +String _createHash() => + +/// Returns a future that completes with a hash when [graph] could be locked. +/// +/// This same hash is expected to be used with [clearLock] when the build +/// process is complete. Throws a `ConcurrentBuildException` if the lock could +/// not be created. +Future createLock(PackageGraph graph) { + +} + +/// Returns a future that completes after clearing the lock file for [graph]. +/// +/// If [hash] is provided, throws a `ConcurrentBuildException` if it does not +/// match the lock file created, and the lock file is not cleared/removed. +Future clearLock(PackageGraph graph, {String hash}) { + +} From fe6e51382122b0cc2f2eba5331b78a6f9577d8c1 Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Sat, 23 Dec 2017 15:06:45 -0800 Subject: [PATCH 2/8] Add lock-file support. --- .gitignore | 1 + build_runner/bin/build_runner.dart | 8 +- build_runner/lib/build_runner.dart | 1 + .../build_script_generate.dart | 3 +- build_runner/lib/src/generate/build_impl.dart | 2 + build_runner/lib/src/generate/exceptions.dart | 9 ++- build_runner/lib/src/generate/lock_file.dart | 48 ++++++++++-- build_runner/lib/src/generate/watch_impl.dart | 2 + .../test/generate/lock_file_test.dart | 78 +++++++++++++++++++ 9 files changed, 139 insertions(+), 13 deletions(-) create mode 100644 build_runner/test/generate/lock_file_test.dart diff --git a/.gitignore b/.gitignore index 046cfabbc..85cea2750 100644 --- a/.gitignore +++ b/.gitignore @@ -7,6 +7,7 @@ pubspec.lock # Files generated by dart tools .dart_tool doc/ +build.lock # Generated by tool/create_all_test.dart tool/test_all.dart diff --git a/build_runner/bin/build_runner.dart b/build_runner/bin/build_runner.dart index e304c2a81..db5f406b3 100644 --- a/build_runner/bin/build_runner.dart +++ b/build_runner/bin/build_runner.dart @@ -9,11 +9,17 @@ import 'package:io/io.dart'; import 'package:logging/logging.dart'; import 'package:build_runner/src/build_script_generate/build_script_generate.dart'; +import 'package:build_runner/src/generate/exceptions.dart'; import 'package:build_runner/src/logging/std_io_logging.dart'; Future main(List args) async { var logListener = Logger.root.onRecord.listen(stdIOLogListener); - await ensureBuildScript(); + try { + await ensureBuildScript(); + } on ConcurrentBuildException catch (e) { + Logger.root.log(Level.SEVERE, '$e'); + return; + } var dart = Platform.resolvedExecutable; final innerArgs = [scriptLocation]..addAll(args); if (stdioType(stdin) == StdioType.TERMINAL && diff --git a/build_runner/lib/build_runner.dart b/build_runner/lib/build_runner.dart index 138975a0a..273fb00b8 100644 --- a/build_runner/lib/build_runner.dart +++ b/build_runner/lib/build_runner.dart @@ -7,6 +7,7 @@ export 'src/asset/writer.dart'; export 'src/entrypoint/run.dart' show run; export 'src/generate/build.dart'; export 'src/generate/build_result.dart'; +export 'src/generate/lock_file.dart' show clearLock, createLock; export 'src/generate/performance_tracker.dart' show BuildPerformance, BuildActionPerformance; export 'src/package_graph/apply_builders.dart' diff --git a/build_runner/lib/src/build_script_generate/build_script_generate.dart b/build_runner/lib/src/build_script_generate/build_script_generate.dart index d3be71592..6a590a349 100644 --- a/build_runner/lib/src/build_script_generate/build_script_generate.dart +++ b/build_runner/lib/src/build_script_generate/build_script_generate.dart @@ -38,11 +38,12 @@ Future _generateBuildScript() async { /// Finds expressions to create all the [BuilderApplication] instances that /// should be applied packages in the build. /// -/// Adds `apply` expressions based on the BuildefDefinitions from any package +/// Adds `apply` expressions based on the BuilderDefinitions from any package /// which has a `build.yaml`. Future> _findBuilderApplications() async { final builderApplications = []; final packageGraph = new PackageGraph.forThisPackage(); + await createLock(packageGraph); final orderedPackages = stronglyConnectedComponents( [packageGraph.root], (node) => node.name, (node) => node.dependencies) .expand((c) => c); diff --git a/build_runner/lib/src/generate/build_impl.dart b/build_runner/lib/src/generate/build_impl.dart index 2dc0deeb9..56da94673 100644 --- a/build_runner/lib/src/generate/build_impl.dart +++ b/build_runner/lib/src/generate/build_impl.dart @@ -30,6 +30,7 @@ import 'build_result.dart'; import 'exceptions.dart'; import 'fold_frames.dart'; import 'heartbeat.dart'; +import 'lock_file.dart'; import 'options.dart'; import 'performance_tracker.dart'; import 'phase.dart'; @@ -71,6 +72,7 @@ Future build( await terminator.cancel(); await options.logListener.cancel(); + await clearLock(options.packageGraph); return result; } diff --git a/build_runner/lib/src/generate/exceptions.dart b/build_runner/lib/src/generate/exceptions.dart index 910b1779c..367b29692 100644 --- a/build_runner/lib/src/generate/exceptions.dart +++ b/build_runner/lib/src/generate/exceptions.dart @@ -8,11 +8,14 @@ import 'package:build_runner/src/generate/phase.dart'; import 'phase.dart'; class ConcurrentBuildException implements Exception { - const ConcurrentBuildException(); + final String lockFilePath; + + const ConcurrentBuildException(this.lockFilePath); @override - String toString() => - 'ConcurrentBuildException: Only one build may be running at a time.'; + String toString() => 'Only one build may be running at a time!\n' + 'If a build process has failed, or you want to remove the lock \n' + 'manually simple delete the file "$lockFilePath".'; } abstract class FatalBuildException implements Exception { diff --git a/build_runner/lib/src/generate/lock_file.dart b/build_runner/lib/src/generate/lock_file.dart index 2c2fac88d..3cd7f1107 100644 --- a/build_runner/lib/src/generate/lock_file.dart +++ b/build_runner/lib/src/generate/lock_file.dart @@ -3,24 +3,56 @@ // BSD-style license that can be found in the LICENSE file. import 'dart:async'; +import 'dart:io'; +import 'package:crypto/crypto.dart'; +import 'package:path/path.dart' as p; + +import '../generate/exceptions.dart'; import '../package_graph/package_graph.dart'; -String _createHash() => +/// Returns a simple string hash used to determine if a lock file is owned. +String _createHash() => md5 + .convert(pid.toString().codeUnits) + .bytes + .map((b) => b.toRadixString(16)) + .join(''); /// Returns a future that completes with a hash when [graph] could be locked. /// /// This same hash is expected to be used with [clearLock] when the build /// process is complete. Throws a `ConcurrentBuildException` if the lock could /// not be created. -Future createLock(PackageGraph graph) { - +Future createLock(PackageGraph graph, {String hash}) async { + // For package graphs without a root (usually for testing), just succeed. + hash ??= _createHash(); + if (graph?.root?.path == null) { + return hash; + } + final path = p.join(graph.root.path, 'build.lock'); + final file = new File(path); + if (await file.exists()) { + throw new ConcurrentBuildException(file.absolute.path); + } + await file.writeAsString(hash); + return hash; } /// Returns a future that completes after clearing the lock file for [graph]. -/// -/// If [hash] is provided, throws a `ConcurrentBuildException` if it does not -/// match the lock file created, and the lock file is not cleared/removed. -Future clearLock(PackageGraph graph, {String hash}) { - +Future clearLock(PackageGraph graph, {String hash}) async { + // For package graphs without a root (usually for testing), just succeed. + if (graph?.root?.path == null) { + return; + } + final path = p.join(graph.root.path, 'build.lock'); + final file = new File(path); + if (!await file.exists()) { + return; + } + final input = await file.readAsString(); + hash ??= _createHash(); + if (hash != input) { + throw new ConcurrentBuildException(file.absolute.path); + } + await file.delete(); } diff --git a/build_runner/lib/src/generate/watch_impl.dart b/build_runner/lib/src/generate/watch_impl.dart index 0743a2c2e..97500e9d7 100644 --- a/build_runner/lib/src/generate/watch_impl.dart +++ b/build_runner/lib/src/generate/watch_impl.dart @@ -25,6 +25,7 @@ import 'build_definition.dart'; import 'build_impl.dart'; import 'build_result.dart'; import 'directory_watcher_factory.dart'; +import 'lock_file.dart'; import 'options.dart'; import 'phase.dart'; import 'terminator.dart'; @@ -71,6 +72,7 @@ Future watch( watch.buildResults.drain().then((_) async { await terminator.cancel(); await options.logListener.cancel(); + await clearLock(options.packageGraph); }); return createServeHandler(watch); diff --git a/build_runner/test/generate/lock_file_test.dart b/build_runner/test/generate/lock_file_test.dart new file mode 100644 index 000000000..e4eb2d7f2 --- /dev/null +++ b/build_runner/test/generate/lock_file_test.dart @@ -0,0 +1,78 @@ +// 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:build_runner/src/generate/exceptions.dart'; +import 'package:build_runner/src/generate/lock_file.dart'; +import 'package:build_runner/src/package_graph/package_graph.dart'; +import 'package:path/path.dart' as p; +import 'package:test/test.dart'; +import 'package:test_descriptor/test_descriptor.dart' as d; + +import '../common/descriptors.dart'; + +void main() { + PackageGraph graph; + + setUp(() async { + await d.dir('pkg', [ + d.dir('lib'), + d.file('.packages', 'pkg:./lib/'), + await pubspec('pkg'), + ]).create(); + + final root = new PackageNode('pkg', p.join(d.sandbox, 'pkg'), isRoot: true); + graph = new PackageGraph.fromRoot(root); + expect(graph.root.path, isNotNull); + }); + + test('should create a lock file', () async { + // Start with no lock file. + await d.dir('pkg', [ + d.nothing('build.lock'), + ]).validate(); + + // Create a lock file. + await createLock(graph, hash: '1234'); + + // Expect a lock file. + await d.dir('pkg', [ + d.file('build.lock', '1234'), + ]).validate(); + }); + + test('should throw if a lock file already exists', () async { + await d.dir('pkg', [ + d.file('build.lock', '1234'), + ]).create(); + + expect(createLock(graph), throwsConcurrentBuildException); + }); + + test('should remove a lock file', () async { + await d.dir('pkg', [ + d.file('build.lock', '1234'), + ]).create(); + + await clearLock(graph, hash: '1234'); + + await d.dir('pkg', [ + d.nothing('build.lock'), + ]).validate(); + }); + + test('should throw on removing a lock file with a wrong hash', () async { + await d.dir('pkg', [ + d.file('build.lock', '1234'), + ]).create(); + + expect(clearLock(graph, hash: 'ABCD'), throwsConcurrentBuildException); + + await d.dir('pkg', [ + d.file('build.lock', '1234'), + ]).validate(); + }); +} + +final throwsConcurrentBuildException = + throwsA(const isInstanceOf()); From 27b22e1ead110ba9bc48f60870c4c21a2771d7d7 Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Sat, 23 Dec 2017 15:09:06 -0800 Subject: [PATCH 3/8] Update CHANGELOG/pubspec.yaml. --- build_runner/CHANGELOG.md | 6 +++++- build_runner/pubspec.yaml | 2 +- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/build_runner/CHANGELOG.md b/build_runner/CHANGELOG.md index d02b45d74..e4d171bdc 100644 --- a/build_runner/CHANGELOG.md +++ b/build_runner/CHANGELOG.md @@ -1,8 +1,12 @@ -## 0.7.1+1 +## 0.7.2 - **BUG FIX**: Running the `build_runner` binary without arguments no longer causes a crash saying `Could not find an option named "assume-tty".`. +- Builds now automatically generate a `build.lock` file in the package + directory while a build is in progress (including watchers and servers), and + fail if a lock file is already created. + ## 0.7.1 - Run Builders which write to the source tree before those which write to the diff --git a/build_runner/pubspec.yaml b/build_runner/pubspec.yaml index e9123abbd..c41ad4a39 100644 --- a/build_runner/pubspec.yaml +++ b/build_runner/pubspec.yaml @@ -1,5 +1,5 @@ name: build_runner -version: 0.7.1+1 +version: 0.7.2 description: Tools to write binaries that run builders. author: Dart Team homepage: https://github.com/dart-lang/build From 83f0a941c4f71e9d724944dfdaff9f3abd51ce42 Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Sat, 23 Dec 2017 15:57:10 -0800 Subject: [PATCH 4/8] Add exit code. --- build_runner/bin/build_runner.dart | 1 + 1 file changed, 1 insertion(+) diff --git a/build_runner/bin/build_runner.dart b/build_runner/bin/build_runner.dart index a27268b35..054327c51 100644 --- a/build_runner/bin/build_runner.dart +++ b/build_runner/bin/build_runner.dart @@ -18,6 +18,7 @@ Future main(List args) async { await ensureBuildScript(); } on ConcurrentBuildException catch (e) { Logger.root.log(Level.SEVERE, '$e'); + exitCode = 1; return; } var dart = Platform.resolvedExecutable; From 82590eeb21ebc00d913a47805014ea366ce0b149 Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Sat, 23 Dec 2017 16:01:51 -0800 Subject: [PATCH 5/8] Add missing lock clear. --- build_runner/lib/src/generate/watch_impl.dart | 1 + 1 file changed, 1 insertion(+) diff --git a/build_runner/lib/src/generate/watch_impl.dart b/build_runner/lib/src/generate/watch_impl.dart index 97500e9d7..3bbfeadb1 100644 --- a/build_runner/lib/src/generate/watch_impl.dart +++ b/build_runner/lib/src/generate/watch_impl.dart @@ -159,6 +159,7 @@ class WatchImpl implements BuildState { var terminate = Future.any([until, fatalBuildCompleter.future]).then((_) { _logger.info('Terminating. No further builds will be scheduled\n'); + return clearLock(options.packageGraph); }); // Start watching files immediately, before the first build is even started. From 7512d9b7e00932a507edc8d2a96f78735fe32b08 Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Sat, 23 Dec 2017 16:08:52 -0800 Subject: [PATCH 6/8] Move into right place. --- build_runner/lib/src/generate/watch_impl.dart | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/build_runner/lib/src/generate/watch_impl.dart b/build_runner/lib/src/generate/watch_impl.dart index 3bbfeadb1..397f63a9f 100644 --- a/build_runner/lib/src/generate/watch_impl.dart +++ b/build_runner/lib/src/generate/watch_impl.dart @@ -151,6 +151,7 @@ class WatchImpl implements BuildState { .hasBeenUpdated(mergedChanges.keys.toSet())) { fatalBuildCompleter.complete(); _logger.severe('Terminating builds due to build script update'); + await clearLock(options.packageGraph); return new BuildResult(BuildStatus.failure, []); } } @@ -159,7 +160,6 @@ class WatchImpl implements BuildState { var terminate = Future.any([until, fatalBuildCompleter.future]).then((_) { _logger.info('Terminating. No further builds will be scheduled\n'); - return clearLock(options.packageGraph); }); // Start watching files immediately, before the first build is even started. From 4ff5d03440bc697e967a3bd4111049ea8bbcf947 Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Sat, 23 Dec 2017 16:15:37 -0800 Subject: [PATCH 7/8] So many places for the build to end... --- build_runner/lib/src/generate/watch_impl.dart | 1 + 1 file changed, 1 insertion(+) diff --git a/build_runner/lib/src/generate/watch_impl.dart b/build_runner/lib/src/generate/watch_impl.dart index 397f63a9f..181e87f07 100644 --- a/build_runner/lib/src/generate/watch_impl.dart +++ b/build_runner/lib/src/generate/watch_impl.dart @@ -160,6 +160,7 @@ class WatchImpl implements BuildState { var terminate = Future.any([until, fatalBuildCompleter.future]).then((_) { _logger.info('Terminating. No further builds will be scheduled\n'); + return clearLock(options.packageGraph); }); // Start watching files immediately, before the first build is even started. From 81adc4d891508bb0d96b886200d9aac5c0b27df0 Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Sat, 23 Dec 2017 16:34:24 -0800 Subject: [PATCH 8/8] Again. --- build_runner/lib/src/generate/watch_impl.dart | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/build_runner/lib/src/generate/watch_impl.dart b/build_runner/lib/src/generate/watch_impl.dart index 181e87f07..a47f1f5a8 100644 --- a/build_runner/lib/src/generate/watch_impl.dart +++ b/build_runner/lib/src/generate/watch_impl.dart @@ -66,7 +66,8 @@ Future watch( final buildActions = await createBuildActions(options.packageGraph, builders, overrideBuildConfig: overrideBuildConfig); - var watch = runWatch(options, buildActions, terminator.shouldTerminate); + var watch = runWatch(options, buildActions, + terminator.shouldTerminate.then((_) => clearLock(options.packageGraph))); // ignore: unawaited_futures watch.buildResults.drain().then((_) async { @@ -160,7 +161,6 @@ class WatchImpl implements BuildState { var terminate = Future.any([until, fatalBuildCompleter.future]).then((_) { _logger.info('Terminating. No further builds will be scheduled\n'); - return clearLock(options.packageGraph); }); // Start watching files immediately, before the first build is even started. @@ -186,6 +186,7 @@ class WatchImpl implements BuildState { await currentBuild; await _buildDefinition.resourceManager.beforeExit(); await controller.close(); + await clearLock(options.packageGraph); _logger.info('Builds finished. Safe to exit\n'); });