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

More logging for simulator selection and deployment #596

Merged
merged 20 commits into from
Apr 17, 2019

Conversation

janpio
Copy link
Member

@janpio janpio commented Apr 14, 2019

Motivation and Context

Currently the simulator selection and app deployment on the selected simulator is a bit of a black box, the logging is insufficient to properly understand and retrace what is happening.

Description

  • When building for simulator
    • display more information about the simulator being built for (simulator ID, simulator identifier string):
      $ cordova run ios --no-telemetry --no-update-notifier --target iPhone-XR --emulator
      Building for "iPhone X" Simulator (com.apple.CoreSimulator.SimDeviceType.iPhone-X, iPhone-X)
      
    • before executing xcodebuild command, output the emulatorTarget that will be used as part of -destination option:
      Building project: /private/var/folders/nz/vv4_9tw56nv9k3tkvyszvwg80000gn/T/tmp-2604oc4wBu2VePt9/platforms/ios/HelloCordova.xcworkspace
      Configuration: Debug
      Platform: emulator
      Target: iPhone X
      
  • When deploying
    • log when deploy (both device and simulator) starts:
      Deploying to simulator
    • to simulator
      • use superspawn to trigger ios-sim
        • output the command used to start simulator:
          Running command: /private/var/folders/nz/vv4_9tw56nv9k3tkvyszvwg80000gn/T/tmp-2604oc4wBu2VePt9/node_modules/ios-sim/bin/ios-sim launch /private/var/folders/nz/vv4_9tw56nv9k3tkvyszvwg80000gn/T/tmp-2604oc4wBu2VePt9/platforms/ios/build/emulator/HelloCordova.app --devicetypeid com.apple.CoreSimulator.SimDeviceType.iPhone-XR --log /private/var/folders/nz/vv4_9tw56nv9k3tkvyszvwg80000gn/T/tmp-2604oc4wBu2VePt9/platforms/ios/cordova/console.log --exit
        • prefix logging coming from ios-sim with ios-sim log: for context:
          ios-sim log: device.name: iPhone X
          ios-sim log: device.runtime: iOS 11.0
          device.id: 5F22BAE5-1DB2-43CC-8983-5C4EE1BA37E7
          
        • output any errors returned from ios-sim and fail (instead of hanging):
           No available runtimes could be found for "iphone xʀ".
          /private/var/folders/nz/vv4_9tw56nv9k3tkvyszvwg80000gn/T/tmp-2604oc4wBu2VePt9/node_modules/ios-sim/bin/ios-sim: Command failed with exit code 1 Error output:
            No available runtimes could be found for "iphone xʀ".
          

(Related: The output this creates would benefit from changes to ios-sim: ios-control/ios-sim#255 + ios-control/ios-sim#256 (8.x))

Testing

This has been used in cordova-paramedic to better understand cordova-ios behaviour and uncover a few problems around simulator selection and deployment.

Checklist

  • I've run the tests to see all new and existing tests pass

@codecov-io
Copy link

codecov-io commented Apr 14, 2019

Codecov Report

Merging #596 into master will decrease coverage by 0.33%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #596      +/-   ##
==========================================
- Coverage   74.58%   74.24%   -0.34%     
==========================================
  Files          11       11              
  Lines        1826     1833       +7     
==========================================
- Hits         1362     1361       -1     
- Misses        464      472       +8
Impacted Files Coverage Δ
bin/templates/scripts/cordova/lib/build.js 51.26% <0%> (-0.53%) ⬇️
bin/templates/scripts/cordova/lib/run.js 22.12% <0%> (-1.96%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 44899e2...55216be. Read the comment docs.

@janpio janpio changed the title [WIP] Simulator deployment things [WIP] More logging for simulator selection and deployment Apr 15, 2019
@janpio janpio changed the title [WIP] More logging for simulator selection and deployment More logging for simulator selection and deployment Apr 16, 2019
@janpio janpio marked this pull request as ready for review April 16, 2019 15:55
@janpio janpio requested review from erisu and raphinesse April 16, 2019 15:55
Copy link
Contributor

@raphinesse raphinesse left a comment

Choose a reason for hiding this comment

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

LGTM 👍. Just two tiny remarks. Please feel free to disagree

bin/templates/scripts/cordova/lib/run.js Outdated Show resolved Hide resolved
bin/templates/scripts/cordova/lib/run.js Show resolved Hide resolved
@raphinesse
Copy link
Contributor

One other minor thing: would be nice if the paths for the ios-sim logging were relative to the project. That would improve readability but it could be more trouble than it's worth too

@janpio
Copy link
Member Author

janpio commented Apr 16, 2019

The apppath and iossim binary? I didn't touch those, and assumed there was a reason for the absolute paths here :/

@raphinesse
Copy link
Contributor

raphinesse commented Apr 16, 2019

The apppath and iossim binary? I didn't touch those, and assumed there was a reason for the absolute paths here :/

Yes, and the log path.

That might well be the case. It's safest to use absolute paths but it should™ also work with relative paths since you cwd to the project dir. But you never know that they don't mess up in ios-sim. And we would need three extra lines just to have prettier paths for logging...

@dpogue dpogue added this to the 5.0.1 milestone Apr 17, 2019
@janpio
Copy link
Member Author

janpio commented Apr 17, 2019

Thanks for the feedback, incorporated the one I understood.

Please feel free to edit the files yourself to get this ready to merge (as I won't be available the next few days).

@dpogue
Copy link
Member

dpogue commented Apr 17, 2019

Okay, if the tests (eventually) pass, let's merge this as-is and ship it in 5.0.1.
We can adjust the stderr output in the future based on whether we see duplicate errors in the output.

@dpogue dpogue merged commit da5b69d into master Apr 17, 2019
@dpogue dpogue deleted the janpio-594_deployment branch April 17, 2019 18:44
@janpio
Copy link
Member Author

janpio commented Apr 17, 2019

Thanks for the cleanup @dpogue!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants