-
Notifications
You must be signed in to change notification settings - Fork 5.7k
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
Fix fetch_provisioning_profile
when output_path
is provided as a match option
#21946
Fix fetch_provisioning_profile
when output_path
is provided as a match option
#21946
Conversation
match/lib/match/runner.rb
Outdated
@@ -356,7 +356,7 @@ def fetch_provisioning_profile(params: nil, profile_type:, certificate_id: nil, | |||
|
|||
if params[:output_path] | |||
FileUtils.cp(stored_profile_path, params[:output_path]) | |||
installed_profile = FastlaneCore::ProvisioningProfile.install(profile, keychain_path) | |||
installed_profile = FastlaneCore::ProvisioningProfile.install(stored_profile_path, keychain_path) |
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.
@nekrich https://github.com/fastlane/fastlane/pull/21691/files#diff-8cd09d7ab3f4398e3160f603839ac2891e3cdf60e7f09930a1288ce33ab8fc1fR359
Looking deeper: I'm a bit less sure about the intent here. It looks to me like we already do a FastlaneCore::ProvisioningProfile.install
on line 354 just above this: https://github.com/fastlane/fastlane/blob/master/match/lib/match/runner.rb#L354
Is it necessary to run that again if output_path
is supplied? Wouldn't that be the same profile that was installed on line 354?
If so, is stored_profile_path
or params[:output_path]
the correct param?
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.
Thanks @TheMetalCode for investigation 💪 This looks like a typo, but as you noted it doesn't seem to be necessary and could be removed.
Installing profile (copying it to a directory used by Xcode) makes sense only on a Mac OS and it's done in line 354.
Also certificates wouldn't be installed on another operating system
fastlane/match/lib/match/runner.rb
Lines 220 to 239 in 152b382
if Helper.mac? | |
UI.message("Installing certificate...") | |
# Only looking for cert in "custom" (non login.keychain) keychain | |
# Doing this for backwards compatibility | |
keychain_name = params[:keychain_name] == "login.keychain" ? nil : params[:keychain_name] | |
if FastlaneCore::CertChecker.installed?(cert_path, in_keychain: keychain_name) | |
UI.verbose("Certificate '#{File.basename(cert_path)}' is already installed on this machine") | |
else | |
Utils.import(cert_path, params[:keychain_name], password: params[:keychain_password]) | |
# Import the private key | |
# there seems to be no good way to check if it's already installed - so just install it | |
# Key will only be added to the partition list if it isn't already installed | |
Utils.import(select_cert_or_key(paths: keys), params[:keychain_name], password: params[:keychain_password]) | |
end | |
else | |
UI.message("Skipping installation of certificate as it would not work on this operating system.") | |
end |
Even if we wanted to install the same profile twice, it would be copied to under the same name, as those profiles are named with UUID.
fastlane/fastlane_core/lib/fastlane_core/provisioning_profile.rb
Lines 75 to 88 in 152b382
def profiles_path | |
path = File.expand_path("~") + "/Library/MobileDevice/Provisioning Profiles/" | |
# If the directory doesn't exist, create it first | |
unless File.directory?(path) | |
FileUtils.mkdir_p(path) | |
end | |
return path | |
end | |
# Installs a provisioning profile for Xcode to use | |
def install(path, keychain_path = nil) | |
UI.message("Installing provisioning profile...") | |
destination = File.join(profiles_path, profile_filename(path, keychain_path)) |
fastlane/fastlane_core/lib/fastlane_core/provisioning_profile.rb
Lines 38 to 40 in 152b382
def uuid(path, keychain_path = nil) | |
parse(path, keychain_path).fetch("UUID") | |
end |
fastlane/fastlane_core/lib/fastlane_core/provisioning_profile.rb
Lines 62 to 65 in 152b382
def profile_filename(path, keychain_path = nil) | |
basename = uuid(path, keychain_path) | |
basename + profile_extension(path, keychain_path) | |
end |
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.
@lucgrabowski Appreciate the response! It does seem safe to simply remove the line that was causing the issue, which I've done and have tested successfully
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.
Thanks for the investigation, fix and improving test 💪. Looks good to me 🔥
How long before this fix is released? It's fixing a pretty serious breaking bug. |
Hi guys, anyone here? I still got the issue. I'm running it on self-hosted (MacBook M1 Pro) github action. |
Same here, I'd really appreciate some update on the potential release date. Thanks! |
Checklist
bundle exec rspec
from the root directory to see all new and existing tests passbundle exec rubocop -a
to ensure the code style is validci/circleci
builds in the "All checks have passed" section of my PR (connect CircleCI to GitHub if not)Motivation and Context
Resolves #21945
Description
Given some investigation and subsequent conversation, this PR proposes that the line of code causing #21945 can simply be removed. It should not be necessary to run
FastlaneCore::ProvisioningProfile.install
a second time whenoutput_path
is provided as a match option.Testing Steps
For my own testing, I used this branch of
fastlane
to execute the same CI workflow (from a private project) that was failing on2.220.0
with the error described in #21945. Thematch
actions for all builds, as well as the subsequentgym
actions, all ran successfully with no observable side effects.