Skip to content

Adapt to vm isolate and isolate snapshot pieces being emitted as asse… #6433

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.

Sign up for GitHub

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

Jump to bottom
Merged
rmacnak-google merged 1 commit into from
Oct 20, 2016
Merged

Adapt to vm isolate and isolate snapshot pieces being emitted as asse… #6433

rmacnak-google merged 1 commit into from
Oct 20, 2016

Conversation

@rmacnak-google
Copy link
Contributor

@rmacnak-google rmacnak-google commented Oct 20, 2016

…mbly.

@eseidelGoogle eseidelGoogle mentioned this pull request Oct 20, 2016
Closed
chinmaygarde
chinmaygarde approved these changes Oct 20, 2016
View reviewed changes
Copy link
Member

@chinmaygarde chinmaygarde left a comment

Choose a reason for hiding this comment

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

lgtm

@rmacnak-google rmacnak-google merged commit a97cf4b into flutter:master Oct 20, 2016
tvolkert
tvolkert reviewed Oct 21, 2016
View reviewed changes
packages/flutter_tools/lib/src/commands/build_aot.dart
printStatus('Building app.dylib...');

// These names are known to from the engine.
const String kDartVmIsolateSnapshotBuffer = 'kDartVmIsolateSnapshotBuffer';
Copy link
Contributor

@tvolkert tvolkert Oct 21, 2016

Choose a reason for hiding this comment

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

It looks like with this change, we're just dropping the vm isolate snapshot (that we generate on line 216) on the floor for iOS. For Android, we include it in the APK (build_apk.dart line 578), but for iOS it goes nowhere. Isn't this a problem?

@tvolkert
Copy link
Contributor

tvolkert commented Oct 21, 2016

So with this change, we pass the following flags to flutter build aot --release --target-platform=ios

--vm_isolate_snapshot=build/aot/snapshot_aot_vmisolate
--isolate_snapshot=build/aot/snapshot_aot_isolate

Yet those snapshot files aren't generated. Is this intentional? It seems weird to pass flags that get dropped on the floor. Does this behavior get triggered because we also pass --assembly=build/aot/snapshot_assembly.S? Or because it's a different gen_snapshot binary that's been built for iOS? Either way, it seems like we should fail if the flags are passed and we don't support them (then update tools to not pass the flags when they're not supported)...

It's also weird that in debug mode, our app.dylib is being created without actually linking anything anymore...

@rmacnak-google let's chat tomorrow - I guess I need more context on what changed and how stuff works now.

@tvolkert
Copy link
Contributor

tvolkert commented Oct 21, 2016

Funny enough, if you try calling gen_snapshot without the --vm_isolate_snapshot flag, it fails with:

No vm isolate snapshot output file specified.

... even though it's ignored when you do pass it.

@rmacnak-google
Copy link
Contributor Author

rmacnak-google commented Oct 21, 2016

They are generated; they're included in the .S now instead of being emitted as separate files.

@tvolkert
Copy link
Contributor

tvolkert commented Oct 21, 2016

Ok, verified that this change causes iOS debug builds to crash, because they're lacking the snapshots required for DBC.

This was referenced Oct 21, 2016
Closed
Merged
@rmacnak-google rmacnak-google deleted the dylib branch October 21, 2016 21:15
tvolkert added a commit that referenced this pull request Oct 21, 2016
@tvolkert
Rollback commits to get iOS into a stable state (#6464)
379b103 
* Rollback commits to get iOS into a stable state

This rolls back the following commits:
* 23c52fc (#6434)
* a97cf4b (#6433)
* e72e174 (#6428)

It also updates the engine to a newer revision that has the
necessary rollbacks in the engine repo.

Fixes #6458
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 15, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@chinmaygarde chinmaygarde chinmaygarde approved these changes

+1 more reviewer

@tvolkert tvolkert tvolkert left review comments

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.

3 participants

@rmacnak-google @tvolkert @chinmaygarde