-
Notifications
You must be signed in to change notification settings - Fork 257
Conversation
There was a problem hiding this 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.
build/toolchain/mac/BUILD.gn
Outdated
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 |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
@dnfield I see that you did the last toolchain roll 9 months ago here? flutter/engine#17483. Are we due for another roll? |
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. |
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. |
@chaselatta is/was updating the toolchain at @dnfield 9 months ago you updated the toolchain at What's the difference? Do we need to update both? |
It's also worth noting that Fuchsia doesn't publish mac-arm64 binaries, so we'd be using Rosetta. |
Ahhh, I'm not quite sure what the fuchsia toolchain host files are then :\ The |
(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) |
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. |
|
Filed flutter/flutter#73757 for the Clang 12.0.0 roll - I'm looking at what this'll take right now. |
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. |
@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. |
c26f967
to
8f51a47
Compare
So, we don't need to actually set any special cflags if we have |
I get one (non-fatal) warning in two different files building harfbuzz with clang from xcode:
|
@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 |
@@ -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, |
There was a problem hiding this comment.
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
There was a problem hiding this 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)
Are we making progress on this? |
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. |
Upstream issue filed: harfbuzz/harfbuzz#2834 |
There was a problem hiding this 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.
/cc @xster |
Woohoo! (no bugs attached, is there a sibling work for arm64 macos builds as well? 😀) |
@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 |
\o/ |
sorry that should say it's NOT on my immediate radar.. :) |
too late, I already bought all the fireworks and booked our hawaii celebration trips |
This reverts commit 72cd672.
This reverts commit 72cd672. Co-authored-by: Kaushik Iska <kaushikiska@google.com>
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.