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

[supply] introduce a new synchronization logic for screenshots #21498

Merged
merged 7 commits into from
Sep 15, 2023
Merged
Show file tree
Hide file tree
Changes from 4 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
1 change: 1 addition & 0 deletions supply/lib/supply.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
require 'supply/client'
require 'supply/listing'
require 'supply/apk_listing'
require 'supply/image_listing'
require 'supply/generated_universal_apk'
require 'supply/release_listing'
require 'supply/uploader'
Expand Down
59 changes: 39 additions & 20 deletions supply/lib/supply/client.rb
Original file line number Diff line number Diff line change
Expand Up @@ -532,6 +532,12 @@ def update_obb(apk_version_code, expansion_file_type, references_version, file_s
# @!group Screenshots
#####################################################

# Fetch all the images of a given type and for a given language of your store listing
#
# @param [String] image_type Typically one of the elements of either Supply::IMAGES_TYPES or Supply::SCREENSHOT_TYPES
# @param [String] language Localization code (a BCP-47 language tag; for example, "de-AT" for Austrian German).
# @return [Array<ImageListing>] A list of ImageListing instances describing each image with its id, sha256 and url
#
def fetch_images(image_type: nil, language: nil)
ensure_active_edit!

Expand All @@ -544,29 +550,17 @@ def fetch_images(image_type: nil, language: nil)
)
end

urls = (result.images || []).map(&:url)
images = urls.map do |url|
uri = URI.parse(url)
clean_url = [
uri.scheme,
uri.userinfo,
uri.host,
uri.port,
uri.path
].join

UI.verbose("Initial URL received: '#{url}'")
UI.verbose("Removed params ('#{uri.query}') from the URL")
UI.verbose("URL after removing params: '#{clean_url}'")
Comment on lines -549 to -560
Copy link
Contributor Author

Choose a reason for hiding this comment

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

All those lines were dead code even before my change: as you can see, clean_url was never used in the returned value after all.


full_url = "#{url}=s0" # '=s0' param ensures full image size is returned (https://github.com/fastlane/fastlane/pull/14322#issuecomment-473012462)
full_url
(result.images || []).map do |row|
full_url = "#{row.url}=s0" # '=s0' param ensures full image size is returned (https://github.com/fastlane/fastlane/pull/14322#issuecomment-473012462)
ImageListing.new(row.id, row.sha1, row.sha256, full_url)
Copy link
Member

Choose a reason for hiding this comment

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

Really nice find to see it has all this info (sha256, etc) 🔥

end

return images
end

# @param image_type (e.g. phoneScreenshots, sevenInchScreenshots, ...)
# Upload an image or screenshot of a specific type for a given language to your store listing
#
# @param [String] image_type (e.g. phoneScreenshots, sevenInchScreenshots, ...)
# @param [String] language localization code (i.e. BCP-47 language tag as in `pt-BR`)
Copy link
Member

Choose a reason for hiding this comment

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

🇧🇷 🥳

#
def upload_image(image_path: nil, image_type: nil, language: nil)
ensure_active_edit!

Expand All @@ -582,6 +576,11 @@ def upload_image(image_path: nil, image_type: nil, language: nil)
end
end

# Remove all screenshots of a given image_type and language from the store listing
#
# @param [String] image_type (e.g. phoneScreenshots, sevenInchScreenshots, ...)
# @param [String] language localization code (i.e. BCP-47 language tag as in `pt-BR`)
#
def clear_screenshots(image_type: nil, language: nil)
ensure_active_edit!

Expand All @@ -595,6 +594,26 @@ def clear_screenshots(image_type: nil, language: nil)
end
end

# Remove a specific screenshot of a given image_type and language from the store listing
#
# @param [String] image_type (e.g. phoneScreenshots, sevenInchScreenshots, ...)
# @param [String] language localization code (i.e. BCP-47 language tag as in `pt-BR`)
# @param [String] image_id The id of the screenshot to remove (as per `ImageListing#id`)
#
def clear_screenshot(image_type: nil, language: nil, image_id: nil)
ensure_active_edit!

call_google_api do
client.delete_edit_image(
current_package_name,
current_edit.id,
language,
image_type,
image_id
)
end
end

def upload_obb(obb_file_path: nil, apk_version_code: nil, expansion_file_type: nil)
ensure_active_edit!

Expand Down
15 changes: 15 additions & 0 deletions supply/lib/supply/image_listing.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
module Supply
class ImageListing
attr_reader :id
attr_reader :sha1
attr_reader :sha256
attr_reader :url

def initialize(id, sha1, sha256, url)
@id = id
@sha1 = sha1
@sha256 = sha256
@url = url
end
end
end
6 changes: 6 additions & 0 deletions supply/lib/supply/options.rb
Original file line number Diff line number Diff line change
Expand Up @@ -210,6 +210,12 @@ def self.available_options
description: "Whether to skip uploading SCREENSHOTS",
type: Boolean,
default_value: false),
FastlaneCore::ConfigItem.new(key: :sync_image_upload,
Copy link
Member

Choose a reason for hiding this comment

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

Should we give it same name as deliver's option name (and a similar name to the env var)? I think that would make the API more familiar to devs that maintain fastlane for both platforms, or are just curious reading the Fastfile, etc. That'd make this sync_screenshots and SUPPLY_SYNC_SCREENSHOTS

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason I didn't name it sync_screenshots is deliberate: this is because this not only applies to screenshots, but also to Play Store artwork — typically the featuredGraphic and icon images that are not screenshots but rather the banner and large icon shown in the Google Play's download page for the app.

I thought calling it sync_screenshots would make people think that this only applied to screenshots (as in skip_upload_screenshots: false) while it also applies to skip_upload_images: false too.
Arguably, because the app artwork is referenced by the term …images… in the skip_upload_images option, one could argue that calling my new parameter sync_image_upload would make people think that it applies to that option only (i.e. where image term here would be interpreted as "the artwork images uploaded as part of skip_upload_images: false), while the intent of the term …image… in sync_image_upload here is more to say "sync the upload of every kind of [PNG, JPEG, …] image file".

But I couldn't find a term that would encompass both "each image file, be it a screenshot or an artwork image" other than just the term "image", hence settling for this.


As for trying to make the deliver and supply option names in sync, that ship as sadly already sailed: For example, deliver uses skip_screenshots while supply uses skip_upload_screenshots (and skip_upload_images), with _upload_ in that option name for supply while deliver doesn't have it 🤷

Copy link
Member

Choose a reason for hiding this comment

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

Good points! 😅 I think you named it well 😊 Thanks for clarifying!

env_name: "SUPPLY_SYNC_IMAGE_UPLOAD",
optional: true,
AliSoftware marked this conversation as resolved.
Show resolved Hide resolved
description: "Weather to use sha256 comparison to skip upload of images and screenshots that are already in Play Store",
AliSoftware marked this conversation as resolved.
Show resolved Hide resolved
type: Boolean,
default_value: false),
FastlaneCore::ConfigItem.new(key: :track_promote_to,
env_name: "SUPPLY_TRACK_PROMOTE_TO",
optional: true,
Expand Down
2 changes: 1 addition & 1 deletion supply/lib/supply/setup.rb
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ def download_images(listing)

UI.message("Downloading `#{image_type}` for #{listing.language}...")

urls = client.fetch_images(image_type: image_type, language: listing.language)
urls = client.fetch_images(image_type: image_type, language: listing.language).map(&:url)
next if urls.nil? || urls.empty?

image_counter = 1 # Used to prefix the downloaded files, so order is preserved.
Expand Down
37 changes: 32 additions & 5 deletions supply/lib/supply/uploader.rb
Original file line number Diff line number Diff line change
Expand Up @@ -270,7 +270,17 @@ def upload_images(language)
path = Dir.glob(search, File::FNM_CASEFOLD).last
next unless path

UI.message("Uploading image file #{path}...")
if Supply.config[:sync_image_upload]
UI.message("🔍 Checking #{image_type} checksum...")
Copy link
Member

Choose a reason for hiding this comment

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

I wouldn't call it a checksum I think. The term is usually used to check whether the fetched is what the sender sent (checking data integrity, so see if it didn't get corrupted during the transfer, or altered by a middleman, etc). Here I'd probably just call "Checking […] hash…" or "Comparing existing image with remote one…" or something like that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair point.

I've already merged the PR though (in the hope that we'd include this PR in the new release that @joshdholtz said he'd do later today to fix the Google API random errors as we talked about in Slack), so it's a bit too late 😅.
Not sure if it's worth opening a subsequent PR just to fix this wording? (but if you feel strongly about it I don't mind us doing so to fix it)

Copy link
Member

Choose a reason for hiding this comment

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

Either way works for me! Not strongly opinionated, just think it'd be more befitting for what's actually happening :) I'll leave this up to you! If you decide to open the PR, tag me to review and I'll approve and merge it asap 😊

existing_images = client.fetch_images(image_type: image_type, language: language)
sha256 = Digest::SHA256.file(path).hexdigest
if existing_images.map(&:sha256).include?(sha256)
UI.message("🟰 Skipping upload of screenshot #{path} as remote sha256 matches.")
next
end
end

UI.message("⬆️ Uploading image file #{path}...")
client.upload_image(image_path: File.expand_path(path),
image_type: image_type,
language: language)
Expand All @@ -280,13 +290,30 @@ def upload_images(language)
def upload_screenshots(language)
Supply::SCREENSHOT_TYPES.each do |screenshot_type|
search = File.join(metadata_path, language, Supply::IMAGES_FOLDER_NAME, screenshot_type, "*.#{IMAGE_FILE_EXTENSIONS}")
paths = Dir.glob(search, File::FNM_CASEFOLD)
paths = Dir.glob(search, File::FNM_CASEFOLD).sort
next unless paths.count > 0

client.clear_screenshots(image_type: screenshot_type, language: language)
if Supply.config[:sync_image_upload]
UI.message("🔍 Checking #{screenshot_type} checksums...")
existing_images = client.fetch_images(image_type: screenshot_type, language: language)
# Don't keep images that either don't exist locally, or that are out of order compared to the `paths` to upload
first_path_checksum = Digest::SHA256.file(paths.first).hexdigest
existing_images.each do |image|
if image.sha256 == first_path_checksum
UI.message("🟰 Skipping upload of screenshot #{paths.first} as remote sha256 matches.")
paths.shift # Remove first path from the list of paths to be uploaded
first_path_checksum = paths.empty? ? nil : Digest::SHA256.file(paths.first).hexdigest
else
UI.message("🚮 Deleting #{language} screenshot id ##{image.id} as it does not exist locally or is out of order...")
client.clear_screenshot(image_type: screenshot_type, language: language, image_id: image.id)
end
end
Comment on lines +301 to +310
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sadly, there doesn't seem to be an Google API on AndroidPublisherService that allows to just reorder the screenshots, hence the need to delete the screenshots that are out of order and re-upload them, so that when new uploaded screenshots go at the end of the list, the final order is still correct in Google Play.

This is why I only compare with the checksum of the next local file (well, in practice, checksum of paths.first, while paths items are being shift'ed and removed from the paths array as they get skipped), instead of using something like all_local_checksums.include?(image.sha256) to just check if the remote image matches an existing local image, which would not guarantee that we'd end up with the expected order of the screenshots in the end.

else
client.clear_screenshots(image_type: screenshot_type, language: language) unless Supply.config[:sync_image_upload]
AliSoftware marked this conversation as resolved.
Show resolved Hide resolved
end

paths.sort.each do |path|
UI.message("Uploading screenshot #{path}...")
paths.each do |path|
UI.message("⬆️ Uploading screenshot #{path}...")
client.upload_image(image_path: File.expand_path(path),
image_type: screenshot_type,
language: language)
Expand Down
1 change: 1 addition & 0 deletions supply/spec/client_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@
expect(subject.class.method_defined?(:list_edit_images)).to eq(true)
expect(subject.class.method_defined?(:upload_edit_image)).to eq(true)
expect(subject.class.method_defined?(:deleteall_edit_image)).to eq(true)
expect(subject.class.method_defined?(:delete_edit_image)).to eq(true)
expect(subject.class.method_defined?(:upload_edit_expansionfile)).to eq(true)
expect(subject.class.method_defined?(:uploadapk_internalappsharingartifact)).to eq(true)
expect(subject.class.method_defined?(:uploadbundle_internalappsharingartifact)).to eq(true)
Expand Down