apache / cordova-ios

Apache Cordova iOS
https://cordova.apache.org/
Apache License 2.0
2.15k stars 987 forks source link

refactor: replace copy-www-build-step script with build phase #1158

Closed NiklasMerz closed 2 years ago

NiklasMerz commented 2 years ago

Platforms affected

iOS and Mac (Catalyst)

Motivation and Context

We could move the build step of copying the www directory and config.xml to the Xcode "Copy files" commands to avoid issues from the manual script like changed paths or different paths between iOS and macOS.

See #699

Description

Up for discussion.

Some links: https://github.com/apache/cordova-ios/issues/699#issuecomment-645119408

This is probably the most important reason for the script: https://issues.apache.org/jira/browse/CB-5397?focusedCommentId=13859057&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-13859057

Testing

Checklist

codecov-commenter commented 2 years ago

Codecov Report

Merging #1158 (dc6a878) into master (92e6830) will not change coverage. The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1158   +/-   ##
=======================================
  Coverage   74.86%   74.86%           
=======================================
  Files          13       13           
  Lines        1723     1723           
=======================================
  Hits         1290     1290           
  Misses        433      433           

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 92e6830...dc6a878. Read the comment docs.

NiklasMerz commented 2 years ago

Fixed now. Good to know why they are there.

I should have checked the diff again. These Xcode files are tricky.

NiklasMerz commented 2 years ago

@dpogue How did you test that? What's the best way to reproduce this?

I did quick tests and this www folder and config.xml look like the right one. I changed them to __Project_NAME/.. but then plugins won't work anymore because cordovas js files are missing.

My test:

To me this looks like the right files get copied into the app.

dpogue commented 2 years ago

hmm, I was testing this as part of https://github.com/apache/cordova-ios/pull/1162 (which used this as a starting point) and getting "Connecting to device..." in the simulator, and log messages about the Console plugin not being found.

This should work to reproduce:

npx cordova@nightly create xcodeTestApp
cd xcodeTestApp
npx cordova@nightly platform add github:apache/cordova-ios#xcode-copy
open platforms/ios/HelloCordova.xcworkspace
# Run the app in a simulator from Xcode
NiklasMerz commented 2 years ago

This works fine for me. Is there another edge case where this could break?

dpogue commented 2 years ago

I'll try to poke at this a bit more today and see if I can figure out what was going wrong on my end

NiklasMerz commented 2 years ago

I just tried the device plugin and indeed it gets stuck at deviceready with this errror:

HelloCordova[24238:422811] ERROR: Plugin 'Device' not found, or is not a CDVPlugin. Check your plugin mapping in config.xml.

I am not sure yet whats the issue.

NiklasMerz commented 2 years ago

Interesting side fact. I just tried to run the osx platform on the M1 mac with this PR https://github.com/apache/cordova-osx/pull/105 and got the same error:

ERROR: Plugin 'Device' not found, or is not a CDVPlugin. Check your plugin mapping in config.xml.

jcesarmobile commented 2 years ago

Can’t we remove the duplicated www/config.xml and have a single one, that is what prepare command copies? I always found confusing having two, the staging one and the root one.

dpogue commented 2 years ago

Can’t we remove the duplicated www/config.xml and have a single one, that is what prepare command copies? I always found confusing having two, the staging one and the root one.

The root one is the app developer-authored one, where they configure the app name, icons, preferences, and allow list.
The staging one is the contents of the root one, with the additions from all the plugins, and is the one that needs to be bundled into the app bundle.

The root one is displayed in Xcode for convenience of the app developer if they are working in the native project, to make edits to config.xml (and www) and not have those changes overwritten when running prepare.

The staging one must be in Xcode to be handled as a resource for copying into the app bundle.

So I don't think we can get rid of either of them, despite how confusing it is to have both 😞

erisu commented 2 years ago

@dpogue I have updated this PR to properly copy the correct content as well as simplify the build process. This PR is strictly focused on copying the app resources to the correct location.

I have combined the build phases Copy www & Copy config.xml into one build phase call Copy Staging Resources.

The contents of the "Staging" group are, www and config.xml.

The location of these resources come from:

And these are what cordova prepare generates.

Additionally, the combine build phase is not blindly copying an entire grouping. If users add things to that group, they would still need to update the build phase to include those items to be copied. I think from a Cordova perspective, users should not add anything to this grouping anyways.