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

jmoody · 2016-01-29T15:40:24Z

Motivation

calabash-ios check gives false negative #945
App does not contain Calabash framework (argument list too long) #718
Integration examples need attention Integration examples need attention #243

The previous implementation (thanks @ark-konopacki) had a couple of problems:

It did not detect calabash dylibs - the load-dylib-at-runtime case
It did not correctly extract pre release versions

This PR is a bit long and I am sorry for that. I will annotate the changes with notes to help the reviewer.

Related to (private links):

test-cloud gem
Check for CalabashServer when uploading
run-loop: implement check for app-is-linked with calabash.framework or dylib exists in bundle #50

Add the fat dylib for testing

The #valid? method calls this new class method. I want to move the valid check into the initializer to: * fail fast * remove the need to call #valid? in instance methods

App is valid because the check in initializer

1.9 seconds => 0.09 seconds

Don't bother with all the validation in individual methods; just create an .app instance from the Payload/App.app bundle

jmoody · 2016-01-29T15:41:34Z

lib/run_loop/app.rb

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


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

jmoody · 2016-01-29T15:52:38Z

No hurry on this @sapieneptus. It does not have to be part of the next release.

sapieneptus · 2016-02-01T16:28:33Z

lib/run_loop/ipa.rb

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


I thought we were supposed to use ditto ?

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.

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.

I was talking about the unzip, not the cp

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

(>_O) Of course you were.

I will fix this.

sapieneptus · 2016-02-01T17:07:01Z

lib/run_loop/app.rb

+        plist?(file) ||
+        lproj_asset?(file) ||
+        code_signing_asset?(file) ||
+        core_data_asset?(file)


@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

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.

sapieneptus · 2016-02-01T17:11:49Z

@jmoody The comments I have made are non-blocking. I don't think cp vs ditto will cause any problems here (at least I don't have any indication that they would cause problems outside of a codesigning context). I do think it may be worth considering a whitelist approach for skip_executable_check, but it seems you've already gotten substantial performance gains.

I don't have enough context to say whether or not this will aversely impact the rest of RunLoop, but I don't see anything obviously wrong with the PR and the tests are passing. So with that in mind, I would say it's good to go.

jmoody · 2016-02-03T16:36:23Z

@sapieneptus

I thought more about using a whitelist and had a brief conversation with Simon.

I don't think we actually can make a whitelist that would cover all cases.

jmoody · 2016-02-03T16:59:36Z

Grr. Travis is failing because it can't install ruby 2.1.7.

Merging. Will deal with this failure if it occurs on develop.

…version-take-2 Improve how App and Ipa instances detect the Calabash server version

sapieneptus · 2016-02-03T18:56:40Z

@jmoody Sorry to keep going back and forth on this, but I thought we agreed that whitelisting by executable binary would be sufficient? I'm pretty sure we can use file and regex for /Mach-O * executable/

jmoody · 2016-02-04T12:52:50Z

back and forth

No worries. Makes the code better. +1

whitelist

I thought you were arguing for a whitelist with these rules:

test the CFBundleExcutable (the app binary)
any dylib

file

I looked at file and was not convinced it was the right tool. otool seems definitive.

executable binary

This is the behavior. A list of executables is created and strings is call on the elements of that list to detect the Calabash iOS server version.

jmoody added 14 commits January 26, 2016 14:53

Dylibs: update calabash dylibs to 0.17.1

713108e

Add the fat dylib for testing

App: implement .valid?(app_bundle_path)

e91b183

The #valid? method calls this new class method. I want to move the valid check into the initializer to: * fail fast * remove the need to call #valid? in instance methods

App: .new raises error if bundle path is invalid

daf2e76

App: internal calls to valid? are no longer necessary

3576714

App is valid because the check in initializer

App: add integration tests

aa5afa6

Add Otool class for interacting with otool

0f80da3

App: can return a list of executables

5442d81

App: optimize executable check by filtering files

21695fa

1.9 seconds => 0.09 seconds

Strings: a class for interfacing with Xcode strings tool

d5f2fc7

Jenkins: run more integrations

e0e7116

App: use Strings class and #executables to find version

b7ba49d

Ipa: use an internal app instance

f88d387

Don't bother with all the validation in individual methods; just create an .app instance from the Payload/App.app bundle

Ipa: fixup style and docs

4683a07

TODO: should not be part of the repo

c80f86c

jmoody reviewed Jan 29, 2016
View reviewed changes

jmoody added the enhancement label Jan 29, 2016

jmoody modified the milestones: 2.0.5, 2.0.6 Jan 29, 2016

jmoody assigned sapieneptus Jan 29, 2016

jmoody mentioned this pull request Feb 1, 2016

SimLauncher: fix autodetection of app bundle calabash/calabash-ios#974

Merged

App: skip exec check on [Assets].car files

843373f

sapieneptus reviewed Feb 1, 2016
View reviewed changes

App: skip CoreData and .db files during exec check

26f030b

sapieneptus reviewed Feb 1, 2016
View reviewed changes

jmoody assigned jmoody and unassigned sapieneptus Feb 1, 2016

jmoody added 2 commits February 3, 2016 16:57

App: filter iTunesArtwork from exec list

54883b3

App: filter common text files

d865426

jmoody added a commit that referenced this pull request Feb 3, 2016

Merge pull request #379 from calabash/feature/detect-calabash-server-…

f5d0367

…version-take-2 Improve how App and Ipa instances detect the Calabash server version

jmoody merged commit f5d0367 into develop Feb 3, 2016

jmoody deleted the feature/detect-calabash-server-version-take-2 branch February 3, 2016 16:59

jmoody mentioned this pull request Feb 5, 2016

Release/2.0.6 #383

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

jmoody commented Jan 29, 2016

jmoody Jan 29, 2016

jmoody commented Jan 29, 2016

sapieneptus Feb 1, 2016

jmoody Feb 1, 2016

jmoody Feb 1, 2016

sapieneptus Feb 1, 2016

sapieneptus Feb 1, 2016

jmoody Feb 1, 2016

sapieneptus Feb 1, 2016

jmoody Feb 1, 2016

sapieneptus commented Feb 1, 2016

jmoody commented Feb 3, 2016

jmoody commented Feb 3, 2016

sapieneptus commented Feb 3, 2016

jmoody commented Feb 4, 2016

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

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

Conversation

jmoody commented Jan 29, 2016

Motivation

Choose a reason for hiding this comment

jmoody commented Jan 29, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sapieneptus commented Feb 1, 2016

jmoody commented Feb 3, 2016

jmoody commented Feb 3, 2016

sapieneptus commented Feb 3, 2016

jmoody commented Feb 4, 2016