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.

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

[Fastlane.swift] fixes issues with running on Apple Silicon #18502 #19555

Merged
merged 34 commits into from
Feb 2, 2022

Conversation

kikeenrique
Copy link
Contributor

@kikeenrique kikeenrique commented Oct 29, 2021

Checklist

  • I've run bundle exec rspec from the root directory to see all new and existing tests pass
  • I've followed the fastlane code style and run bundle exec rubocop -a to ensure the code style is valid
  • I've read the Contribution Guidelines
  • I've updated the documentation if necessary.

Motivation and Context

Swift fastlane does not run on Apple Silicon, it crashes due to unsigned binary and also next due to a data race condition.

Description

  • Update code to swift 5
  • Enable binary code signing, required for M1.
  • Fix data race
  • Modify copy binary phase, that copies the binary signed.
  • Added verbose option to allow output to be seen if needed when swift fastlane binary is launched.

I created a swift fastlane fresh project, adding next lane on fastlane/Fastfile.swift to test basic functionality.

class Fastfile: LaneFile {
	func customLane() {
        desc("Description of what the lane does")
        print("test if output is printed somewhere")
        sh(command: "echo hola", log: false)
        sh(command: "echo hola", log: true)
        sh(command: "ls -la", log: true)
	}
}

Testing Steps

I would add some tests for swift fastlane executing basic actions on M1 host, as I did, but I lack some knowledge on fastlane on how to do it.

…e#18502

* [Fastlane.Swift] Xcode recommedation. Enabling Base Internationalization is recommended for all projects.

* Migrating the “English, deprecated” localization to “English” is recommended for all projects.
   This will ensure localized resources are placed in “en.lproj” directories instead of deprecated “English.lproj” directories.
…e#18502

* [Fastlane.Swift] Xcode recommendation. Enable recommended warnings.
  Implicit retain of self within blocks
  Overriding deprecated Objective-C methods
  Quoted Include in framework header.
…e#18502

* [Fastlane.Swift] Xcode recommendation. Enable code signing. It's recommended for macOS executables. This setting will cause executable's code signature to be trusted by your Mac.
   Currently, macOS on M1 is killing the binary at launch. According to console traces, reason is ASP Security policy would not allow process.
…e#18502

* [Fastlane.Swift] Fix waring Using 'class' keyword to define a class-constrained protocol is deprecated; use 'AnyObject' instead.
…e#18502

* [Fastlane.Swift] Fix waring 'withUnsafeBytes' is deprecated: use `withUnsafeBytes<R>(_: (UnsafeRawBufferPointer) throws -> R) rethrows -> R` instead
…e#18502

* [Fastlane.Swift] Fix waring Switch covers known cases, but 'DispatchTimeInterval' may have additional unknown values, possibly added in future versions. Handle unknown values using "@unknown default"
…e#18502

* [Fastlane.Swift] Update protocol generator to use AnyObject
…e#18502

* [Fastlane.Swift] Fix thread data race using a thread safe storage
…e#18502

* [Fastlane.Swift] On verbose mode, allow output from the runner thread. It allows traces and ease feedback from fastlane swift.
…e#18502

* [Fastlane.Swift] Replace Build Phase - ShellScript with Build Phase - CopyFiles. This allows to copy the executable signed.
…e#18502

* [Testing] Add some useful help to debug problems on FastlaneSwiftRunner
…e#18502

* [Testing] Fix ensure_actions_config_items_formatting
@google-cla google-cla bot added the cla: yes label Oct 29, 2021
@kikeenrique
Copy link
Contributor Author

If any help is needed on reviewing I'm open to a video meeting 😀

Copy link
Member

@joshdholtz joshdholtz left a comment

Choose a reason for hiding this comment

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

This looks really good! I think I just have one question on this code 😊

I'm going to do a bit more testing and see if I can get another review but this is 💯

@joshdholtz joshdholtz changed the title Swift fastlane does not run on Apple Silicon #18502 [Fastlane.swift] fixes issues with running on Apple Silicon #18502 Nov 30, 2021
@minuscorp
Copy link
Collaborator

I don't have an M1 to test with :(

@kikeenrique
Copy link
Contributor Author

I don't have an M1 to test with :(

It's a pity that GitHub actions doesn't have still M1 builders (actions/runner-images#2187).
I wonder if any other CI service does.

Copy link
Member

@rogerluan rogerluan left a comment

Choose a reason for hiding this comment

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

I have some minor formatting suggestions that I've posted below @kikeenrique, but when I tested this PR, I faced some issues.

My setup:

  1. mkdir fastlane-test && cd fastlane-test
  2. Created a Gemfile and put this content inside of it:
source 'https://rubygems.org'

gem "fastlane" # Notice that I was pointing to the latest version of fastlane, and not your PR
  1. bundle install
  2. bundle exec fastlane swift init
  3. Go through the installation steps accepting all default config

Then I got this error:

image

Which's probably expected - it's probably what this PR aims to fix, right? 😄 Since I'm running on M1 and pointing fastlane to the latest release.

So then I modified my Gemfile to point to your repo and branch:

source 'https://rubygems.org'

gem "fastlane", :git => "https://github.com/kikeenrique/fastlane.git", :branch => "18502"

And ran bundle exec fastlane custom again, and got the following:

image

image

PS: Click the picture to enlarge them 🙏

Then I ran bundle exec fastlane custom again, and the error changes:

image

No idea what was causing that. But I thought I may have corrupted the files, so I deleted the fastlane directory that bundle exec fastlane swift init had created, and set it up again:

  1. bundle exec fastlane swift init
  2. bundle exec fastlane custom

And then it worked just fine:

image

I double checked that the steps I just posted are reproducible (I did everything twice again and obtained the same results).

@kikeenrique @joshdholtz any idea how we can prevent this from happening? It looks like a terrible UX if this will be a breaking release for Fastlane.Swift users 😬

Also tagging @minuscorp in here, the real Fastlane.Swift bad ass 🙇

kikeenrique and others added 3 commits December 1, 2021 09:50
Include review comments

Co-authored-by: Roger Oba <[email protected]>
Include review comments

Co-authored-by: Roger Oba <[email protected]>
Include review comments

Co-authored-by: Roger Oba <[email protected]>
@kikeenrique
Copy link
Contributor Author

I will take a look at this upgrade problems, I don't know why that happened.
The kill that you got before resetting fastlane directory smells to problems with signing on M1.
At a first sight, both problems look like the Xcode project didn't get fully upgraded, as I said, I will look further into this.

As a side note, I've experienced in the past problems in macOS when signing files with same name and overwriting them again with a copy. I read in Apple forums that it was a macOS cache problem that sometimes maybe fixed moving files instead of copying.

@kikeenrique
Copy link
Contributor Author

Latest commit fixed the crash. 🎉

@minuscorp
Copy link
Collaborator

Not the implementation, I wanted but what I could with the time I had, I'll try to fix the code in a better way 😅

@kikeenrique
Copy link
Contributor Author

@rogerluan @joshdholtz Although I can see a 🔴_Merging is blocked_ message due to requested changes, I think everything is ready to be merged at least on my side and @minuscorp, so whenever you're ready it can go to 🚀🌕.

@minuscorp
Copy link
Collaborator

I've made a bit of cleanup of pretty hacky code. More readable and extensible for other types. Approved but test it beforehand.

@kikeenrique
Copy link
Contributor Author

I've made a bit of cleanup of pretty hacky code. More readable and extensible for other types. Approved but test it beforehand.

Related to testing, I've created another PR 19755 with the test I've using to test this PR.
I've added it in other PR in order to discuss it independently and don't add more delay in this PR.

@Robin-Gupta1
Copy link

Hey guys can we please fix this issue as it is causing a lot of problems for us while working with Fastlane swift? We have waited for more than 3 months for this issue to be resolved and now this PR is sitting ideal for more than a week without any activites. Please help us.

Copy link
Member

@rogerluan rogerluan left a comment

Choose a reason for hiding this comment

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

Code LGTM :)

@hiragram
Copy link

👀 🚀 ❓

@khaptonstall
Copy link

@kikeenrique My team just started running into issues that should be resolved once this is released, thanks for submitting a fix! It looks like this has been open for quite some time. Is there an estimate on when this might get merged? What's currently blocking it?

@kikeenrique
Copy link
Contributor Author

@kikeenrique My team just started running into issues that should be resolved once this is released, thanks for submitting a fix! It looks like this has been open for quite some time. Is there an estimate on when this might get merged? What's currently blocking it?

At this point I think is just a matter of official maintainers availability to merge it (we all are volunteers with a life 🌱), as it has already been approved by reviewers.

@roisg
Copy link

roisg commented Feb 1, 2022

Is there an approximate timeline for this to be merged and released? Does anyone know how much time would usually take for an approved request to be released?

Thank you very much for the great effort.

@dgilbert-foreflight
Copy link

👀 🚀 ❓

@joshdholtz
Copy link
Member

Getting this merged and released in new version tonight!

Copy link
Member

@joshdholtz joshdholtz left a comment

Choose a reason for hiding this comment

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

This looks amazing! Great work, team 🥰 I will get this out in tonight's release 💪

@joshdholtz joshdholtz merged commit 77e732c into fastlane:master Feb 2, 2022
@fastlane-bot
Copy link

Congratulations! 🎉 This was released as part of fastlane 2.204.0 🚀

@fastlane fastlane locked and limited conversation to collaborators Apr 5, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.