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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

[deliver] Implements verify with altool for Xcode 14 validation #20738

Merged
merged 14 commits into from
Mar 28, 2023
Merged
Show file tree
Hide file tree
Changes from 11 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
16 changes: 8 additions & 8 deletions deliver/lib/deliver/runner.rb
Original file line number Diff line number Diff line change
Expand Up @@ -169,7 +169,7 @@ def verify_binary
pkg_path = options[:pkg]

platform = options[:platform]
transporter = transporter_for_selected_team
transporter = transporter_for_selected_team(altool_compatible_command: true)

case platform
when "ios", "appletvos"
Expand All @@ -179,15 +179,15 @@ def verify_binary
package_path: "/tmp",
platform: platform
)
result = transporter.verify(package_path: package_path)
result = transporter.verify(package_path: package_path, asset_path: ipa_path, platform: platform)
when "osx"
package_path = FastlaneCore::PkgUploadPackageBuilder.new.generate(
app_id: Deliver.cache[:app].id,
pkg_path: pkg_path,
package_path: "/tmp",
platform: platform
)
result = transporter.verify(package_path: package_path)
result = transporter.verify(package_path: package_path, asset_path: pkg_path, platform: platform)
else
UI.user_error!("No suitable file found for verify for platform: #{options[:platform]}")
end
Expand All @@ -206,7 +206,7 @@ def upload_binary
pkg_path = options[:pkg]

platform = options[:platform]
transporter = transporter_for_selected_team(upload: true)
transporter = transporter_for_selected_team(altool_compatible_command: true)

case platform
when "ios", "appletvos"
Expand Down Expand Up @@ -266,7 +266,7 @@ def submit_for_review
# If itc_provider was explicitly specified, use it.
# If there are multiple teams, infer the provider from the selected team name.
# If there are fewer than two teams, don't infer the provider.
def transporter_for_selected_team(upload: false)
def transporter_for_selected_team(altool_compatible_command: false)
Copy link
Collaborator

Choose a reason for hiding this comment

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

What's the situation altool_compatible_command to be false?

I think this flag is not necessary for ItunesTransporter.

In the previous implementation, verify task doesn't use altool, so some flags are needed to decide which executors are used.
However, in the current implementation, the transporter always uses altool when meeting the situations.
So I think this is not needed. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it will still be needed as not all commands have altool implementations. The reason why the upload property existed was so that even if the conditions were met only upload would use itmstransporter, as it was implemented.

As we go ahead and add more commands, we'll need to mark them as altool_compatible so that we can slowly move them across. I might be missing something but I think it's the only way we can do it at the moment without breaking anything. Let me know what you think 馃憤

Copy link
Collaborator

Choose a reason for hiding this comment

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

I meant all callers of this method seem to pass true for altool_compatible_command

https://github.com/fastlane/fastlane/pull/20738/files#diff-51e845b40462cd52020ab428469c85f65e24e1343e0b17c88d0c1a11e1ec22ecR172
https://github.com/fastlane/fastlane/pull/20738/files#diff-51e845b40462cd52020ab428469c85f65e24e1343e0b17c88d0c1a11e1ec22ecR209

So we can get rid of this argument. We can always pass true on L284 to reduce unnecessary states.

# Use JWT auth
api_token = Spaceship::ConnectAPI.token
api_key = if options[:api_key].nil? && !api_token.nil?
Expand All @@ -281,20 +281,20 @@ def transporter_for_selected_team(upload: false)

unless api_token.nil?
api_token.refresh! if api_token.expired?
return FastlaneCore::ItunesTransporter.new(nil, nil, false, nil, api_token.text, upload: upload, api_key: api_key)
return FastlaneCore::ItunesTransporter.new(nil, nil, false, nil, api_token.text, altool_compatible_command: altool_compatible_command, api_key: api_key)
Copy link
Collaborator

Choose a reason for hiding this comment

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

We can always pass altool_compatible_command: true because of the above comment.

end

tunes_client = Spaceship::ConnectAPI.client.tunes_client

generic_transporter = FastlaneCore::ItunesTransporter.new(options[:username], nil, false, options[:itc_provider], upload: upload, api_key: api_key)
generic_transporter = FastlaneCore::ItunesTransporter.new(options[:username], nil, false, options[:itc_provider], altool_compatible_command: altool_compatible_command, api_key: api_key)
return generic_transporter unless options[:itc_provider].nil? && tunes_client.teams.count > 1

begin
team = tunes_client.teams.find { |t| t['providerId'].to_s == tunes_client.team_id }
name = team['name']
provider_id = generic_transporter.provider_ids[name]
UI.verbose("Inferred provider id #{provider_id} for team #{name}.")
return FastlaneCore::ItunesTransporter.new(options[:username], nil, false, provider_id, upload: upload, api_key: api_key)
return FastlaneCore::ItunesTransporter.new(options[:username], nil, false, provider_id, altool_compatible_command: altool_compatible_command, api_key: api_key)
rescue => ex
UI.verbose("Couldn't infer a provider short name for team with id #{tunes_client.team_id} automatically: #{ex}. Proceeding without provider short name.")
return generic_transporter
Expand Down
6 changes: 3 additions & 3 deletions deliver/spec/runner_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ def provider_ids
expect_any_instance_of(FastlaneCore::IpaUploadPackageBuilder).to receive(:generate)
.with(app_id: 'YI8C2AS', ipa_path: 'ACME.ipa', package_path: '/tmp', platform: 'ios')
.and_return('path')
expect(transporter).to receive(:verify).with(package_path: 'path').and_return(true)
expect(transporter).to receive(:verify).with(asset_path: "ACME.ipa", package_path: 'path', platform: "ios").and_return(true)
runner.verify_binary
end
end
Expand All @@ -124,7 +124,7 @@ def provider_ids
expect_any_instance_of(FastlaneCore::IpaUploadPackageBuilder).to receive(:generate)
.with(app_id: 'YI8C2AS', ipa_path: 'ACME.ipa', package_path: '/tmp', platform: 'appletvos')
.and_return('path')
expect(transporter).to receive(:verify).with(package_path: 'path').and_return(true)
expect(transporter).to receive(:verify).with(asset_path: "ACME.ipa", package_path: 'path', platform: "appletvos").and_return(true)
runner.verify_binary
end
end
Expand All @@ -140,7 +140,7 @@ def provider_ids
expect_any_instance_of(FastlaneCore::PkgUploadPackageBuilder).to receive(:generate)
.with(app_id: 'YI8C2AS', pkg_path: 'ACME.pkg', package_path: '/tmp', platform: 'osx')
.and_return('path')
expect(transporter).to receive(:verify).with(package_path: 'path').and_return(true)
expect(transporter).to receive(:verify).with(asset_path: "ACME.pkg", package_path: 'path', platform: "osx").and_return(true)
runner.verify_binary
end
end
Expand Down
60 changes: 46 additions & 14 deletions fastlane_core/lib/fastlane_core/itunes_transporter.rb
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ def build_upload_command(username, password, source = "/tmp", provider_short_nam
not_implemented(__method__)
end

def build_verify_command(username, password, source = "/tmp", provider_short_name = "", jwt = nil)
def build_verify_command(username, password, source = "/tmp", provider_short_name = "", **kwargs)
not_implemented(__method__)
end

Expand Down Expand Up @@ -307,8 +307,22 @@ def build_download_command(username, password, apple_id, destination = "/tmp", p
raise "This feature has not been implemented yet with altool for Xcode 14"
end

def build_verify_command(username, password, source = "/tmp", provider_short_name = "", jwt = nil)
raise "This feature has not been implemented yet with altool for Xcode 14"
def build_verify_command(username, password, source = "/tmp", provider_short_name = "", **kwargs)
api_key = kwargs[:api_key]
platform = kwargs[:platform]
use_api_key = !api_key.nil?
[
("API_PRIVATE_KEYS_DIR=#{api_key[:key_dir]}" if use_api_key),
"xcrun altool",
"--validate-app",
("-u #{username.shellescape}" unless use_api_key),
("-p #{password.shellescape}" unless use_api_key),
("--apiKey #{api_key[:key_id]}" if use_api_key),
("--apiIssuer #{api_key[:issuer_id]}" if use_api_key),
("--asc-provider #{provider_short_name}" unless use_api_key || provider_short_name.to_s.empty?),
platform_option(platform),
file_upload_option(source)
].compact.join(' ')
end

def additional_upload_parameters
Expand Down Expand Up @@ -409,7 +423,8 @@ def build_provider_ids_command(username, password, jwt = nil, api_key = nil)
].compact.join(' ')
end

def build_verify_command(username, password, source = "/tmp", provider_short_name = "", jwt = nil)
def build_verify_command(username, password, source = "/tmp", provider_short_name = "", **kwargs)
jwt = kwargs[:jwt]
use_jwt = !jwt.to_s.empty?
[
'"' + Helper.transporter_path + '"',
Expand Down Expand Up @@ -504,7 +519,8 @@ def build_upload_command(username, password, source = "/tmp", provider_short_nam
end
end

def build_verify_command(username, password, source = "/tmp", provider_short_name = "", jwt = nil)
def build_verify_command(username, password, source = "/tmp", provider_short_name = "", **kwargs)
jwt = kwargs[:jwt]
use_jwt = !jwt.to_s.empty?
if !Helper.user_defined_itms_path? && Helper.mac? && Helper.xcode_at_least?(11)
[
Expand Down Expand Up @@ -660,7 +676,7 @@ def self.hide_transporter_output?
# see: https://github.com/fastlane/fastlane/issues/1524#issuecomment-196370628
# for more information about how to use the iTMSTransporter to list your provider
# short names
def initialize(user = nil, password = nil, use_shell_script = false, provider_short_name = nil, jwt = nil, upload: false, api_key: nil)
def initialize(user = nil, password = nil, use_shell_script = false, provider_short_name = nil, jwt = nil, altool_compatible_command: false, api_key: nil)
# Xcode 6.x doesn't have the same iTMSTransporter Java setup as later Xcode versions, so
# we can't default to using the newer direct Java invocation strategy for those versions.
use_shell_script ||= Helper.is_mac? && Helper.xcode_version.start_with?('6.')
Expand All @@ -675,7 +691,7 @@ def initialize(user = nil, password = nil, use_shell_script = false, provider_sh
@jwt = jwt
@api_key = api_key

if should_use_altool?(upload, use_shell_script)
if should_use_altool?(altool_compatible_command, use_shell_script)
UI.verbose("Using altool as transporter.")
@transporter_executor = AltoolTransporterExecutor.new
else
Expand Down Expand Up @@ -800,10 +816,20 @@ def upload(app_id = nil, dir = nil, package_path: nil, asset_path: nil, platform
# @return (Bool) True if everything worked fine
# @raise [Deliver::TransporterTransferError] when something went wrong
# when transferring
def verify(app_id = nil, dir = nil, package_path: nil)
raise "Either a combination of app id and directory or a package_path are required" if (app_id.nil? || dir.nil?) && package_path.nil?
def verify(app_id = nil, dir = nil, package_path: nil, asset_path: nil, platform: nil)
raise "app_id and dir are required or package_path or asset_path is required" if (app_id.nil? || dir.nil?) && package_path.nil? && asset_path.nil?

force_itmsp = FastlaneCore::Env.truthy?("ITMSTRANSPORTER_FORCE_ITMS_PACKAGE_UPLOAD")
can_use_asset_path = Helper.is_mac? && asset_path

actual_dir = if package_path
actual_dir = if can_use_asset_path && !force_itmsp
# The asset gets deleted upon completion so copying to a temp directory
# (with randomized filename, for multibyte-mixed filename upload fails)
new_file_name = "#{SecureRandom.uuid}#{File.extname(asset_path)}"
tmp_asset_path = File.join(Dir.tmpdir, new_file_name)
FileUtils.cp(asset_path, tmp_asset_path)
tmp_asset_path
giginet marked this conversation as resolved.
Show resolved Hide resolved
giginet marked this conversation as resolved.
Show resolved Hide resolved
elsif package_path
package_path
else
File.join(dir, "#{app_id}.itmsp")
Expand All @@ -812,8 +838,14 @@ def verify(app_id = nil, dir = nil, package_path: nil)
password_placeholder = @jwt.nil? ? 'YourPassword' : nil
jwt_placeholder = @jwt.nil? ? nil : 'YourJWT'

command = @transporter_executor.build_verify_command(@user, @password, actual_dir, @provider_short_name, @jwt)
UI.verbose(@transporter_executor.build_verify_command(@user, password_placeholder, actual_dir, @provider_short_name, jwt_placeholder))
# Handle AppStore Connect API
use_api_key = !@api_key.nil?
api_key_placeholder = use_api_key ? { key_id: "YourKeyID", issuer_id: "YourIssuerID", key_dir: "YourTmpP8KeyDir" } : nil
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's better to be commented for code readability 馃憤

Suggested change
api_key_placeholder = use_api_key ? { key_id: "YourKeyID", issuer_id: "YourIssuerID", key_dir: "YourTmpP8KeyDir" } : nil
# Masking credentials for verbose outputs
api_key_placeholder = use_api_key ? { key_id: "YourKeyID", issuer_id: "YourIssuerID", key_dir: "YourTmpP8KeyDir" } : nil


api_key = api_key_with_p8_file_path(@api_key) if use_api_key

command = @transporter_executor.build_verify_command(@user, @password, actual_dir, @provider_short_name, jwt: @jwt, platform: platform, api_key: api_key)
UI.verbose(@transporter_executor.build_verify_command(@user, password_placeholder, actual_dir, @provider_short_name, jwt: jwt_placeholder, platform: platform, api_key: api_key_placeholder))

begin
result = @transporter_executor.execute(command, ItunesTransporter.hide_transporter_output?)
Expand Down Expand Up @@ -883,9 +915,9 @@ def api_key_with_p8_file_path(original_api_key)
end

# Returns whether altool should be used or ItunesTransporter should be used
def should_use_altool?(upload, use_shell_script)
def should_use_altool?(altool_compatible_command, use_shell_script)
# Xcode 14 no longer supports iTMSTransporter. Use altool instead
!use_shell_script && upload && !Helper.user_defined_itms_path? && Helper.mac? && Helper.xcode_at_least?(14)
!use_shell_script && altool_compatible_command && !Helper.user_defined_itms_path? && Helper.mac? && Helper.xcode_at_least?(14)
end

# Returns the password to be used with the transporter
Expand Down
10 changes: 5 additions & 5 deletions fastlane_core/spec/itunes_transporter_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -1157,14 +1157,14 @@ def xcrun_download_command(provider_short_name = nil, jwt: nil)
end
context "upload command generation" do
it 'generates a call to altool' do
transporter = FastlaneCore::ItunesTransporter.new(email, password, false, 'abcd123', upload: true)
transporter = FastlaneCore::ItunesTransporter.new(email, password, false, 'abcd123', altool_compatible_command: true)
expect(transporter.upload('my.app.id', '/tmp', package_path: '/tmp/my.app.id.itmsp', platform: "osx")).to eq(altool_upload_command(provider_short_name: 'abcd123'))
end
end

context "provider IDs command generation" do
it 'generates a call to altool' do
transporter = FastlaneCore::ItunesTransporter.new(email, password, false, 'abcd123', upload: true)
transporter = FastlaneCore::ItunesTransporter.new(email, password, false, 'abcd123', altool_compatible_command: true)
expect(transporter.provider_ids).to eq(altool_provider_id_command)
end
end
Expand All @@ -1177,7 +1177,7 @@ def xcrun_download_command(provider_short_name = nil, jwt: nil)
end
context "upload command generation" do
it 'generates a call to xcrun iTMSTransporter instead altool' do
transporter = FastlaneCore::ItunesTransporter.new(email, password, false, 'abcd123', upload: true)
transporter = FastlaneCore::ItunesTransporter.new(email, password, false, 'abcd123', altool_compatible_command: true)
expect(transporter.upload('my.app.id', '/tmp', platform: "osx")).to eq(java_upload_command(provider_short_name: 'abcd123', classpath: false))
end
end
Expand All @@ -1194,15 +1194,15 @@ def xcrun_download_command(provider_short_name = nil, jwt: nil)
end
context "upload command generation" do
it 'generates a call to altool' do
transporter = FastlaneCore::ItunesTransporter.new(email, password, false, 'abcd123', upload: true, api_key: api_key)
transporter = FastlaneCore::ItunesTransporter.new(email, password, false, 'abcd123', altool_compatible_command: true, api_key: api_key)
expected = Regexp.new("API_PRIVATE_KEYS_DIR=#{Regexp.escape(Dir.tmpdir)}.*\s#{Regexp.escape(altool_upload_command(api_key: api_key, provider_short_name: 'abcd123'))}")
expect(transporter.upload('my.app.id', '/tmp', platform: "osx")).to match(expected)
end
end

context "provider IDs command generation" do
it 'generates a call to altool' do
transporter = FastlaneCore::ItunesTransporter.new(email, password, false, 'abcd123', upload: true, api_key: api_key)
transporter = FastlaneCore::ItunesTransporter.new(email, password, false, 'abcd123', altool_compatible_command: true, api_key: api_key)
expected = Regexp.new("API_PRIVATE_KEYS_DIR=#{Regexp.escape(Dir.tmpdir)}.*\s#{Regexp.escape(altool_provider_id_command(api_key: api_key))}")
expect(transporter.provider_ids).to match(expected)
end
Expand Down
6 changes: 3 additions & 3 deletions pilot/lib/pilot/build_manager.rb
Original file line number Diff line number Diff line change
Expand Up @@ -401,13 +401,13 @@ def transporter_for_selected_team(options)

unless api_token.nil?
api_token.refresh! if api_token.expired?
return FastlaneCore::ItunesTransporter.new(nil, nil, false, nil, api_token.text, upload: true, api_key: api_key)
return FastlaneCore::ItunesTransporter.new(nil, nil, false, nil, api_token.text, altool_compatible_command: true, api_key: api_key)
end

# Otherwise use username and password
tunes_client = Spaceship::ConnectAPI.client ? Spaceship::ConnectAPI.client.tunes_client : nil

generic_transporter = FastlaneCore::ItunesTransporter.new(options[:username], nil, false, options[:itc_provider], upload: true, api_key: api_key)
generic_transporter = FastlaneCore::ItunesTransporter.new(options[:username], nil, false, options[:itc_provider], altool_compatible_command: true, api_key: api_key)
return generic_transporter if options[:itc_provider] || tunes_client.nil?
return generic_transporter unless tunes_client.teams.count > 1

Expand All @@ -416,7 +416,7 @@ def transporter_for_selected_team(options)
name = team['name']
provider_id = generic_transporter.provider_ids[name]
UI.verbose("Inferred provider id #{provider_id} for team #{name}.")
return FastlaneCore::ItunesTransporter.new(options[:username], nil, false, provider_id, upload: true, api_key: api_key)
return FastlaneCore::ItunesTransporter.new(options[:username], nil, false, provider_id, altool_compatible_command: true, api_key: api_key)
rescue => ex
STDERR.puts(ex.to_s)
UI.verbose("Couldn't infer a provider short name for team with id #{tunes_client.team_id} automatically: #{ex}. Proceeding without provider short name.")
Expand Down