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

Fix fetch_provisioning_profile when output_path is provided as a match option #21946

Conversation

TheMetalCode
Copy link
Contributor

@TheMetalCode TheMetalCode commented Apr 4, 2024

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 see several green ci/circleci builds in the "All checks have passed" section of my PR (connect CircleCI to GitHub if not)
  • I've read the Contribution Guidelines
  • I've updated the documentation if necessary.
  • I've added or updated relevant unit tests.

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 when output_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 on 2.220.0 with the error described in #21945. The match actions for all builds, as well as the subsequent gym actions, all ran successfully with no observable side effects.

match(
    output_path: "/tmp/match_certs", # must include, this creates the condition that this PR fixes
    # everything below is merge suggestion, use whatever you consider to be a standard set of match params
    git_url: YOUR_MATCH_REPO_URL,
    type: "adhoc",
    app_identifier: YOUR_APP_IDENTIFIER,
    force_for_new_devices: true,
    include_all_certificates: true,
    force_for_new_certificates: true,
    shallow_clone: true,
    readonly: true,
)

@@ -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)
Copy link
Contributor Author

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?

Copy link
Contributor

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

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.

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))

def uuid(path, keychain_path = nil)
parse(path, keychain_path).fetch("UUID")
end

def profile_filename(path, keychain_path = nil)
basename = uuid(path, keychain_path)
basename + profile_extension(path, keychain_path)
end

Copy link
Contributor Author

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

@TheMetalCode TheMetalCode marked this pull request as ready for review April 5, 2024 20:54
Copy link
Contributor

@lucgrabowski lucgrabowski left a 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 🔥

@lucgrabowski lucgrabowski merged commit 3060e06 into fastlane:master Apr 7, 2024
3 checks passed
@legoesbenr
Copy link

How long before this fix is released? It's fixing a pretty serious breaking bug.
We are using fastlane only to resign, so we don't have a gemfile to pin to the previous version.

@tiennguyen1203
Copy link

Hi guys, anyone here? I still got the issue.

I'm running it on self-hosted (MacBook M1 Pro) github action.

@jakub-przy
Copy link

Same here, I'd really appreciate some update on the potential release date. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[match] undefined local variable or method profile when running match with output_path
5 participants