Skip to content

Conversation

@jonahwilliams
Copy link
Contributor

Description

This is a reland of #37508. The depfile's were not correctly escaped for windows, which caused failures when rebuilding.

Additionally fixes a bug in the file cache cleanup, and ensures we use a package uri when building a snapshot in assemble.

@fluttergithubbot fluttergithubbot added c: contributor-productivity Team-specific productivity, code health, technical debt. tool Affects the "flutter" command-line tool. See also t: labels. labels Sep 14, 2019
@jonahwilliams jonahwilliams changed the title Reland: implement build bundle with assemble [WIP] Reland: implement build bundle with assemble Sep 14, 2019
@codecov
Copy link

codecov bot commented Sep 14, 2019

Codecov Report

Merging #40470 into master will decrease coverage by 0.46%.
The diff coverage is 33.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #40470      +/-   ##
==========================================
- Coverage   59.75%   59.29%   -0.47%     
==========================================
  Files         193      193              
  Lines       18705    18795      +90     
==========================================
- Hits        11177    11144      -33     
- Misses       7528     7651     +123
Flag Coverage Δ
#flutter_tool 59.29% <33.33%> (-0.47%) ⬇️
Impacted Files Coverage Δ
...er_tools/lib/src/build_system/file_hash_store.dart 91.8% <ø> (+1.48%) ⬆️
...kages/flutter_tools/lib/src/commands/assemble.dart 78.68% <ø> (-0.35%) ⬇️
...s/flutter_tools/lib/src/commands/build_bundle.dart 98.11% <ø> (ø) ⬆️
...ter_tools/lib/src/build_system/targets/assets.dart 64.86% <0%> (-22.41%) ⬇️
...utter_tools/lib/src/build_system/build_system.dart 86.87% <100%> (-0.28%) ⬇️
packages/flutter_tools/lib/src/artifacts.dart 69.2% <100%> (+0.1%) ⬆️
...utter_tools/lib/src/build_system/targets/dart.dart 47.05% <2.5%> (-29.51%) ⬇️
packages/flutter_tools/lib/src/bundle.dart 75.25% <74.19%> (-0.5%) ⬇️
...ges/flutter_tools/lib/src/commands/ide_config.dart 30.09% <0%> (-53.4%) ⬇️
...ckages/flutter_tools/lib/src/commands/upgrade.dart 4.34% <0%> (-39.14%) ⬇️
... and 14 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4c724b9...4d026bb. Read the comment docs.

@jonahwilliams jonahwilliams changed the title [WIP] Reland: implement build bundle with assemble Reland: implement build bundle with assemble Sep 18, 2019
Copy link
Member

@jmagman jmagman left a comment

Choose a reason for hiding this comment

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

Last 3 commits since the master merge LGTM

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 and nit.

final PoolResource resource = await pool.request();
try {
final File file = fs.file(fs.path.join(environment.outputDir.path, entry.key));
file.parent.createSync(recursive: true);
Copy link
Member

Choose a reason for hiding this comment

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

What happens when one of these file operations fails?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We should throw, will update

Copy link
Contributor Author

Choose a reason for hiding this comment

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

actually, we're already propagating the future, just ensuring that the resource is released first


void _writeFilesToBuffer(List<File> files, StringBuffer buffer) {
for (File outputFile in files) {
if (platform.isWindows) {
Copy link
Member

Choose a reason for hiding this comment

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

nit: 2 space indent

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@jonahwilliams jonahwilliams merged commit ea7876a into flutter:master Sep 19, 2019
@jonahwilliams
Copy link
Contributor Author

I can't see if the devicelab failure is real or not, so I'll rollback for now

jonahwilliams pushed a commit that referenced this pull request Sep 19, 2019
Inconnu08 pushed a commit to Inconnu08/flutter that referenced this pull request Sep 30, 2019
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 4, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

c: contributor-productivity Team-specific productivity, code health, technical debt. tool Affects the "flutter" command-line tool. See also t: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants