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

[screengrab] Fix screenshots on Android Q and above #18434

Merged
merged 2 commits into from
Mar 25, 2021

Conversation

KartikSoneji
Copy link
Contributor

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

Trying to capture screenshots with screengrab on Android versions >= Android Q (v10, API version 29) fails on Windows.
Resolves #18335

Description

FastlaneCore::Helper.which uses the platform-specific path separator (/ on UNIX, \ on Windows).
Adds a new method, FastlaneCore::Helper.localize_file_path(path) that fixes the path separator.

Fixes the command used to extract the screenshots as described in the issue

Microsoft is including some UNIX commands like `tar` and `curl` in the standard install of Windows ([source](https://docs.microsoft.com/en-us/virtualization/community/team-blog/2017/20171219-tar-and-curl-come-to-windows)).
> where tar
C:\Windows\System32\tar.exe

The bundled tar is a version of bsdtar

> tar --version
bsdtar 3.3.2 - libarchive 3.3.2 zlib/1.2.5.f-ipp

Piping the output directly doesn't work

> type archive.tar | tar -xvC folder
tar: Error opening archive: Failed to open '\\.\tape0'
The process tried to write to a nonexistent pipe.

Fortunately, specifying the input file with -f- works

> type archive.tar | tar -f- -xvC folder

Testing Steps

On a Windows PC:

  • Create an Android Emulator running Android Q or higher
  • Setup Fastlane and Screengrab for an Android Project
  • Run fastlane screengrab for the project

@google-cla google-cla bot added the cla: yes label Mar 21, 2021
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 is a good fix! Thanks for catching this 😊 Just one small suggestion!

@@ -377,6 +377,8 @@ def self.executable?(cmd_path)

# returns the path of the executable with the correct extension on Windows
def self.get_executable_path(cmd_path)
cmd_path = localize_file_path(cmd_path)
Copy link
Member

Choose a reason for hiding this comment

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

Thoughts on removing this new method and just doing this 👇 😇 I tested this on macOS and Windows and it worked 🤷‍♂️ FILE::ALT_SEPARATOR is nil on Windows

Suggested change
cmd_path = localize_file_path(cmd_path)
cmd_path = cmd_path.gsub(File::SEPARATOR,File::ALT_SEPARATOR || File::SEPARATOR)

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 localize_file_path function is used multiple times, so I think you mean something like this?

diff --git a/fastlane_core/lib/fastlane_core/helper.rb b/fastlane_core/lib/fastlane_core/helper.rb
index 72ab5f36a..20f66d759 100644
--- a/fastlane_core/lib/fastlane_core/helper.rb
+++ b/fastlane_core/lib/fastlane_core/helper.rb
@@ -398,7 +398,7 @@ module FastlaneCore
     # returns the path with the platform-specific path separator (`/` on UNIX, `\` on Windows)
     def self.localize_file_path(path)
       # change `/` to `\` on Windows
-      return self.windows? ? path.gsub('/', '\\') : path
+      return path.gsub(File::SEPARATOR, File::ALT_SEPARATOR || File::SEPARATOR)
     end
 
     # checks if given file is a valid json file

But that would mean duplicating the setup for all the tests:

diff --git a/fastlane/spec/actions_specs/adb_spec.rb b/fastlane/spec/actions_specs/adb_spec.rb
index c148b8cae..b2dfdb7a8 100644
--- a/fastlane/spec/actions_specs/adb_spec.rb
+++ b/fastlane/spec/actions_specs/adb_spec.rb
@@ -3,6 +3,7 @@ describe Fastlane do
     describe "adb" do
       before(:each) do
         allow(FastlaneCore::Helper).to receive(:windows?).and_return(false)
+        allow(File).to receive(:ALT_SEPARATOR).and_return(nil)
       end
 
       it "generates a valid command" do
@@ -78,6 +79,7 @@ describe Fastlane do
     describe "adb on Windows" do
       before(:each) do
         allow(FastlaneCore::Helper).to receive(:windows?).and_return(true)
+        allow(File).to receive(:ALT_SEPARATOR).and_return('\\')
       end
 
       it "generates a valid command" do

Copy link
Member

Choose a reason for hiding this comment

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

@KartikSoneji Oh shoot, it is used multiple times! 😱 I thought I only saw it used once which is why I suggested what I did. My bad 🤦‍♂️

Hmmmm... I don’t like those changes to the tests 😔

I think what you had originally is okay 😊 We’ll go with that! Thanks for looking into!

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.

Turns out this is good and I was crazy during my first review 😛 Really appreciate the contribution! Thank you ❤️

@joshdholtz joshdholtz merged commit f6d685e into fastlane:master Mar 25, 2021
@KartikSoneji KartikSoneji deleted the fix-screenshot-android-q branch March 25, 2021 12:00
@fastlane-bot
Copy link

Hey @KartikSoneji 👋

Thank you for your contribution to fastlane and congrats on getting this pull request merged 🎉
The code change now lives in the master branch, however it wasn't released to RubyGems yet.
We usually ship about once a week, and your PR will be included in the next one.

Please let us know if this change requires an immediate release by adding a comment here 👍
We'll notify you once we shipped a new release with your changes 🚀

@fastlane-bot
Copy link

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

@penn5
Copy link
Contributor

penn5 commented Apr 20, 2021

Many devices don't have tar installed. There are also large issues with the current Android-side helper implementation across various android versions. Please see #18003 where I fixed (some of?) these. It looks like my PR fell through some cracks and just didn't get reviewed. :(

@fastlane fastlane locked and limited conversation to collaborators Jun 20, 2021
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.

[screengrab] Screenshots fail on Android versions >= Q on Windows
4 participants