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.

Sign up for GitHub

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

Jump to bottom

Improve how App and Ipa instances detect the Calabash server version #379

Merged
merged 18 commits into from
Feb 3, 2016
Merged

Improve how App and Ipa instances detect the Calabash server version #379

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
713108e
Dylibs: update calabash dylibs to 0.17.1
jmoody Jan 20, 2016
e91b183
App: implement .valid?(app_bundle_path)
jmoody Jan 22, 2016
daf2e76
App: .new raises error if bundle path is invalid
jmoody Jan 22, 2016
3576714
App: internal calls to valid? are no longer necessary
jmoody Jan 22, 2016
aa5afa6
App: add integration tests
jmoody Jan 22, 2016
0f80da3
Add Otool class for interacting with otool
jmoody Jan 22, 2016
5442d81
App: can return a list of executables
jmoody Jan 29, 2016
21695fa
App: optimize executable check by filtering files
jmoody Jan 29, 2016
d5f2fc7
Strings: a class for interfacing with Xcode strings tool
jmoody Jan 29, 2016
e0e7116
Jenkins: run more integrations
jmoody Jan 29, 2016
b7ba49d
App: use Strings class and #executables to find version
jmoody Jan 29, 2016
f88d387
Ipa: use an internal app instance
jmoody Jan 29, 2016
4683a07
Ipa: fixup style and docs
jmoody Jan 29, 2016
c80f86c
TODO: should not be part of the repo
jmoody Jan 29, 2016
843373f
App: skip exec check on [Assets].car files
jmoody Feb 1, 2016
26f030b
App: skip CoreData and .db files during exec check
jmoody Feb 1, 2016
54883b3
App: filter iTunesArtwork from exec list
jmoody Feb 3, 2016
d865426
App: filter common text files
jmoody Feb 3, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions lib/run_loop.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@
require 'run_loop/device'
require 'run_loop/instruments'
require 'run_loop/lipo'
require "run_loop/otool"
require "run_loop/strings"
require 'run_loop/cache/cache'
require 'run_loop/host_cache'
require 'run_loop/patches/awesome_print'
Expand Down
144 changes: 124 additions & 20 deletions lib/run_loop/app.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,20 @@ class App
# @return [RunLoop::App] A instance of App with a path.
def initialize(app_bundle_path)
@path = File.expand_path(app_bundle_path)

if !App.valid?(app_bundle_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.

Immediately raise an error if the app bundle is invalid. This avoids having to check in each instance method.

raise ArgumentError,
%Q{App does not exist at path or is not an app bundle.

#{app_bundle_path}

Bundle must:

1. be a directory that exists,
2. have a .app extension,
3. and contain an Info.plist.
}
end
end

# @!visibility private
Expand All @@ -28,19 +42,23 @@ def inspect

# Is this a valid app?
def valid?
[File.exist?(path),
File.directory?(path),
File.extname(path) == '.app'].all?
App.valid?(path)
end

# @!visibility private
def self.valid?(app_bundle_path)
return false if app_bundle_path.nil?

File.exist?(app_bundle_path) &&
File.directory?(app_bundle_path) &&
File.extname(app_bundle_path) == '.app' &&
File.exist?(File.join(app_bundle_path, "Info.plist"))
end

# Returns the Info.plist path.
# @raise [RuntimeError] If there is no Info.plist.
def info_plist_path
info_plist = File.join(path, 'Info.plist')
unless File.exist?(info_plist)
raise "Expected an Info.plist at '#{path}'"
end
info_plist
@info_plist_path ||= File.join(path, 'Info.plist')
end

# Inspects the app's Info.plist for the bundle identifier.
Expand Down Expand Up @@ -69,20 +87,26 @@ def executable_name

# Inspects the app's file for the server version
def calabash_server_version
if valid?
path_to_bin = File.join(path, executable_name)
xcrun ||= RunLoop::Xcrun.new
hash = xcrun.exec(["strings", path_to_bin])
unless hash.nil?
version_str = hash[:out][/CALABASH VERSION: \d+\.\d+\.\d+/, 0]
unless version_str.nil? || version_str == ""
server_ver = version_str.split(":")[1].delete(' ')
RunLoop::Version.new(server_ver)
end
version = nil
executables.each do |executable|
version = strings(executable).server_version
break if version
end
version
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Handles the load-calabash-dylib-at-runtime example.

Collect the executable files and use strings to check for the Calabash server version.

Copy link

Choose a reason for hiding this comment

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

Is this a recursive search of executables? I'm not really sure where the executables list is coming from...

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 am using otool to find executable files in the bundle. It is a recursive search over all the files in the bundle. There is a filter that skips the obvious non-executable files.

It is hard to see the code through the comments.

+    # @!visibility private
+    # Collects the paths to executables in the bundle.
+    def executables
+      executables = []
+      Dir.glob("#{path}/**/*") do |file|
+        next if File.directory?(file)
+        next if skip_executable_check?(file)

Copy link

Choose a reason for hiding this comment

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

Is that recursive or just two levels down? I thought recursive glob syntax was dir/*** ?

Copy link

Choose a reason for hiding this comment

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

I think I'm wrong about that, nvm.

Copy link

Choose a reason for hiding this comment

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

Wait, not completely wrong - path/**/* isn't completely recursive. Though I guess it doesn't really make sense to recurse as far down as app extensions since those shouldn't really matter for validating whether calabash is linked.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

path/**/* is recursive.

> Dir.glob("spec/resources/CalSmoke.app/**/*").count
280

# The extra 1 is the last newline
$ find spec/resources/CalSmoke.app | wc -l
     281

$ tree spec/resources/CalSmoke.app
27 directories, 253 files  # 280

end

# @!visibility private
# Collects the paths to executables in the bundle.
def executables
executables = []
Dir.glob("#{path}/**/*") do |file|
next if File.directory?(file)
next if skip_executable_check?(file)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

An optimization.

Calling otool on each file took ~2.0 seconds for a small bundle. After applying this filter, it took ~0.09 seconds.

if otool(file).executable?
executables << file
end
else
raise 'Path is not valid'
end
executables
end

# Returns the sha1 of the application.
Expand All @@ -92,9 +116,89 @@ def sha1

private

# @!visibility private
def plist_buddy
@plist_buddy ||= RunLoop::PlistBuddy.new
end

# @!visibility private
# An otool factory.
def otool(file)
RunLoop::Otool.new(file)
end

# @!visibility private
# A strings factory
def strings(file)
RunLoop::Strings.new(file)
end

# @!visibility private
def skip_executable_check?(file)
image?(file) ||
text?(file) ||
plist?(file) ||
lproj_asset?(file) ||
code_signing_asset?(file) ||
core_data_asset?(file)
Copy link

Choose a reason for hiding this comment

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

@jmoody Why not use a whitelist approach instead? Files that need to be checked for calabash linkage should be just the main app binary and any dylibs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

whitelist

I thought about pretty seriously. I am tempted to use a whitelist.

I was thinking about extensions and a future where we would automate those as well - but the same whitelist rules would apply.

I think I will change the approach now to use whitelist.

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.

Could use a second pair of eyes on this. Anything else I should skip?

Ack! CoreData bundles!!!

Copy link

Choose a reason for hiding this comment

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

Why not just check the main executable + dylibs ?


# @!visibility private
def text?(file)
extension = File.extname(file)

extension == ".txt" ||
extension == ".md" ||
extension == ".html" ||
extension == ".xml" ||
extension == ".json" ||
extension == ".yaml" ||
extension == ".yml" ||
extension == ".rtf" ||
file[/NOTICE|LICENSE|README|ABOUT/, 0]
end

# @!visibility private
def image?(file)
file[/jpeg|jpg|gif|png|tiff|svg|pdf|car|iTunesArtwork/, 0]
end

# @!visibility private
def plist?(file)
File.extname(file) == ".plist"
end

# @!visibility private
def lproj_asset?(file)
extension = File.extname(file)

file[/lproj/, 0] ||
file[/storyboard/, 0] ||
extension == ".strings" ||
extension == ".xib" ||
extension == ".nib"
end

# @!visibility private
def code_signing_asset?(file)
name = File.basename(file)
extension = File.extname(file)

name == "PkgInfo" ||
name == "embedded" ||
extension == ".mobileprovision" ||
extension == ".xcent" ||
file[/_CodeSignature/, 0]
end

# @!visibility private
def core_data_asset?(file)
extension = File.extname(file)

file[/momd/, 0] ||
extension == ".mom" ||
extension == ".db"
end
end
end

88 changes: 22 additions & 66 deletions lib/run_loop/ipa.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,18 +2,11 @@ module RunLoop
# A model of the an .ipa - a application binary for iOS devices.
class Ipa


# The path to this .ipa.
# @!attribute [r] path
# @return [String] A path to this .ipa.
attr_reader :path

# The bundle identifier of this ipa.
# @!attribute [r] bundle_identifier
# @return [String] The bundle identifier of this ipa; obtained by inspecting
# the app's Info.plist.
attr_reader :bundle_identifier

# Create a new ipa instance.
# @param [String] path_to_ipa The path the .ipa file.
# @return [Calabash::Ipa] A new ipa instance.
Expand Down Expand Up @@ -42,96 +35,59 @@ def inspect

# The bundle identifier of this ipa.
# @return [String] A string representation of this ipa's CFBundleIdentifier
# @raise [RuntimeError] If ipa does not expand into a Payload/<app name>.app
# directory.
# @raise [RuntimeError] If an Info.plist does exist in the .app.
def bundle_identifier
if bundle_dir.nil? || !File.exist?(bundle_dir)
raise "Expected a '#{File.basename(path).split('.').first}.app'\nat path '#{payload_dir}'"
end

@bundle_identifier ||= lambda {
info_plist_path = File.join(bundle_dir, 'Info.plist')
unless File.exist? info_plist_path
raise "Expected an 'Info.plist' at '#{bundle_dir}'"
end
identifier = plist_buddy.plist_read('CFBundleIdentifier', info_plist_path)

unless identifier
raise "Expected key 'CFBundleIdentifier' in '#{info_plist_path}'"
end
identifier
}.call
app.bundle_identifier
end

# Inspects the app's Info.plist for the executable name.
# @return [String] The value of CFBundleExecutable.
# @raise [RuntimeError] If the plist cannot be read or the
# CFBundleExecutable is empty or does not exist.
def executable_name
if bundle_dir.nil? || !File.exist?(bundle_dir)
raise "Expected a '#{File.basename(path).split('.').first}.app'\nat path '#{payload_dir}'"
end

@executable_name ||= lambda {
info_plist_path = File.join(bundle_dir, 'Info.plist')
unless File.exist? info_plist_path
raise "Expected an 'Info.plist' at '#{bundle_dir}'"
end
name = plist_buddy.plist_read('CFBundleExecutable', info_plist_path)

unless name
raise "Expected key 'CFBundleExecutable' in '#{info_plist_path}'"
end
name
}.call
app.executable_name
end

# Inspects the app's file for the server version
# Inspects the app's executables for the server version
# @return[RunLoop::Version] a version instance
def calabash_server_version
if bundle_dir.nil? || !File.exist?(bundle_dir)
raise "Expected a '#{File.basename(path).split('.').first}.app'\nat path '#{payload_dir}'"
else
if !executable_name.nil? && executable_name != ''
path_to_bin = File.join(bundle_dir, executable_name)
xcrun ||= RunLoop::Xcrun.new
hash = xcrun.exec(["strings", path_to_bin])
unless hash.nil?
version_str = hash[:out][/CALABASH VERSION: \d+\.\d+\.\d+/, 0]
unless version_str.nil? || version_str == ""
server_ver = version_str.split(":")[1].delete(' ')
RunLoop::Version.new(server_ver)
end
end
end
end
app.calabash_server_version
end

private

# @!visibility private
def tmpdir
@tmpdir ||= Dir.mktmpdir
end

# @!visibility private
def payload_dir
@payload_dir ||= lambda {
@payload_dir ||= lambda do
FileUtils.cp(path, tmpdir)
zip_path = File.join(tmpdir, File.basename(path))
Dir.chdir(tmpdir) do
system('unzip', *['-q', zip_path])
Copy link

Choose a reason for hiding this comment

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

I thought we were supposed to use ditto ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For this operation we are copying a zip file (.ipa) to a temp location.

It is safe, I think, to copy with cp. Who knows, maybe FileUtils.cp is implemented with ditto on MacOS.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To clarify, we should use ditto when copying .app (app bundles) directories. I don't think we need to use ditto when copying archive files because the resource forks are included in the archive.

Copy link

Choose a reason for hiding this comment

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

I was talking about the unzip, not the cp

Copy link

Choose a reason for hiding this comment

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

E.g., for resigning we do:

    // NOTE: To correctly preserve MACOS metadata we must use ditto instead of zip.
    // Failing to do so can make codesign bail out with some very exotic
    // error messages in some rare cases.
    ShellOrDie("ditto", "-x", "-k", "--sequesterRsrc", filepath, tmpDir)

Perhaps it's not relevant here, but I thought it was just generally safer

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(>_O) Of course you were.

I will fix this.

end
File.join(tmpdir, 'Payload')
}.call
end.call
end

# @!visibility private
def bundle_dir
@bundle_dir ||= lambda {
Dir.glob(File.join(payload_dir, '*')).detect {|f| File.directory?(f) && f.end_with?('.app')}
}.call
@bundle_dir ||= lambda do
Dir.glob(File.join(payload_dir, '*')).detect do |f|
File.directory?(f) && f.end_with?('.app')
end
end.call
end

# @!visibility private
def app
@app ||= RunLoop::App.new(bundle_dir)
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.

🎱

We should have been doing this from the beginning.

The Payload/My.app is just an app bundle - use the App class so we don't have to duplicate code.


# @!visibility private
def plist_buddy
@plist_buddy ||= RunLoop::PlistBuddy.new
end
end
end

Loading