Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Fix detection of Mac target specification as a host build.#35063

Merged
auto-submit[bot] merged 1 commit intoflutter:mainfrom
chinmaygarde:fix_mac_builds
Aug 1, 2022
Merged

Fix detection of Mac target specification as a host build.#35063
auto-submit[bot] merged 1 commit intoflutter:mainfrom
chinmaygarde:fix_mac_builds

Conversation

@chinmaygarde
Copy link
Contributor

@chinmaygarde chinmaygarde commented Aug 1, 2022

@chinmaygarde
Copy link
Contributor Author

chinmaygarde commented Aug 1, 2022

cc @zanderso since you authored the patch.

gaaclarke
gaaclarke approved these changes Aug 1, 2022
View reviewed changes
Copy link
Member

@gaaclarke gaaclarke left a comment

Choose a reason for hiding this comment

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

This is an improvement over the status quo. I think there is other work we should do that makes the host and the --mac build different though. That can be addressed in the future.

@chinmaygarde
Copy link
Contributor Author

chinmaygarde commented Aug 1, 2022

Can you file an issue for that? I was thinking of the opposite of just having --mac be the default with no host variant.

@chinmaygarde chinmaygarde added the autosubmit Merge PR when tree becomes green via auto submit App label Aug 1, 2022
@zanderso
Copy link
Member

zanderso commented Aug 1, 2022 •
edited
Loading

Does building on an arm64 mac still work with this patch?

The logic here might have a different result, but I'm just eyeballing it: https://github.com/flutter/engine/blob/main/tools/gn#L292

@chinmaygarde
Copy link
Contributor Author

chinmaygarde commented Aug 1, 2022 •
edited
Loading

Does building on an arm64 mac still work with this patch?

Really can't say without access to a device right now or on CI. I expect so since this patch fixes just the build with the --mac flag. And, if I am reading that code correctly, we need emulation on Mac still right?

But yeah, we should wire this up on CI to be sure. I have an M2 device on order to test by my desk too but that is being delayed.

@auto-submit auto-submit bot merged commit 4063cfb into flutter:main Aug 1, 2022
@chinmaygarde chinmaygarde deleted the fix_mac_builds branch August 1, 2022 22:15
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Aug 2, 2022
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 2, 2022
@engine-flutter-autoroll
4063cfb Fix detection of Mac target specification as a host build. (f…
4dfbffa 
…lutter/engine#35063)
betrevisan pushed a commit to betrevisan/engine that referenced this pull request Aug 5, 2022
@chinmaygarde @betrevisan
Fix detection of Mac target specification as a host build. (flutter#3…
a4d49b8 
…5063)
emilyabest pushed a commit to emilyabest/engine that referenced this pull request Aug 12, 2022
@chinmaygarde @emilyabest
Fix detection of Mac target specification as a host build. (flutter#3…
c442212 
…5063)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@zanderso zanderso Awaiting requested review from zanderso

1 more reviewer

@gaaclarke gaaclarke gaaclarke approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

autosubmit Merge PR when tree becomes green via auto submit App

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

GN error when building macos engine

3 participants

@chinmaygarde @zanderso @gaaclarke