Skip to content

Move embedding and linking macOS Flutter frameworks into the tool #71764

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
jmagman merged 2 commits into from
Dec 8, 2020
Merged

Move embedding and linking macOS Flutter frameworks into the tool #71764

jmagman merged 2 commits into from
Dec 8, 2020

Conversation

@jmagman
Copy link
Member

@jmagman jmagman commented Dec 5, 2020 •
edited
Loading

Description

macOS version of #51453. See details and discussion there.

Push linker and embedding logic into the tool so future changes (like distributing as an XCFramework, renaming FlutterMacOS.framework, etc) do not require changes in the user's project.

At some point FlutterMacOS.framework will ship with x86 and ARM slices. With this change, we will be able to introduce thinning (as iOS does) in the script without requiring a user project migration.

This change would also set us up to stop copying FlutterMacOS.framework to Flutter/ephemeral. On iOS, for example, the framework is copied directly from the artifacts directory to the BUILT_PRODUCTS_DIR so there's never a mismatch between a Release build and a Debug version of Flutter.

Changes

Future Documentation

Related Issues

Fixes #56581
Part of #70413
Will make #60113 easier.

Tests

macos_project_migration_test

@jmagman jmagman self-assigned this Dec 5, 2020
@flutter-dashboard flutter-dashboard bot added c: contributor-productivity Team-specific productivity, code health, technical debt. tool Affects the "flutter" command-line tool. See also t: labels. labels Dec 5, 2020
@google-cla google-cla bot added the cla: yes label Dec 5, 2020
jmagman
jmagman commented Dec 5, 2020
View reviewed changes
packages/flutter_tools/bin/macos_assemble.sh Outdated
Copy link
Member Author

@jmagman jmagman Dec 5, 2020

Choose a reason for hiding this comment

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

This logic is unchanged other than putting it in a function (so whitespace indentation) and adding locals.

packages/flutter_tools/bin/macos_assemble.sh Outdated
Copy link
Member Author

@jmagman jmagman Dec 5, 2020

Choose a reason for hiding this comment

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

This is new.

packages/flutter_tools/bin/macos_assemble.sh Outdated
Copy link
Member Author

@jmagman jmagman Dec 5, 2020

Choose a reason for hiding this comment

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

Also new.

...tter_tools/lib/src/macos/migrations/remove_macos_framework_link_and_embedding_migration.dart Outdated
Copy link
Member Author

@jmagman jmagman Dec 5, 2020

Choose a reason for hiding this comment

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

This file is almost the same as https://github.com/flutter/flutter/blob/e1598c96c072a6323e8dbf2a1724a896533a6c27/packages/flutter_tools/lib/src/ios/migrations/remove_framework_link_and_embedding_migration.dart with different identifiers, doesn't do anything with add-to-app, and leaves off any Xcode version checks.

...tter_tools/lib/src/macos/migrations/remove_macos_framework_link_and_embedding_migration.dart Outdated
Copy link
Member Author

@jmagman jmagman Dec 5, 2020

Choose a reason for hiding this comment

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

I'll write a doc like https://flutter.dev/docs/development/ios-project-migration and link it here in a future PR.

stuartmorgan-g
stuartmorgan-g approved these changes Dec 8, 2020
View reviewed changes
Copy link
Contributor

@stuartmorgan-g stuartmorgan-g left a comment

Choose a reason for hiding this comment

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

LGTM with nit. The Dart rework part is optional; if you go that route I can re-review that piece.

Thanks for tackling this; it's great to reduce delta relative to iOS, and this sets us up for some great improvements!

packages/flutter_tools/bin/macos_assemble.sh Outdated
Copy link
Contributor

@stuartmorgan-g stuartmorgan-g Dec 8, 2020

Choose a reason for hiding this comment

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

This is non-trivial new bash code; any chance we could use this as the impetus to finally switch macOS over to tool_backend.* so that this could all be Dart code instead?

Copy link
Member Author

@jmagman jmagman Dec 8, 2020

Choose a reason for hiding this comment

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

I'm converging macOS and iOS scripts, and I'd like to do https://github.com/flutter/flutter/pull/70224/files#diff-cf570f69a008f028e95cfad7690ee32b3acbbc88379d340ea08ef67c0a3aff69R297 for macOS next (copy the FlutterMacOS framework directly to BUILT_PRODUCTS_DIR instead of ephemeral).

I'd rather get them to be the same bash script first since I know the iOS variant works, then swap them both over to the same dart code. That way each PR is a good improvement that isn't doing too much and is easy to review, and if it causes issues won't be a big painful revert.

Copy link
Member Author

@jmagman jmagman Dec 8, 2020

Choose a reason for hiding this comment

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

Actually, a macOS version of #59044 needs to happen first because the BUILT_PRODUCTS_DIR update requires a Podfile change, but you get the idea.

@jmagman
Copy link
Member Author

jmagman commented Dec 8, 2020

Google testing failing because #71757 hasn't rolled yet.

@jmagman jmagman merged commit 70f8fde into flutter:master Dec 8, 2020
@jmagman jmagman deleted the macos-link-tool branch December 8, 2020 19:59
jmagman added a commit that referenced this pull request Dec 8, 2020
@jmagman
Revert "Move embedding and linking macOS Flutter frameworks into the …
0b42c3c 
…tool (#71764)"

This reverts commit 70f8fde.
@jmagman jmagman mentioned this pull request Dec 8, 2020
Merged
jmagman added a commit that referenced this pull request Dec 8, 2020
@jmagman
Revert "Move embedding and linking macOS Flutter frameworks into the …
c88ab79 
…tool (#71764)" (#71951)

This reverts commit 70f8fde.
jmagman added a commit to jmagman/flutter that referenced this pull request Dec 9, 2020
@jmagman
Move embedding and linking macOS Flutter frameworks into the tool (fl…
d4eb08e 
…utter#71764)
This was referenced Dec 9, 2020
Merged
Merged
Merged
This was referenced Dec 21, 2020
Merged
Closed
Merged
@jmagman jmagman mentioned this pull request Nov 1, 2022
Open
@jmagman jmagman mentioned this pull request Sep 6, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@stuartmorgan-g stuartmorgan-g stuartmorgan-g approved these changes

Assignees

@jmagman jmagman

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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

macOS xcode project should move Framework linking to tool

2 participants

@jmagman @stuartmorgan-g