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

(ios) issue-912: fix deployment to device #936

Merged
merged 1 commit into from
Jul 12, 2020
Merged

(ios) issue-912: fix deployment to device #936

merged 1 commit into from
Jul 12, 2020

Conversation

imgos
Copy link
Contributor

@imgos imgos commented Jul 8, 2020

cordova-ios used to use "mv -f" and it was replaced with fs-extra.moveSync. moveSync behaves
differently than mv, which broke deployment to devices. This fixes the folder move and
subsequently deployment to device (when using "cordova run ios --device ..."

Platforms affected

Motivation and Context

This was motivated by wanting to run an ios app on the device. This worked in versions of cordova-ios <= 5.1.1 and was broken in 6.0.0 and 6.1.0.

fixes: #912

Description

Fix a file copy. The behavior of moveSync is not exactly the same as mv -f which was previously used.

Testing

I have tested by deploying ios apps with cordova run ios --device from the command line. This restores the functionality from the old versions of cordova-ios.

Checklist

  • I've run the tests to see all new and existing tests pass
  • I added automated test coverage as appropriate for this change
  • Commit is prefixed with (platform) if this change only applies to one platform (e.g. (android))
  • If this Pull Request resolves an issue, I linked to the issue in the text above (and used the correct keyword to close issues using keywords)
  • I've updated the documentation if necessary

cordova-ios used to use "mv -f" and it was replaced with fs-extra.moveSync.  moveSync behaves
differently than mv, which broke deployment to devices.  This fixes the folder move and
subsequently deployment to device (when using "cordova run ios --device ..."
Copy link
Contributor

@breautek breautek left a comment

Choose a reason for hiding this comment

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

Thank you for taking a time to investigate and submit a PR. This change looks good to me, but I don't have a mac to actually test this myself. I'd like to see at least one other PMC member, preferably someone who can actually execute and test this code change.

@breautek breautek requested a review from erisu July 8, 2020 23:41
@erisu
Copy link
Member

erisu commented Jul 9, 2020

The changes are correct and the files are copied over to the correct location.

The app deploys to the phone but I noticed that only a black screen displays, no default app shown. If I open the project with Xcode and run on device, the application will launch normally and display default app content.

I used an iPhone Xs w/ iOS 13.5.1 for this test.

@imgos Is this the same behaviour you see?

@imgos
Copy link
Contributor Author

imgos commented Jul 9, 2020

@erisu - I've been able to deploy two cordova apps to my ios devices (iphone 7 plus and iphone 8) successfully with this. I have not had an app build and then open with a black screen.

I see there is an xcode project in the tests folder, but I'm not sure how to deploy it without opening in xcode. If you can point me to the instructions I'd be happy to try it and report the results.

@timbru31
Copy link
Member

timbru31 commented Jul 9, 2020

I'll give this PR a test today - codewise it looks good. Thanks for the investigation :)

Copy link
Member

@timbru31 timbru31 left a comment

Choose a reason for hiding this comment

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

App launches again on my connected device with this PR. If no veto, I'll merge this in 24 hours.

@erisu erisu removed their request for review July 10, 2020 08:33
@timbru31 timbru31 merged commit f1a6737 into apache:master Jul 12, 2020
@imgos imgos deleted the issue-912 branch July 12, 2020 19:18
@StratusBase
Copy link

When is a release going to happen with this change? This is pretty crucial...

@timbru31
Copy link
Member

We do not give any ETAs of new releases.

@motla
Copy link

motla commented Aug 10, 2020

It has been a month, is it complicated for you to release a patch?

@mignam
Copy link

mignam commented Aug 10, 2020

Hello !

Same here, hope it will be patch soon :)

Thanks for the investigation !

@codecat
Copy link

codecat commented Aug 11, 2020

Thanks for the pull request, I was facing this issue as well today.

As a temporary workaround until this gets released to stable, you can either downgrade to 5.1.1 (latest version prior to 6.0.0) or use the latest nightly version. (Which you can find here under "versions".)

Here's how to change the version of your iOS platform:

$ cordova platform remove ios
$ cordova platform add [email protected]

To install the nightly version, simply replace 5.1.1 with nightly.

@breautek
Copy link
Contributor

breautek commented Aug 11, 2020

To install the nightly version, simply replace 5.1.1 with nightly.

Nightly builds should be reserved for testing only. But for this case, a safer workaround is to use a specific version, such as 6.2.0-nightly.2020.8.11.f1a67376

This will at least provide consistency over nightly which will change for every commit merged into master and may suddenly turn unstable (by accident of course).

@StratusBase
Copy link

Downgrading to 5.1.1 is not an option as I will not start a brand new project using the old UIWebview... Apple won’t allow apps with any reference to it. The 6x version removes that reference and uses the WKWebView but has a bug that prevents the package from even getting deployed to your iOS device. A recent commit has resolved this but hasn’t made it into a release so I’m hard-coding the change for the time being... Anyone who wants to use this package is basically stuck doing this since no one is going to want to downgrade if they’re taking iOS deployment seriously... Apple won’t allow it.

@codecat
Copy link

codecat commented Aug 11, 2020

Do you have a link to the Apple docs where it talks about this? I'll have to deploy soon and I have some projects still on 5.1.1.

@imgos
Copy link
Contributor Author

imgos commented Aug 11, 2020

Can we open a separate issue to address problems in the release process?

What are the issues in the release process that make it difficult to release a patch? Once these are identified, maybe there are ways some of us can help automate the process.

@StratusBase
Copy link

Do you have a link to the Apple docs where it talks about this? I'll have to deploy soon and I have some projects still on 5.1.1.

https://developer.apple.com/news/?id=12232019b

https://developer.apple.com/documentation/uikit/uiwebview
https://developer.apple.com/documentation/webkit/wkwebview

@StratusBase
Copy link

Can we open a separate issue to address problems in the release process?

What are the issues in the release process that make it difficult to release a patch? Once these are identified, maybe there are ways some of us can help automate the process.

Exactly, at a minimum this should just be a patch, not a minor version release or requiring me to tag nightly or any of that...

@timbru31
Copy link
Member

timbru31 commented Aug 11, 2020

Sorry to interrupt here, but some comments are simply wrong. It is safe to use cordova-ios@5 with the WKWebView plugin. v6 simply integrated those changes into the core platform.

@imgos please refer to https://www.apache.org/legal/release-policy.html#release-approval - we can’t alter the process much. And yes a patch is more work than a minor because we need to cherry pick to the correct branches etc. besides that, all development (no support!) discussions should take place via the dev mailing list.

@breautek
Copy link
Contributor

Do you have a link to the Apple docs where it talks about this? I'll have to deploy soon and I have some projects still on 5.1.1.

5.1.1 uses UIWebView by default. Additional steps are necessary to make it compliant with Apple, which are documented here.

dpogue pushed a commit that referenced this pull request Aug 28, 2020
cordova-ios used to use "mv -f" and it was replaced with fs-extra.moveSync.  moveSync behaves
differently than mv, which broke deployment to devices.  This fixes the folder move and
subsequently deployment to device (when using "cordova run ios --device ..."
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.

cordova-ios 6.0.0/6.1.0 wont deploy on physical device only simulator
8 participants