Skip to content

Reapply flutter_assets#13567

Merged
szakarias merged 2 commits intoflutter:masterfrom
szakarias:reapplyFlutterAssets
Dec 14, 2017
Merged

Reapply flutter_assets#13567
szakarias merged 2 commits intoflutter:masterfrom
szakarias:reapplyFlutterAssets

Conversation

@szakarias
Copy link
Contributor

@szakarias szakarias commented Dec 14, 2017

This reverts commit ee65db1 except the commit marking complex_layout_ios__start_up as flaky.

The problem with the Windows bots should be fixed with #13564.

@szakarias
Copy link
Contributor Author

szakarias commented Dec 14, 2017

cc @yjbanov .

jakobr-google
jakobr-google approved these changes Dec 14, 2017
View reviewed changes
Copy link
Contributor

@jakobr-google jakobr-google left a comment

Choose a reason for hiding this comment

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

RSLGTM

@yjbanov
Copy link
Contributor

yjbanov commented Dec 14, 2017

lgtm

@szakarias szakarias merged commit 5e18c07 into flutter:master Dec 14, 2017
@xster
Copy link
Member

xster commented Dec 15, 2017

Should this add some gitignores in the templates and the example projects?

@Hixie
Copy link
Contributor

Hixie commented Dec 15, 2017

This dramatically increased the size of the gallery. Is that expected? Are we including assets twice now or some such?

@Hixie Hixie mentioned this pull request Dec 15, 2017
Closed
@mehmetf
Copy link
Contributor

mehmetf commented Dec 19, 2017

Could we revert this and give @tvolkert a chance to review? It affects Google build system in a significant way. Rolls are blocked because of it.

@tvolkert
Copy link
Contributor

tvolkert commented Dec 19, 2017

@mehmetf not easily because we'd have to revert several engine commits as well.

s'ok - I'll roll this once I'm back - there shouldn't be that much activity over the holidays anyway.

@tvolkert
Copy link
Contributor

tvolkert commented Dec 19, 2017 •
edited
Loading

@szakarias was this tested with --project=assets? That flag is used for prebuilt applications (which are used inside Google).

flutter/packages/flutter_tools/lib/src/commands/run.dart

Line 115 in 12f4055

argParser.addOption('project-assets',

flutter/packages/flutter_tools/lib/src/resident_runner.dart

Line 406 in 12f4055

_assetBundle = new AssetBundle.fixed(_projectRootPath, projectAssets);

@szakarias
Copy link
Contributor Author

szakarias commented Dec 19, 2017

No, I will do that.

@alanrussian
Copy link
Contributor

alanrussian commented Dec 19, 2017

I need to merge #13521 into Google but this is currently blocking me, so I have to manually merge in #13521. Would be nice to have this fixed ASAP. Thanks!

@tvolkert
Copy link
Contributor

tvolkert commented Dec 20, 2017

FTR @szakarias the error we're seeing inside Google patching this in is:

I/flutter ( 3392): [INFO:engine.cc(586)] Could not configure asset bundle at path:

You can reproduce by patching in cl/179637015

DaveShuckerow pushed a commit to DaveShuckerow/flutter that referenced this pull request May 14, 2018
@szakarias
Reapply flutter_assets (flutter#13567)
cb30105 
* Revert "Revert engine (flutter#13547)"

This reverts commit ee65db1.

* Keep flaky mark
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 12, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@yjbanov yjbanov yjbanov approved these changes

@mravn-google mravn-google Awaiting requested review from mravn-google

+1 more reviewer

@jakobr-google jakobr-google jakobr-google approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

@szakarias @yjbanov @xster @Hixie @mehmetf @tvolkert @alanrussian @jakobr-google @googlebot