8000 Support iOS ARM64 simulator builds by gw280 · Pull Request #422 · flutter/buildroot · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content
This repository was archived by the owner on Apr 21, 2025. It is now read-only.

Support iOS ARM64 simulator builds #422

Merged
merged 4 commits into from
Jan 28, 2021
Merged

Conversation

gw280
Copy link
Contributor
@gw280 gw280 commented Dec 30, 2020

I'm not convinced this is the right way to go just yet, but basically in order to support ARM64 simulator builds, we need to have a newer version of LLVM.

We already have a mechanism in place for us to use LLVM from Xcode (e.g. https://github.com/flutter/buildroot/blob/master/build/toolchain/mac/BUILD.gn#L238), so I'm proposing that we simply have a new toolchain definition for ARM64 simulator builds only that uses the system Xcode.

Unfortunately, to get Flutter and third party deps to even build in clang 12 shipped with Xcode 12.2, I had to add a couple of warning suppression options.

Copy link
Member
@jmagman jmagman left a comment

Choose a reason for hiding this comment

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

This LGTM but I'd like @chinmaygarde to also approve.

mac_toolchain("ios_clang_arm_sim") {
toolchain_cpu = "arm"
toolchain_os = "mac"
# TODO(gw280): We have to use llvm from xcode until we upgrade our copy of llvm
Copy link
Member

Choose a reason for hiding this comment

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

Does this mean Xcode 12 is required to be selected in the LUCI recipe? I think it's on 11.6.
https://github.com/flutter/infra/blob/a39a176cf6248d008aa3534ff7c233b4beac2d0a/config/engine_config.star#L20

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I suspect so. I'm not sure what exact version of Xcode we need to be able to build on M1 but 12 seems like a reasonable assumption.

I'd say we should target getting the machinery in place to build locally first, then we can deal with getting infra the right versions later.

@zanderso
Copy link
Member

@dnfield I see that you did the last toolchain roll 9 months ago here? flutter/engine#17483. Are we due for another roll?

@dnfield
Copy link
Contributor
dnfield commented Jan 11, 2021

I believe @chaselatta was recently working on rolling clang to 12, but it got reverted. I'm not sure why.

If we need to move Xcode forward, we need to make sure that the devicelab gets updated as well. Xcode 12 has been available for at least 6 months at this point, so it's probably fine to require it for customers.

@jmagman
Copy link
Member
jmagman commented Jan 11, 2021

If we need to move Xcode forward, we need to make sure that the devicelab gets updated as well. Xcode 12 has been available for at least 6 months at this point, so it's probably fine to require it for customers.

Is devicelab or user Xcode builds relevant to this change? If I'm reading it correctly it only uses the Xcode (12) version of llvm for the ARM simulator step on the engine builder, which doesn't matter to users for bitcode purposes.

@zanderso
Copy link
Member

@chaselatta is/was updating the toolchain at src/fuchsia/toolchain/{host_os} only: https://github.com/flutter/engine/pull/23067/files

@dnfield 9 months ago you updated the toolchain at src/buildtools/{host_os}-x64/clang https://github.com/flutter/engine/blob/master/DEPS#L499

What's the difference? Do we need to update both?

@gw280
Copy link
Contributor Author
gw280 commented Jan 11, 2021

It's also worth noting that Fuchsia doesn't publish mac-arm64 binaries, so we'd be using Rosetta.

@dnfield
Copy link
Contributor
dnfield commented Jan 11, 2021

Ahhh, I'm not quite sure what the fuchsia toolchain host files are then :\

The //buildtools files are the actual Clang compiler we use. I rolled it to 11. The latest goma tagged binary is available for 12.0.0. We could try rolling it, will have to see what build scripts will need to be updated for it.

@dnfield
Copy link
Contributor
dnfield commented Jan 11, 2021

(the goal has been to get to the point where we could have an autoroller for it, but I don't think anyone has seriously looked at setting that up)

@chaselatta
Copy link
Contributor

We have our own toolchain for fuchsia which I was trying to bump to get a newer version of libc++. I had to revert it because there were some subtle bugs that popped up in our runner around isolate shutdown. I was going to land a fix either today or tomorrow which would allow me to reland our roll. I don't think anything in this PR will affect anything related to fuchsia.

8000

@jmagman
Copy link
Member
jmagman commented Jan 12, 2021

If we need to move Xcode forward, we need to make sure that the devicelab gets updated as well.

flutter/flutter#73623

@dnfield
Copy link
Contributor
dnfield commented Jan 12, 2021

Filed flutter/flutter#73757 for the Clang 12.0.0 roll - I'm looking at what this'll take right now.

@dnfield
Copy link
Contributor
dnfield commented Jan 13, 2021

I believe I have a few patches that let us build with Clang 12. There are some dependencies to roll in dart/buildroot/engine but should be available within the next few days at worst.

@dnfield
Copy link
Contributor
dnfield commented Jan 14, 2021

@gw280 - can you check whether the version of LLVM in flutter/engine#23698 is sufficient for your needs here?

@gw280
Copy link
Contributor Author
gw280 commented Jan 22, 2021

@gw280 - can you check whether the version of LLVM in flutter/engine#23698 is sufficient for your needs here?

Doesn't look like it :( I ran a test build with the clang12 toolchain from Fuchsia and it generates ios-arm64 not simulator-arm64. It looks like the Xcode that Apple provides has some special sauce to make this work.

@gw280 gw280 force-pushed the gwright-ios-toolchain branch from c26f967 to 8f51a47 Compare January 22, 2021 23:43
@gw280 gw280 requested a review from dnfield January 22, 2021 23:48
@gw280
Copy link
Contributor Author
gw280 commented Jan 22, 2021

So, we don't need to actually set any special cflags if we have use_xcode set, so we will have to make sure that it's set when trying to build for ARM64 simulator. This set of patches seems to work locally.

@gw280
Copy link
Contributor Author
gw280 commented Jan 22, 2021

I get one (non-fatal) warning in two different files building harfbuzz with clang from xcode:

../../third_party/harfbuzz/src/hb-ot-cff-common.hh:186:30: warning: loop variable '_' is always a copy because the range of type 'hb_map_iter_t<hb_array_t<const hb_vector_t<unsigned char> >, (lambda at ../../third_party/harfbuzz/src/hb-ot-cff-common.hh:201:15), hb_function_sortedness_t::NOT_SORTED, nullptr>' does not return a reference [-Wrange-loop-analysis]

@dnfield
Copy link
Contributor
dnfield commented Jan 22, 2021

@gw280 - we should either patch harfbuzz and roll it (or just roll it if they've already fixed this), or we should remove those errors if use_xcode is set to true with a comment explaining that it applies to harfbuzz.

@@ -26,5 +28,7 @@ declare_args() {
# TODO(dnfield): We shouldn't need Xcode for bitcode_marker builds, but we do
# until libpng and dart have explicitly added LLVM asm sections to their
# assembly sources. https://github.com/flutter/flutter/issues/39281
use_xcode = enable_bitcode
# TODO(gw280): Once our own copy of clang 12 is capable of building for arm64 simulator,
Copy link
Contributor

Choose a reason for hiding this comment

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

Once our own copy of clang 12 is capable

I like the optimism

Copy link
Contributor
@dnfield dnfield left a comment

Choose a reason for hiding this comment

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

LGTM once the warnings are opted-out for the harbuzz compilation issues (or we roll to a harfbuzz that doesn't have those problems)

@chinmaygarde
Copy link
Member

Are we making progress on this?

@gw280
Copy link
Contributor Author
gw280 commented Jan 28, 2021

Been discussing the options with Dan earlier today. So the issue with Harfbuzz is that they explicitly control which warnings are enabled/disabled in hb.hh which overrides any CFLAGS we set, so we can't suppress it in our buildsystem.

I think we can land this change as is (the warning is considered non-fatal) and upstream a change to either suppress this warning or fix the underlying issue.

@gw280 gw280 requested a review from dnfield January 28, 2021 22:58
@gw280
Copy link
Contributor Author
gw280 commented Jan 28, 2021

Upstream issue filed: harfbuzz/harfbuzz#2834

Copy link
Contributor
@dnfield dnfield left a comment

Choose a reason for hiding this comment

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

LGTM.

It might be worth adding a TODO referencing the harfbuzz bug incase someone goes digging.

@gw280 gw280 merged commit 72cd672 into flutter:master Jan 28, 2021
@zanderso
Copy link
Member

/cc @xster

@xster
Copy link
Member
xster commented Jan 29, 2021

Woohoo! (no bugs attached, is there a sibling work for arm64 macos builds as well? 😀)

@gw280
Copy link
Contributor Author
gw280 commented Jan 29, 2021

@xster it's on my immediate radar but I can take a look. shouldn't be too hard. I don't know if we have any issues filed for it yet.

@jmagman
Copy link
Member
jmagman commented Jan 29, 2021

@xster it's on my immediate radar but I can take a look. shouldn't be too hard. I don't know if we have any issues filed for it yet.

Not a lot of details in it, but we have flutter/flutter#69221

@xster
Copy link
Member
xster commented Jan 29, 2021

\o/

@gw280
Copy link
Contributor Author
gw280 commented Jan 29, 2021

sorry that should say it's NOT on my immediate radar.. :)

@xster
Copy link
Member
xster commented Jan 29, 2021

too late, I already bought all the fireworks and booked our hawaii celebration trips

iskakaushik pushed a commit to iskakaushik/buildroot that referenced this pull request Feb 3, 2021
iskakaushik added a commit that referenced this pull request Feb 3, 2021
This reverts commit 72cd672.

Co-authored-by: Kaushik Iska <kaushikiska@google.com>
gw280 pushed a commit to gw280/buildroot that referenced this pull request Feb 5, 2021
gw280 pushed a commit to gw280/buildroot that referenced this pull request Feb 11, 2021
chinmaygarde pushed a commit to chinmaygarde/flutter_buildroot that referenced this pull request May 4, 2021
chinmaygarde pushed a commit to chinmaygarde/flutter_buildroot that referenced this pull request May 4, 2021
This reverts commit 72cd672.

Co-authored-by: Kaushik Iska <kaushikiska@google.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants
0