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

Update project files for Xcode 10 #409

Merged
merged 6 commits into from
Dec 15, 2018
Merged

Update project files for Xcode 10 #409

merged 6 commits into from
Dec 15, 2018

Conversation

dpogue
Copy link
Member

@dpogue dpogue commented Sep 16, 2018

This is an alternative proposal to #408, but I lack the knowledge to make any definitive statements about one being better than the other.

/cc @sgoldberg-sfdc

Platforms affected

iOS

What does this PR do?

Updates the CordovaLib.xcodeproj and template xcodeproj files to use the recommended settings from Xcode 10. This appears to solve the problems with building in Xcode 10, but doesn't appear to break building in Xcode 9.

What testing has been done on this change?

Manual testing done with Xcode 10 GM and Xcode 9.4.1.
Updated unit tests for new behaviour.

@brodycj
Copy link
Contributor

brodycj commented Sep 16, 2018 via email

@sgoldberg-sfdc
Copy link
Contributor

WIth this branch I still see the Xcode 10 failures

Testing failed: Redefinition of enumerator 'CDVCommandStatus_NO_RESULT' Redefinition of enumerator 'CDVCommandStatus_OK' Redefinition of enumerator 'CDVCommandStatus_CLASS_NOT_FOUND_EXCEPTION' Redefinition of enumerator 'CDVCommandStatus_ILLEGAL_ACCESS_EXCEPTION' Redefinition of enumerator 'CDVCommandStatus_INSTANTIATION_EXCEPTION' Redefinition of enumerator 'CDVCommandStatus_MALFORMED_URL_EXCEPTION' Redefinition of enumerator 'CDVCommandStatus_IO_EXCEPTION' Redefinition of enumerator 'CDVCommandStatus_INVALID_ACTION' Redefinition of enumerator 'CDVCommandStatus_JSON_EXCEPTION' Redefinition of enumerator 'CDVCommandStatus_ERROR' Typedef redefinition with different types ('enum CDVCommandStatus' vs 'enum CDVCommandStatus') Duplicate interface definition for class 'CDVPluginResult' Property has a previous declaration Property has a previous declaration Property has a previous declaration Property has a previous declaration Duplicate interface definition for class 'CDVInvokedUrlCommand' Property has a previous declaration Property has a previous declaration Too many errors emitted, stopping now linker command failed with exit code 1 (use -v to see invocation) Testing cancelled because the build failed. ** TEST FAILED **'
type errors. This is with both Carthage and building in the IDE, and from xcode build.

@sgoldberg-sfdc
Copy link
Contributor

Updating to recommended settings is something that should happen regardless of the current build issues though.

@dpogue
Copy link
Member Author

dpogue commented Sep 17, 2018

@sgoldberg-sfdc What commands are you running to test this?

I created a new Cordova project based on the changes in this PR and was able to make builds using Xcode 10, both in Xcode and via the command-line, both debug and enterprise release builds, and in the simulator.

@sgoldberg-sfdc
Copy link
Contributor

@dpogue The issue seems to stem from how the new build system handles the include directories, and the fact that the static lib copies the headers to a folder that would get picked up by Xcode on a subsequent build. Depending on the state of your derived data, it may appear to build as it is skipping certain build steps. Try building the Framework App target with a clean derived data and the new build system and you will see the issue.

@dpogue
Copy link
Member Author

dpogue commented Sep 17, 2018

Try building the Framework App target

Not sure what you mean by this. There's only one app in my newly-created project.

@sgoldberg-sfdc
Copy link
Contributor

Open Cordova-ios.xcworkspace and you have the following targets:

  • Cordova (a framework)
  • CordovaFrameworkApp (Sample app to test framework)
  • CordovaLib (Static Library)
  • CordovaLibApp (Sample app to test static library target)
  • CordovaLibTests (Test suite for static library target)

I am referring to the CordovaFrameworkApp which runs the test suite. If you have xcode-select configured for xcode10 you can also run this command which is one of the test suites executed by the PR checks.

xcodebuild test -workspace tests/cordova-ios.xcworkspace -scheme CordovaFrameworkApp -destination "platform=iOS Simulator,name=iPhone 8" CONFIGURATION_BUILD_DIR="mktemp -d 2>/dev/null || mktemp -d -t 'cordova-ios'"

@dpogue
Copy link
Member Author

dpogue commented Sep 17, 2018

Ahhh, I didn't even realize that project existed. It probably also needs to be updated to recommended settings, and hopefully that will fix the issues.

Will try to tackle that tonight!

@mhartington
Copy link
Contributor

Tested this PR out, only thing I can note is with regards to splashscreens/launch images not showing up when emulating from the command line. Not sure if that is because of the changes or something else though.

Other than that, builds work from the command line. Thanks @dpogue !

@sgoldberg-sfdc
Copy link
Contributor

I believe when you generate a project using Cordova it uses the static library, so the framework issues will not appear in the typical workflow.

@dpogue
Copy link
Member Author

dpogue commented Sep 18, 2018

I believe when you generate a project using Cordova it uses the static library, so the framework issues will not appear in the typical workflow.

Yeah... I poked at the Cordova-ios.xcworkspace stuff last night, and by the end of it my changes looked almost identical to what you've got in #408.

I was poking around at old JIRA issues last night and noticed CB-13043 which suggests we might want to move to using Cordova.framework in all cases, but I'm a little bit concerned about the potential impact of that sort of change on 3rd party plugins and their header includes.

@sgoldberg-sfdc I'll try to get what I did last night into a fully working state and push the changes here (and I'll attribute them to you, since they're basically identical to the work you already did), and then maybe look at adding a preference for an app to switch between the static and framework versions.

@sgoldberg-sfdc
Copy link
Contributor

We consume the framework as we use Carthage for dependency management. I don't think I would force it on developers, but an option to the CLI might work.

Another simple (but of unknown impact to downstream developers) solution, would be to change the static library public headers folder such that its not include/Cordova , something like Cordova/include.

@dpogue
Copy link
Member Author

dpogue commented Sep 19, 2018

@mhartington The splashscreen/icon issue you noticed, is that specific to this branch or does it also affect projects using master (with the -UseModernBuildSystem=0 flag)? It was only happening with Xcode 10?

@mhartington
Copy link
Contributor

Happens with the UseModernBuildSystem and the CLI emulate/run commands with master as well as with this PR applied

@leshik
Copy link

leshik commented Sep 20, 2018

I tried this patch and for me it builds the project successfully, but then fails to launch it (when using cordova run ios --device.) For now I've disabled this modern build thing.

@codecov-io
Copy link

codecov-io commented Oct 11, 2018

Codecov Report

Merging #409 into master will increase coverage by 0.04%.
The diff coverage is 90%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #409      +/-   ##
=========================================
+ Coverage   75.36%   75.4%   +0.04%     
=========================================
  Files          12      12              
  Lines        1802    1805       +3     
=========================================
+ Hits         1358    1361       +3     
  Misses        444     444
Impacted Files Coverage Δ
bin/templates/scripts/cordova/lib/prepare.js 83.89% <90%> (+0.1%) ⬆️

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 65015c1...d1acc92. Read the comment docs.

@dpogue dpogue changed the title Another possible Xcode 10 fix Update project files for Xcode 10 Dec 13, 2018
@dpogue dpogue requested review from shazron and erisu December 13, 2018 18:00
Copy link
Member

@erisu erisu left a comment

Choose a reason for hiding this comment

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

Build Environment

  • Xcode 10.1 Build version 10B61

Physical Device Test

  • iPhone 6 - iOS 12.1.2 (16D5024a)

Steps Performed

  1. npx cordova@nightly create xcode10Test com.foo.bar
    1.a. Set the app ID in config.xml if not passed in during create.
  2. cd ./xcode10Test
  3. npx cordova@nightly platform add github:dpogue/cordova-ios#xcode10
  4. Add build.json
  5. npx cordova@nightly build ios --device --release
  6. Manually deploy IPA to iPhone
  7. Manually Launched App
  8. App display Device is Ready

Step 5 Result

Cordova 8.0.1-nightly.2018.12.14.8a112b0b
Cordova-Lib 9.0.0-nightly.2018.12.14.a478ce3
** ARCHIVE SUCCEEDED **
** EXPORT SUCCEEDED **

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.

9 participants