Skip to content

Add Filesystem underneath Reader, simplify tests further. #3860

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Feb 19, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion _test_common/lib/in_memory_reader_writer.dart
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ class InMemoryRunnerAssetReaderWriter extends InMemoryAssetReaderWriter
Stream<AssetId> get onCanRead => _onCanReadController.stream;
void Function(AssetId)? onDelete;

InMemoryRunnerAssetReaderWriter({super.sourceAssets, super.rootPackage});
InMemoryRunnerAssetReaderWriter({super.rootPackage});

@override
Future<bool> canRead(AssetId id) {
Expand Down
4 changes: 2 additions & 2 deletions _test_common/lib/test_phases.dart
Original file line number Diff line number Diff line change
Expand Up @@ -113,9 +113,9 @@ Future<TestBuildersResult> testBuilders(
inputs.forEach((serializedId, contents) {
var id = makeAssetId(serializedId);
if (contents is String) {
readerWriter.cacheStringAsset(id, contents);
readerWriter.filesystem.writeAsStringSync(id, contents);
} else if (contents is List<int>) {
readerWriter.cacheBytesAsset(id, contents);
readerWriter.filesystem.writeAsBytesSync(id, contents);
}
});

Expand Down
2 changes: 2 additions & 0 deletions build/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@
- Use `build_test` 3.0.0.
- Refactor `PathProvidingAssetReader` to `AssetPathProvider`.
- Refactor `MultiPackageAssetReader` to internal `AssetFinder`.
- Add internal `Filesystem` that backs `AssetReader` and `AssetWriter`
implementations.

## 2.4.2

Expand Down
4 changes: 4 additions & 0 deletions build/lib/src/builder/build_step_impl.dart
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import '../asset/writer.dart';
import '../resource/resource.dart';
import '../state/asset_finder.dart';
import '../state/asset_path_provider.dart';
import '../state/filesystem.dart';
import '../state/input_tracker.dart';
import '../state/reader_state.dart';
import 'build_step.dart';
Expand Down Expand Up @@ -82,6 +83,9 @@ class BuildStepImpl implements BuildStep, AssetReaderState {
_stageTracker = stageTracker ?? NoOpStageTracker.instance,
_reportUnusedAssets = reportUnusedAssets;

@override
Filesystem get filesystem => _reader.filesystem;

@override
AssetFinder get assetFinder => _reader.assetFinder;

Expand Down
1 change: 1 addition & 0 deletions build/lib/src/internal.dart
Original file line number Diff line number Diff line change
Expand Up @@ -8,5 +8,6 @@ library;

export 'state/asset_finder.dart';
export 'state/asset_path_provider.dart';
export 'state/filesystem.dart';
export 'state/input_tracker.dart';
export 'state/reader_state.dart';
177 changes: 177 additions & 0 deletions build/lib/src/state/filesystem.dart
Original file line number Diff line number Diff line change
@@ -0,0 +1,177 @@
// Copyright (c) 2025, 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:convert';
import 'dart:io';
import 'dart:typed_data';

import 'package:pool/pool.dart';

import '../asset/id.dart';
import 'asset_path_provider.dart';

/// The filesystem the build is running on.
///
/// Methods behave as the `dart:io` methods with the same names, with some
/// exceptions noted.
abstract interface class Filesystem {
Future<bool> exists(AssetId id);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A existsSync wouldn't be useful? Or sync versions of read? Are there any "rules" for when calling the sync version or not?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It certainly will :) in this PR I just added what's already used.

The generator-facing interfaces, AssetReader/AssetWriter, are only async. We can add sync methods but we can't move existing async use to sync.

But for build_runner internal use we can switch things to sync and it's probably better--benchmarking showed it gives about a 2x speedup. So one of the goals of this refactoring is to give build_runner internals the choice.


Future<String> readAsString(AssetId id, {Encoding encoding = utf8});
Future<Uint8List> readAsBytes(AssetId id);

/// Deletes a file.
///
/// If the file does not exist, does nothing.
Future<void> delete(AssetId id);

/// Deletes a file.
///
/// If the file does not exist, does nothing.
void deleteSync(AssetId id);

/// Writes a file.
///
/// Creates enclosing directories as needed if they don't exist.
void writeAsStringSync(AssetId id, String contents,
{Encoding encoding = utf8});

/// Writes a file.
///
/// Creates enclosing directories as needed if they don't exist.
Future<void> writeAsString(AssetId id, String contents,
{Encoding encoding = utf8});

/// Writes a file.
///
/// Creates enclosing directories as needed if they don't exist.
void writeAsBytesSync(AssetId id, List<int> contents);

/// Writes a file.
///
/// Creates enclosing directories as needed if they don't exist.
Future<void> writeAsBytes(AssetId id, List<int> contents);
}

/// A filesystem using [assetPathProvider] to map to the `dart:io` filesystem.
class IoFilesystem implements Filesystem {
final AssetPathProvider assetPathProvider;

/// Pool for async file operations.
final _pool = Pool(32);

IoFilesystem({required this.assetPathProvider});

@override
Future<bool> exists(AssetId id) => _pool.withResource(_fileFor(id).exists);

@override
Future<Uint8List> readAsBytes(AssetId id) =>
_pool.withResource(_fileFor(id).readAsBytes);

@override
Future<String> readAsString(AssetId id, {Encoding encoding = utf8}) =>
_pool.withResource(_fileFor(id).readAsString);

@override
void deleteSync(AssetId id) {
final file = _fileFor(id);
if (file.existsSync()) file.deleteSync();
}

@override
Future<void> delete(AssetId id) {
return _pool.withResource(() async {
final file = _fileFor(id);
if (await file.exists()) await file.delete();
});
}

@override
void writeAsBytesSync(AssetId id, List<int> contents,
{Encoding encoding = utf8}) {
final file = _fileFor(id);
file.parent.createSync(recursive: true);
file.writeAsBytesSync(contents);
}

@override
Future<void> writeAsBytes(AssetId id, List<int> contents) {
return _pool.withResource(() async {
final file = _fileFor(id);
await file.parent.create(recursive: true);
await file.writeAsBytes(contents);
});
}

@override
void writeAsStringSync(AssetId id, String contents,
{Encoding encoding = utf8}) {
final file = _fileFor(id);
file.parent.createSync(recursive: true);
file.writeAsStringSync(contents, encoding: encoding);
}

@override
Future<void> writeAsString(AssetId id, String contents,
{Encoding encoding = utf8}) {
return _pool.withResource(() async {
final file = _fileFor(id);
await file.parent.create(recursive: true);
await file.writeAsString(contents, encoding: encoding);
});
}

/// Returns a [File] for [id] for the current [assetPathProvider].
File _fileFor(AssetId id) {
return File(assetPathProvider.pathFor(id));
}
}

/// An in-memory [Filesystem].
class InMemoryFilesystem implements Filesystem {
final Map<AssetId, List<int>> assets = {};

@override
Future<bool> exists(AssetId id) async => assets.containsKey(id);

@override
Future<Uint8List> readAsBytes(AssetId id) async => assets[id] as Uint8List;

@override
Future<String> readAsString(AssetId id, {Encoding encoding = utf8}) async =>
encoding.decode(assets[id] as Uint8List);

@override
Future<void> delete(AssetId id) async {
assets.remove(id);
}

@override
void deleteSync(AssetId id) async {
assets.remove(id);
}

@override
void writeAsBytesSync(AssetId id, List<int> contents) {
assets[id] = contents;
}

@override
Future<void> writeAsBytes(AssetId id, List<int> contents) async {
assets[id] = contents;
}

@override
void writeAsStringSync(AssetId id, String contents,
{Encoding encoding = utf8}) {
assets[id] = encoding.encode(contents);
}

@override
Future<void> writeAsString(AssetId id, String contents,
{Encoding encoding = utf8}) async {
assets[id] = encoding.encode(contents);
}
}
12 changes: 12 additions & 0 deletions build/lib/src/state/reader_state.dart
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,16 @@
import '../asset/reader.dart';
import 'asset_finder.dart';
import 'asset_path_provider.dart';
import 'filesystem.dart';
import 'input_tracker.dart';

/// Provides access to the state backing an [AssetReader].
extension AssetReaderStateExtension on AssetReader {
Filesystem get filesystem {
_requireIsAssetReaderState();
return (this as AssetReaderState).filesystem;
}

AssetFinder get assetFinder {
_requireIsAssetReaderState();
return (this as AssetReaderState).assetFinder;
Expand Down Expand Up @@ -43,6 +49,12 @@ extension AssetReaderStateExtension on AssetReader {

/// The state backing an [AssetReader].
abstract interface class AssetReaderState {
/// The [Filesystem] that this reader reads from.
///
/// Warning: this access to the filesystem bypasses reader functionality
/// such as read tracking, caching and visibility restriction.
Filesystem get filesystem;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this need to be on the interface?
It - at least mostly - seems to be used on the InMemoryAssetReaderWriter.
And it generally seems like something that shouldn't be used (having a warning and all) - are there other use cases than testing? If not, should it perhaps also be called something else? (e.g. filesystemForTesting or something).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are correct that only test code uses this for now.

That's because non-test code was restricted to using the Reader interface, which hides the underlying files.

Each generator has a different view on the filesystem, with some set of files hidden because they are "generated after" the current generator runs. Currently the import graph computation deals with that by checking the whole thing for each generator.

Through this refactoring build_runner internals are allowed to bypass that abstraction and see the actual filesystem; in the next PRs they will also get access to information about what is different about the current generator's view.

Then they can be fast :) by building the base state once then applying the diff for the current generator on top.


/// The [AssetFinder] associated with this reader.
///
/// All readers have an [AssetFinder], but the functionality it provides,
Expand Down
1 change: 1 addition & 0 deletions build/pubspec.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ dependencies:
meta: ^1.3.0
package_config: ^2.1.0
path: ^1.8.0
pool: ^1.5.0

dev_dependencies:
build_resolvers: ^2.4.0
Expand Down
28 changes: 13 additions & 15 deletions build/test/generate/run_post_process_builder_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,7 @@ import '../common/builders.dart';

void main() {
group('runPostProcessBuilder', () {
late InMemoryAssetReader reader;
late InMemoryAssetWriter writer;
late InMemoryAssetReaderWriter readerWriter;
final copyBuilder = CopyingPostProcessBuilder();
final deleteBuilder = DeletePostProcessBuilder();
final aTxt = makeAssetId('a|lib/a.txt');
Expand All @@ -24,38 +23,37 @@ void main() {
void addAsset(AssetId id) => adds[id] = true;
void deleteAsset(AssetId id) => deletes[id] = true;

Map<AssetId, String> sourceAssets;

setUp(() async {
sourceAssets = {
aTxt: 'a',
};
reader = InMemoryAssetReader(sourceAssets: sourceAssets);
writer = InMemoryAssetWriter();
readerWriter = InMemoryAssetReaderWriter()
..filesystem.writeAsStringSync(aTxt, 'a');
adds.clear();
deletes.clear();
});

test('can delete assets', () async {
await runPostProcessBuilder(copyBuilder, aTxt, reader, writer, logger,
await runPostProcessBuilder(
copyBuilder, aTxt, readerWriter, readerWriter, logger,
addAsset: addAsset, deleteAsset: deleteAsset);
await runPostProcessBuilder(deleteBuilder, aTxt, reader, writer, logger,
await runPostProcessBuilder(
deleteBuilder, aTxt, readerWriter, readerWriter, logger,
addAsset: addAsset, deleteAsset: deleteAsset);
expect(deletes, contains(aTxt));
expect(deletes, isNot(contains(aTxtCopy)));
});

test('can create assets and read the primary asset', () async {
await runPostProcessBuilder(copyBuilder, aTxt, reader, writer, logger,
await runPostProcessBuilder(
copyBuilder, aTxt, readerWriter, readerWriter, logger,
addAsset: addAsset, deleteAsset: deleteAsset);
expect(writer.assets, contains(aTxtCopy));
expect(writer.assets[aTxtCopy], decodedMatches('a'));
expect(readerWriter.assets, contains(aTxtCopy));
expect(readerWriter.assets[aTxtCopy], decodedMatches('a'));
expect(adds, contains(aTxtCopy));
});

test('throws if addAsset throws', () async {
expect(
() => runPostProcessBuilder(copyBuilder, aTxt, reader, writer, logger,
() => runPostProcessBuilder(
copyBuilder, aTxt, readerWriter, readerWriter, logger,
addAsset: (id) => throw InvalidOutputException(id, ''),
deleteAsset: deleteAsset),
throwsA(const TypeMatcher<InvalidOutputException>()));
Expand Down
Loading
Loading