Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Mac universal builds #3120

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Mac universal builds #3120

wants to merge 7 commits into from

Conversation

colinbendell
Copy link

@colinbendell colinbendell commented Dec 1, 2021 •
edited

Summary

New M1 Mac's must use rosetta to use Flipper. The Electron builder only produces x86_64 builds and the PortForwardingApp is precompiled and committed in the code base with an x64 build. This PR adds universal building of the electron app and the PortForwardingMacApp

Addresses: #3073, #2169, #2193, #2898

Changelog

Enable universal mac (mac/x64) builds

Test Plan

  • yarn build --mac to build the application as per usual
  • run the flipper.app as usual
  • verify that app is working and that on an m1, the Kind is Apple

@facebook-github-bot facebook-github-bot added the CLA Signed Do not delete this pull request or issue due to inactivity. label Dec 1, 2021
colinbendell
colinbendell commented Dec 1, 2021
View reviewed changes
iOS/Podfile Outdated
#use_frameworks!
platform :osx, '11'
workspace 'FlipperKit.xcworkspace'
project '../PortForwardingMacApp.xcodeproj'
Copy link
Author

Choose a reason for hiding this comment

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

Placing the PortFrowarding project at the root level is messy. I only placed it there because the src is split between /PortForwardingMacApp/main.m and /ios/FlipperKit/FKPortForwarding/*.

Q: should we just refactor this and place the project and relevant sources under /desktop?

Copy link
Author

Choose a reason for hiding this comment

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

Decided to refactor anyway. PR now moves /ios/FlipperKit/FKPortForwarding/FKPortForwardingClient.* to /desktop/PortForwardingMacApp

@colinbendell colinbendell changed the title Update to enable Mac universal builds of Flipper Mac universal builds Dec 1, 2021
@colinbendell
Update to enable universal builds of Flipper
bb0a7de 
* added PortForwardingMacApp project target
* updated Podfiles
* updated electron builder to 22.14.10
* updated electron to 16.0.3
* added universal architecture to the build process
@colinbendell
revert to electron 11
7ca9d98
@colinbendell
add static binary to repo
b7b2338
@colinbendell
cleanup
4d49241
@coveralls
Copy link

coveralls commented Dec 3, 2021 •
edited

Pull Request Test Coverage Report for Build 1528623973

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 14 unchanged lines in 3 files lost coverage.
  • Overall coverage increased (+0.004%) to 48.257%
Files with Coverage Reduction New Missed Lines %
desktop/flipper-server-core/src/devices/ios/iOSContainerUtility.tsx 2 26.15%
desktop/flipper-ui-core/src/sandy-chrome/appinspect/LaunchEmulator.tsx 2 59.42%
desktop/flipper-server-core/src/devices/ios/IOSBridge.tsx 10 68.33%
Totals Coverage Status
Change from base Build 1527315526: 0.004%
Covered Lines: 7423
Relevant Lines: 13974
💛 - Coveralls

@passy
Copy link
Member

passy commented Dec 3, 2021

Thanks for putting this together, @colinbendell!

We'll probably need some time to look into this. I'm not quite sure what this means when rolling it out to M1s due the binary signing requires for which we currently have no infrastructure in place.

@colinbendell
Copy link
Author

colinbendell commented Dec 5, 2021 •
edited

@passy, Why not simply use ad-hoc signed binaries? This is the approach that homebrew uses instead of signing binaries.

I've confirmed that with xcode 13 and the latest electron-builder that the resulting binaries are already ad-hoc signed when a valid Identity is not found or when CSC_IDENTITY_AUTO_DISCOVERY=false. Using this as-is would be sufficient for the time being.

@marko-asplund
Copy link

Any news on publishing universal builds?

@ZComwiz
Copy link

ZComwiz commented Jan 18, 2022

Let's not leave Mac Catalyst behind: #3117

@colinbendell
Copy link
Author

Let's not leave Mac Catalyst behind: #3117

what's the intersection with this PR and Mac Catalyst?

@aarongrider
Copy link

aarongrider commented Jan 26, 2022 •
edited

Let me know if I can help at all with getting this over the line. Looks like the build might need some attention.

@umangloria
Copy link

Still waiting for this PR to get merged :(

@aarongrider aarongrider mentioned this pull request Mar 19, 2022
Open
@lockieluke
Copy link

please hurry up react native devs apple silicon machines were released two years ago

@cristiandan
Copy link

i guess this is just as good as #3553

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
CLA Signed Do not delete this pull request or issue due to inactivity.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
10 participants
@colinbendell @coveralls @passy @marko-asplund @ZComwiz @aarongrider @umangloria @lockieluke @cristiandan @facebook-github-bot