Skip to content

Build App.framework directly to build directory#69612

Merged
fluttergithubbot merged 2 commits intoflutter:masterfrom
jmagman:app-fw-build-loc
Nov 3, 2020
Merged

Build App.framework directly to build directory#69612
fluttergithubbot merged 2 commits intoflutter:masterfrom
jmagman:app-fw-build-loc

Conversation

@jmagman
Copy link
Member

@jmagman jmagman commented Nov 2, 2020

Description

Since #51453 we no longer need to build the App.framework to a known directory like project/ios/Flutter/App.framework. Instead, build the frameworks directly to the build products directory. For example, flutter build ios -v --simulator will build to /build/ios/Debug-iphonesimulator/App.framework.

Additionally, this sets us up to no longer need to lipo the simulator and non-simulator frameworks together since they will build to different directories.

flutter/packages/flutter_tools/lib/src/build_system/targets/ios.dart

Lines 103 to 112 in cb67513

final String resultPath = environment.fileSystem.path.join(environment.buildDir.path, 'App.framework', 'App');
environment.fileSystem.directory(resultPath).parent.createSync(recursive: true);
final ProcessResult result = await environment.processManager.run(<String>[
'lipo',
...darwinArchs.map((DarwinArch iosArch) =>
environment.fileSystem.path.join(buildOutputPath, getNameForDarwinArch(iosArch), 'App.framework', 'App')),
'-create',
'-output',
resultPath,
]);

When Apple Silicon ships and ARM simulators are supported, this lipo will fail since the simulator and real device frameworks will both contain arm64.

Also stop copying the .DS_Store directories, while we're at it (already done with Flutter.framework). These files shouldn't be copied, and it adds spew during the installation phase:

[        ] [ 31%] Copying
/build/ios/iphoneos/Runner.app/Frameworks/App.framework/flutter_assets/assets/navbar/.DS_Store to
device

Related Issues

#60118

Tests

No direct tests of the new location. Already many many integration tests that will blow up in the compiled dart code or flutter_assets are missing.

@jmagman jmagman added tool Affects the "flutter" command-line tool. See also t: labels. t: xcode "xcodebuild" on iOS and general Xcode project management labels Nov 2, 2020
@jmagman jmagman self-assigned this Nov 2, 2020
@google-cla google-cla bot added the cla: yes label Nov 2, 2020
jmagman
jmagman commented Nov 2, 2020
View reviewed changes
packages/flutter_tools/bin/xcode_backend.sh

# Prefer the hidden .ios folder, but fallback to a visible ios folder if .ios
# doesn't exist.
local flutter_ios_out_folder="${project_path}/.ios/Flutter"
Copy link
Member Author

@jmagman jmagman Nov 2, 2020

Choose a reason for hiding this comment

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

Removed flutter_ios_out_folder. flutter_ios_engine_folder initialization was moved down a few lines to where it's used.

jmagman
jmagman commented Nov 2, 2020
View reviewed changes
packages/flutter_tools/bin/xcode_backend.sh
AssertExists "${flutter_ios_engine_folder}"

# Copy Xcode behavior and don't copy over headers or modules.
RunCommand rsync -av --delete --filter "- .DS_Store/" --filter "- Headers/" --filter "- Modules/" "${flutter_ios_engine_folder}/Flutter.framework" "${xcode_frameworks_dir}/"
Copy link
Member Author

@jmagman jmagman Nov 2, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Here's where .DS_Store is already being filtered out of Flutter.framework rsync.

Copy link
Contributor

@jonahwilliams jonahwilliams Nov 2, 2020

Choose a reason for hiding this comment

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

We probably need to also filter this out of the asset detection logic too, separate issue though

jmagman
jmagman commented Nov 2, 2020
View reviewed changes
packages/flutter_tools/lib/src/project.dart
/// No longer compiled to this location.
///
/// Used only for "flutter clean" to remove old references.
Directory get deprecatedCompiledDartFramework => _flutterLibRoot
Copy link
Member Author

@jmagman jmagman Nov 2, 2020

Choose a reason for hiding this comment

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

Open to naming suggestions here.

Copy link
Contributor

@jonahwilliams jonahwilliams Nov 2, 2020

Choose a reason for hiding this comment

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

Seems reasonable to me. Might want to TODO removal after a stable release cycle or so

jonahwilliams
jonahwilliams approved these changes Nov 2, 2020
View reviewed changes
Copy link
Contributor

@jonahwilliams jonahwilliams left a comment

Choose a reason for hiding this comment

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

LGTM

jmagman
jmagman commented Nov 2, 2020
View reviewed changes
packages/flutter_tools/lib/src/ios/mac.dart
logger.printError('Your Xcode project requires migration. See https://flutter.dev/docs/development/ios-project-migration for details.');
logger.printError('');
logger.printError('You can temporarily work around this issue by running:');
logger.printError(' rm -rf ios/Flutter/App.framework');
Copy link
Member Author

@jmagman jmagman Nov 2, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Forgot about this one.

@fluttergithubbot fluttergithubbot merged commit b770883 into flutter:master Nov 3, 2020
jonahwilliams pushed a commit that referenced this pull request Nov 3, 2020
Revert "Build App.framework directly to build directory (#69612)"
fadbe4d 
This reverts commit b770883.
@jonahwilliams jonahwilliams mentioned this pull request Nov 3, 2020
Merged
jonahwilliams pushed a commit that referenced this pull request Nov 3, 2020
Revert "Build App.framework directly to build directory (#69612)" (#6…
a2a4e9e 
…9641)

This reverts commit b770883.
@jmagman jmagman deleted the app-fw-build-loc branch November 3, 2020 18:33
@jmagman jmagman mentioned this pull request Nov 3, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@christopherfujino christopherfujino Awaiting requested review from christopherfujino

1 more reviewer

@jonahwilliams jonahwilliams jonahwilliams approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@jmagman jmagman

Labels

t: xcode "xcodebuild" on iOS and general Xcode project management 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.

3 participants

@jmagman @jonahwilliams @fluttergithubbot