Skip to content

Commit

Permalink
Add a check for empty kernel files
Browse files Browse the repository at this point in the history
Towards #2362

We don't know where the zero byte files are being introduced to the
system - if they are coming from the kernel worker try to catch them
earlier by checking for when we copy a file with no content. This won't
solve the issue but if errors surface through this they should be more
clear.

- Add a `requireContent` argument to `copyOutput`. In the future we
  might consider making the default either with or without an output -
  we don't know of any use cases where we want to allow copying a zero
  byte file.
- Use the new argument from the kernel builder to check earlier whether
  a kernel file is empty.
  • Loading branch information
natebosch committed Jul 30, 2019
1 parent d14a8bb commit 69b28c5
Show file tree
Hide file tree
Showing 7 changed files with 32 additions and 6 deletions.
4 changes: 4 additions & 0 deletions build_modules/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
## 2.4.1

- Require non-empty output from kernel build steps.

## 2.4.0

- Allow overriding the target name passed to the kernel worker.
Expand Down
2 changes: 1 addition & 1 deletion build_modules/lib/src/kernel_builder.dart
Original file line number Diff line number Diff line change
Expand Up @@ -186,7 +186,7 @@ Future<void> _createKernel(
}

// Copy the output back using the buildStep.
await scratchSpace.copyOutput(outputId, buildStep);
await scratchSpace.copyOutput(outputId, buildStep, requireContent: true);
} finally {
await packagesFile.parent.delete(recursive: true);
}
Expand Down
8 changes: 6 additions & 2 deletions build_modules/pubspec.yaml
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
name: build_modules
version: 2.4.0
version: 2.4.1
description: Builders for Dart modules
author: Dart Team <misc@dartlang.org>
homepage: https://github.com/dart-lang/build/tree/master/build_modules
Expand All @@ -22,7 +22,7 @@ dependencies:
meta: ^1.1.0
path: ^1.4.2
pedantic: ^1.0.0
scratch_space: ^0.0.3
scratch_space: ^0.0.4

dev_dependencies:
build_runner: ^1.0.0
Expand All @@ -34,3 +34,7 @@ dev_dependencies:
path: test/fixtures/a
b:
path: test/fixtures/b

dependency_overrides:
scratch_space:
path: ../scratch_space
5 changes: 5 additions & 0 deletions scratch_space/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,8 @@
## 0.0.4

- Add `requireContent` argument to `copyOutput` to allow asserting that a file
produced in a scratch space is not empty.

## 0.0.3+2

- Declare support for `package:build` version `1.x.x`.
Expand Down
3 changes: 2 additions & 1 deletion scratch_space/lib/scratch_space.dart
Original file line number Diff line number Diff line change
Expand Up @@ -2,4 +2,5 @@
// 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.

export 'src/scratch_space.dart' show ScratchSpace, canonicalUriFor;
export 'src/scratch_space.dart'
show ScratchSpace, canonicalUriFor, EmptyOutputException;
14 changes: 13 additions & 1 deletion scratch_space/lib/src/scratch_space.dart
Original file line number Diff line number Diff line change
Expand Up @@ -51,9 +51,14 @@ class ScratchSpace {
/// This must be called for all outputs which you want to be included as a
/// part of the actual build (any other outputs will be deleted with the
/// tmp dir and won't be available to other [Builder]s).
Future copyOutput(AssetId id, AssetWriter writer) async {
///
/// If [requireContent] is true and the file is empty an
/// [EmptyOutputException] is thrown.
Future copyOutput(AssetId id, AssetWriter writer,
{bool requireContent = false}) async {
var file = fileFor(id);
var bytes = await _descriptorPool.withResource(file.readAsBytes);
if (requireContent && bytes.isEmpty) throw EmptyOutputException(id);
await writer.writeAsBytes(id, bytes);
}

Expand Down Expand Up @@ -137,3 +142,10 @@ String canonicalUriFor(AssetId id) {
/// The path relative to the root of the environment for a given [id].
String _relativePathFor(AssetId id) =>
canonicalUriFor(id).replaceFirst('package:', 'packages/');

/// An indication that an output file which was expect to be non-empty had no
/// content.
class EmptyOutputException implements Exception {
final AssetId id;
EmptyOutputException(this.id);
}
2 changes: 1 addition & 1 deletion scratch_space/pubspec.yaml
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
name: scratch_space
version: 0.0.3+2
version: 0.0.4
description: A tool to manage running external executables within package:build
author: Dart Team <misc@dartlang.org>
homepage: https://github.com/dart-lang/build/tree/master/scratch_space
Expand Down

0 comments on commit 69b28c5

Please sign in to comment.