Skip to content

Remove Source.behavior, fix bug in depfile invalidation #43945

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 6 commits into from
Nov 5, 2019

Conversation

jonahwilliams
Copy link
Contributor

@jonahwilliams jonahwilliams commented Nov 1, 2019

Description

Remove Source.behavior and all custom asset behavior. This is now replaced with a single helper method that returns a depfile.

Fixes a bug in depfile invalidation. IF a depfile already existed, we would not recompute the outputs after running it. Thus, changes in the outputs expressed in the depfile would not be picked up until a subsequent run. This delayed the removal of stale files.

See the test case that demonstrates the issue

The test skip is due to a bug in package:file https://github.com/google/file.dart/issues/131

@fluttergithubbot fluttergithubbot added the tool Affects the "flutter" command-line tool. See also t: labels. label Nov 1, 2019
@jonahwilliams
Copy link
Contributor Author

I have a fix pending that allows me to remove the skip in dart-archive/file.dart#132

@@ -526,17 +526,17 @@ class _BuildInstance {
await node.target.build(environment);
printTrace('${node.target.name}: Complete');

// If we were missing the depfile, resolve files after executing the
node.inputs.clear();
Copy link
Member

Choose a reason for hiding this comment

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

Maybe:

node.inputs
  ..clear()
  ..addAll(...);
node.outputs
  ..clear()
  ..addAll(...);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@@ -58,6 +58,9 @@ class Depfile {
///
/// If either [inputs] or [outputs] is empty, does not write to the file.
void writeToFile(File depfile) {
if (depfile.existsSync()) {
Copy link
Member

Choose a reason for hiding this comment

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

The writeAsStringSync() below will truncate the file, so I guess this delete is needed for the case where inputs or outputs is empty. Maybe add a comment to that effect, or move the delete under the if for the inputs/outputs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved under the if and added more to the method's doc comment

/// A helper function to copy an asset bundle into an [environment]'s output
/// directory.
///
/// [pathSuffix] may be optionally provided to add additional paths to the
Copy link
Member

Choose a reason for hiding this comment

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

It looks like this parameter no longer eixsts.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed

Copy link
Member

@zanderso zanderso left a comment

Choose a reason for hiding this comment

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

lgtm w/ question

@@ -65,7 +67,7 @@ flutter:

// See https://github.com/flutter/flutter/issues/35293
expect(fs.file(fs.path.join(environment.buildDir.path, 'flutter_assets', 'assets/foo/bar.png')).existsSync(), false);
}));
}), skip: Platform.isWindows); // See https://github.com/google/file.dart/issues/131
Copy link
Member

Choose a reason for hiding this comment

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

Does this imply that stale files will be left in the build directory on Windows? Could this result in an incorrect build?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I could not reproduce this on a real filesystem, which leads me to believe it is only an issue with the memory filesystem.

@jonahwilliams jonahwilliams merged commit 0f6c093 into flutter:master Nov 5, 2019
@jonahwilliams jonahwilliams deleted the remove_behavior branch November 5, 2019 00:37
Inconnu08 pushed a commit to Inconnu08/flutter that referenced this pull request Nov 26, 2019
* remove Source.behavior, fix bug in depfile invalidation

* more cleanup of assets

* Add skip

* address comments

* Update build_system.dart
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 3, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
tool Affects the "flutter" command-line tool. See also t: labels.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants