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

SimLauncher: fix autodetection of app bundle #974

Merged

Conversation

ark-konopacki
Copy link
Contributor

Fix and improvement in searching mechanism
it can be used as a draft for future implementations in run_loop

0.17.0 Cannot automatically detect app #959

0.17.0 Cannot automatically detect app calabash#959
@calabash-ci
Copy link

Can one of the admins verify this patch?

@jmoody
Copy link
Contributor

jmoody commented Jan 6, 2016

Jenkins test this please.

@jmoody jmoody added this to the 0.17.1 milestone Jan 8, 2016
@calabash-ci
Copy link

Can one of the admins verify this patch?

@ark-konopacki
Copy link
Contributor Author

@jmoody: Can you please review my changes, it looks ok for me now ;)

sim_dirs = Dir.glob(File.join(bo_dir, '*-iphonesimulator', '*.app'))
search_dir = build_output_dir_for_project || DERIVED_DATA
sim_dirs = ''
apps = `find #{search_dir} -name "*.app" | sort -n`.split("\n")
Copy link
Contributor

Choose a reason for hiding this comment

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

@ark-konopacki

Interesting! Why sort numerically (-n)?

We want to search for the modification time and take the newest.

find #{search_dir} -type d -name "*.app" -exec stat -f "%m %N" {} \; | sort -rn | cut -d" " -f2

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jmoody my fault will fix it

Copy link
Contributor

Choose a reason for hiding this comment

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

Be sure to search for directories; it will be faster - -type d.

sim_dirs = Dir.glob(app_path)
end
end
end
Copy link
Contributor

Choose a reason for hiding this comment

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

In the worst case, this looks like it will iterate over all apps that found and then sim_dirs will be the last app that matches the criteria and not the first.

I think we want to use: apps.find do instead of apps.each

sim_dirs = apps.find do |app_path|

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.

@jmoody: maybe it can be done like by adding break after
sim_dirs = Dir.glob(app_path)

@jmoody jmoody modified the milestones: 0.17.2, 0.17.1 Jan 13, 2016
@jmoody jmoody modified the milestones: 0.18.0, 0.17.2 Jan 22, 2016
@jmoody
Copy link
Contributor

jmoody commented Feb 1, 2016

@ark-konopacki This is a patch. I think we will need to come back to this later once some changes have been released in run-loop.

sim_dirs = Dir.glob(File.join(bo_dir, '*-iphonesimulator', '*.app'))
search_dir = build_output_dir_for_project || DERIVED_DATA
sim_dirs = ''
apps = `find #{search_dir} -type d -name "*.app" -exec stat -f "%m %N" {} \\; | sort -rn | cut -d" " -f2`.split("\n")
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we need to be careful of UTF-8 chars here.

We can use ruby's Find or Dir.glob + File.mtime to perform this operation.

@jmoody jmoody modified the milestones: 0.18.1, 0.18.0 Feb 1, 2016
@ark-konopacki
Copy link
Contributor Author

@jmoody thanks for feedback, after calabash/run_loop#379 will be merged and released i propose to rewrite that patch with more clear and simple implementation.

…to feature/fix-autodetection-of-app-bundle
Added UTF-8 filter to cut invalid byte sequence
each replaced with find
@ark-konopacki
Copy link
Contributor Author

@jmoody Rebased and updated ;)
Ready for review.

* Improve error message when no .app can be found
@jmoody
Copy link
Contributor

jmoody commented Feb 16, 2016

This is a great start. There is work still to do here:

  1. The entire launch/simulator_launcher.rb needs to be removed - it is very outdated
  2. .app detection should be moved to run-loop
  3. Needs integration tests

jmoody added a commit that referenced this pull request Feb 16, 2016
…of-app-bundle

SimLauncher: fix autodetection of app bundle
@jmoody jmoody merged commit 52db759 into calabash:develop Feb 16, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants