Add support for DarwinArchs when assembling macOS App.framework #81243

knopp · 2021-04-26T22:55:41Z

#81205

This can be used to specify target architectures and produce fat binaries.

Pre-launch Checklist

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
I signed the CLA.
I listed at least one issue that this PR fixes in the description above.
I updated/added relevant documentation (doc comments with ///).
I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt.
All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

flutter#81205 This can be used to specify target architectures and produce fat binaries.

even though TargetPlatform is called darwin

jonahwilliams

We should have some example unit tests for the iOS lipo you could copy from for the macOS example

packages/flutter_tools/lib/src/artifacts.dart

packages/flutter_tools/lib/src/build_system/targets/macos.dart

knopp · 2021-04-27T16:35:50Z

I'll look at the tests for lipo. Also debug build only produces x86_64 framework, I'll fix that too.

packages/flutter_tools/lib/src/artifacts.dart

jmagman · 2021-04-27T17:50:53Z

packages/flutter_tools/lib/src/base/build.dart

@@ -311,7 +311,7 @@ class AOTSnapshotter {
      TargetPlatform.android_arm64,
      TargetPlatform.android_x64,
      TargetPlatform.ios,
-      TargetPlatform.darwin_x64,
+      TargetPlatform.darwin,


I agree with you in #81205 (comment)

It does feel a bit off that there is TargetPlatform.ios and --define=IosArchs=... and then TargetPlatform.darwin. But iOS is darwin. It really should be TargetPlatform.macOS and --define=MacOSArchs=....

Just glancing at this I wouldn't be sure which to use (the confusion predates this PR). This also gets hairier when we start targeting iOS apps as native ARM mac apps for the unfortunately named "Mac Designed for iPad" #66184.

I'd be supportive if you wanted to rename this macos instead of darwin.

I'd be supportive if you wanted to rename this macos instead of darwin.

I'll keep that in mind, but I'm not sure I'd want to do it in this PR.

packages/flutter_tools/lib/src/build_system/targets/macos.dart

knopp · 2021-04-27T20:17:58Z

I added lipo tests for both framework thinning and universal binary.

jonahwilliams · 2021-04-27T21:21:53Z

packages/flutter_tools/lib/src/artifacts.dart

@@ -103,6 +103,14 @@ enum HostArtifact {
  pubExecutable,
 }

+// TODO(knopp): Remove once darwin artifacts are moved out of darwin-x64


I don't think we're going to move darwin artifacts out of darwin-x64. We'll still have different engine builds for darwin-x64 and darwin-arm - the target platform is best though of as a convenience for how the engine builds think of targets that was incorrectly extended to how our users interact with the platforms.

Why do you need different engine builds? Can you just have universal FlutterMacOS.framework (just like you have universal Flutter.framework for ios)? The PR already implements framework thinning.

I assumed that all the artifacts would be distributed as universal in the end.

No better answer than "that is how the engine builds are configured currently".

Well, sure, but I assumed that at some point there's going to be work done to build the engine for darwin-x64, darwin-arm64 and lipo them together. Isn't that the whole purpose of not having separate TargetPlatform.darwin_x64 and .darwin_arm64?

Somehow this is done for iOS, why can't it be done for macOS? (plus flutter_tester, font-subset and gen_snapshot).

CachedArtifacts doesn't know about DarwinArchs so I don't quite see how it would choose the right folder if you want to distribute darwin-arm64 build separately.

You could certainly do that work, but there is no point in changing the cache structure in the tool before that is done

I didn't assume this would happen before having universal artifacts. There's no point in doing that. I'll clarify that in the TODO comment.

Quasi related for distribution but I suggested combining the macOS framework into the iOS Flutter.xcframework #70413 though considering how unfortunately different the two platform projects look, and how manual it was to get the right framework slice out of the iOS xcframework, I'm not now sure if that will be very valuable. In any case, we should distribute a universal macOS framework. But no one has worked on that yet.

Well, I have no clue how flutter builds work on server side so I'm not sure I can help with that :) However with recent changes landed in buildroot and engine it is now possible to cross compile flutter for darwin-arm64. But it requires manually editing args.gn. I think the next step would be to add darwin cross compilation to flutter/tools/gn. I'll open an issue for it.

jonahwilliams

LGTM if LG to @jmagman

jmagman

LGTM, thanks @knopp!

zanderso · 2022-01-21T04:40:49Z

@knopp @jonahwilliams @jmagman I'm having a bit of trouble following the discussion and determining what the missing pieces are. This PR seems to be making some assumptions about how the arm64 artifacts will be supplied and consumed. Can someone ELI5 what those are for me? (e.g. libflutter.so)

jonahwilliams · 2022-01-21T05:15:48Z

So IIRC the idea was that we'd change how we were handling architecture - instead of having platform + arch in one enum this changes macos to be more like ios where the platform and arch are separate flags. I don't remember if any additional work was done to support arm64 builds.

jmagman · 2022-01-21T19:12:03Z

Exactly, instead of macOS platform being specified darwin_x64 instead it's darwin + either x86_64 or arm64. It was mostly refactoring for the sake of the enums.

@zanderso I can do the outstanding in the tool to stop using Rosetta once there's an arm FlutterMacOS.framework, if that's what you're exploring. I already had to do it for iOS, and it'll be easier with macOS since it's the same artifact paths, but with an extra architecture lipo'd in.

knopp · 2022-01-21T22:16:13Z

wing the discussion and determining what the missing pieces are. This PR seems to be making some assumptions about how the arm64 artifacts will be supplied and consumed. Can someone ELI5 what those are for me? (e.g. libflutter.so)

This is similar to how iOS work. Flutter tool can produce fat binaries for App.framework and strip unneeded architectures from FlutterMacOS.framework. IIRC what's left to do is mostly

Produce fat binary for FlutterMacOS.framework. We can already cross compile the framework for ARM64, just need script to lipo the framework and other desktop artifacts and add that to Luci.
Modify the paths in tool to download and use universal framework
Modify the tool to stop using rosetta (don't launch build through arch)

knopp · 2022-01-21T22:20:18Z

Exactly, instead of macOS platform being specified darwin_x64 instead it's darwin + either x86_64 or arm64. It was mostly refactoring for the sake of the enums.

Actually, it can be both x86_64 and arm64. Tool should build snapshot for all architectures and then lipo them together.

zanderso · 2022-01-22T00:17:42Z

How about gen_snapshot? I think the gen_snapshot_arm64 from the ios artifacts will work. Can the desktop build also use that?

jmagman · 2022-01-22T01:25:03Z

Exactly, instead of macOS platform being specified darwin_x64 instead it's darwin + either x86_64 or arm64. It was mostly refactoring for the sake of the enums.

Actually, it can be both x86_64 and arm64. Tool should build snapshot for all architectures and then lipo them together.

Yes, I meant the targeted platform can be described as darwin+x86_64 or darwin+arm64.

jmagman · 2022-01-22T01:27:19Z

How about gen_snapshot? I think the gen_snapshot_arm64 from the ios artifacts will work. Can the desktop build also use that?

If the same gen_snapshot_arm64 works (not sure if it does) then we'd also need to either ship it in the macos artifacts, or update the tool to always require the iOS artifacts to be downloaded. Either way, need tooling work to detect the current arch and choose the appropriate gen_snapshot if they can't be lipo'd together.

knopp · 2022-01-22T05:54:11Z

How about gen_snapshot? I think the gen_snapshot_arm64 from the ios artifacts will work. Can the desktop build also use that?

If the same gen_snapshot_arm64 works (not sure if it does) then we'd also need to either ship it in the macos artifacts, or update the tool to always require the iOS artifacts to be downloaded. Either way, need tooling work to detect the current arch and choose the appropriate gen_snapshot if they can't be lipo'd together.

Why couldn't gen_snapshot be lipod together?

flutter-dashboard bot added the tool Affects the "flutter" command-line tool. See also t: labels. label Apr 26, 2021

google-cla bot added the cla: yes label Apr 26, 2021

knopp force-pushed the darwin_architectures branch 4 times, most recently from 26454e6 to eebf222 Compare April 27, 2021 13:22

knopp changed the title ~~WIP: Add support for DarwinArchs when assembling macOS App.framework~~ Add support for DarwinArchs when assembling macOS App.framework Apr 27, 2021

knopp requested a review from jonahwilliams April 27, 2021 14:13

knopp mentioned this pull request Apr 27, 2021

Add support for flutter tool to assemble darwin-arm64 resources #81205

Closed

knopp added 4 commits April 27, 2021 16:41

Add support for DarwinArchs when assembling macOS App.framework

bee330b

flutter#81205 This can be used to specify target architectures and produce fat binaries.

Include clang_arm64 when looking for gen_snapshot

9d5f008

For now darwin artifacts are located in darwin-x64

5afb172

even though TargetPlatform is called darwin

Remove obsolete comment

03b23e2

knopp force-pushed the darwin_architectures branch from 2fabd56 to 03b23e2 Compare April 27, 2021 14:41

knopp requested a review from jmagman April 27, 2021 15:38

jonahwilliams reviewed Apr 27, 2021

View reviewed changes

packages/flutter_tools/lib/src/artifacts.dart Outdated Show resolved Hide resolved

packages/flutter_tools/lib/src/build_system/targets/macos.dart Outdated Show resolved Hide resolved

knopp added 2 commits April 27, 2021 18:14

Remove duplicate argument

b192580

Remove else

4e953d6

Respect DarwinArchs in debug build

f8e5571

jmagman reviewed Apr 27, 2021

View reviewed changes

knopp added 4 commits April 27, 2021 20:09

Rename _platformDirName to _enginePlatformDirectoryName

b881b46

Add tests for DebugMacOSFramework

d4c0f4e

Add framework thinning + tests

e03059e

Add test for CompileMacOSFramework (lipo, universal)

322cee0

jonahwilliams reviewed Apr 27, 2021

View reviewed changes

Clarify the TODO comment

537dcaa

jonahwilliams approved these changes Apr 28, 2021

View reviewed changes

jmagman approved these changes Apr 28, 2021

View reviewed changes

jmagman merged commit f6726b4 into flutter:master Apr 28, 2021

knopp deleted the darwin_architectures branch January 22, 2022 08:56

Add support for DarwinArchs when assembling macOS App.framework #81243

Add support for DarwinArchs when assembling macOS App.framework #81243

Uh oh!

Conversation

knopp commented Apr 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

jonahwilliams left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

knopp commented Apr 27, 2021

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

knopp commented Apr 27, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

knopp Apr 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonahwilliams left a comment

Choose a reason for hiding this comment

Uh oh!

jmagman left a comment

Choose a reason for hiding this comment

Uh oh!

zanderso commented Jan 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonahwilliams commented Jan 21, 2022

Uh oh!

jmagman commented Jan 21, 2022

Uh oh!

knopp commented Jan 21, 2022

Uh oh!

knopp commented Jan 21, 2022

Uh oh!

zanderso commented Jan 22, 2022

Uh oh!

jmagman commented Jan 22, 2022

Uh oh!

jmagman commented Jan 22, 2022

Uh oh!

knopp commented Jan 22, 2022

Uh oh!

Uh oh!

knopp commented Apr 26, 2021 •

edited

Loading

knopp Apr 27, 2021 •

edited

Loading

zanderso commented Jan 21, 2022 •

edited

Loading