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

Removal of xcconfig build flag #398

Merged
merged 1 commit into from
Sep 6, 2018
Merged

Conversation

erisu
Copy link
Member

@erisu erisu commented Aug 28, 2018

Platforms affected

ios

What does this PR do?

  • Remove xcconfig build flag for device and emulator.
  • Remove custom argument override for xcconfig.
  • Updated build test spec with removal of xcconfig with cleanup.

Since Cordova projects are using xcworkspace and the content.xcworkspacedata file declares our projects xcodeproj, the xcconfig file paths are declared in the pbxproj.

contents.xcworkspacedata

<FileRef location = "group:HelloCordova.xcodeproj"></FileRef>

Once a Pod project is added to the application, for example cordova-plugin-firebase-messaging, the contents.xcworkspacedata file is updated to reflect the Pod project.

<FileRef location = "group:HelloCordova.xcodeproj"></FileRef>
<FileRef location = "group:Pods/Pods.xcodeproj"></FileRef>

If the xcconfig flag was set, in this scenario, pathing issues are introduced. For example header files will not be found and can not load. The xcconfig flag is applied to the entire workspace and will affect also the pod project’s xcconfig files.

Example

  • In our application, the SRCROOT varaible is declared as platforms/ios.
  • With the current implmetation (xcconfig flag deifned) the SRCROOT variable will be declared as platforms/ios/Pods for pod project.

In this case, in regards to the last item, the PODS_ROOT = ${SRCROOT}/Pods in Pods-HelloCordova.debug.xcconfig will be defined as platforms/ios/Pods/Pods which is incorrect.

Following this example:

$ cordova create podtest com.foobar.podtest
$ cd podtest
$ cordova platform add [email protected]
$ cordova plugin add cordova-plugin-firebase-messaging
$ cordova prepare ios
$ cordova compile ios

You will notice in the build output the incorrect paths for example: /cordova/podtest/platforms/ios/Pods/Pods/Headers/FirebaseCore -

By removing the flag, the SRCROOT will not be affected for the pods project so that when Pods-HelloCordova.debug.xcconfig declares PODS_ROOT, the path will then be correct platforms/ios/Pods.

What testing has been done on this change?

General Tests

  • npm test
  • npm run eslint

Default Template

  • cordova build ios
  • cordova build ios --release --device
  • cordova build ios --debug --device
  • cordova run ios --emulator
  • cordova prepare ios
  • cordova compile ios

Default Template + CocoaPods Plugin

  • cordova build ios
  • cordova build ios --release --device
  • cordova build ios --debug --device
  • cordova run ios --emulator
  • cordova prepare ios
  • cordova compile ios

Pod test is using cordova-support-google-services and cordova-plugin-firebase-messaging plugin.

@erisu erisu changed the title WIP - Removal of xcconfig build flag WIP: Removal of xcconfig build flag Aug 28, 2018
@codecov-io
Copy link

codecov-io commented Aug 29, 2018

Codecov Report

Merging #398 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #398   +/-   ##
=======================================
  Coverage   74.26%   74.26%           
=======================================
  Files          12       12           
  Lines        1562     1562           
=======================================
  Hits         1160     1160           
  Misses        402      402
Impacted Files Coverage Δ
bin/templates/scripts/cordova/lib/build.js 57.71% <ø> (ø) ⬆️

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 9d2ea13...4dd0586. Read the comment docs.

- Remove xcconfig build flag for device and emulator.
- Remove custom argument override for xcconfig.
- Updated build test spec with removal of xcconfig with cleanup.
@erisu erisu changed the title WIP: Removal of xcconfig build flag Removal of xcconfig build flag Aug 29, 2018
@erisu erisu requested review from dpogue and shazron September 4, 2018 00:06
Copy link
Member

@dpogue dpogue left a comment

Choose a reason for hiding this comment

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

👍

Just to confirm, it still looks at the build.xcconfig file when building? Existing workflows that append to that file will continue to work as they current do?

@knight9999
Copy link
Contributor

@dpogue Yes, the project still refers to build.xcconfig when building. This is defined in project.pbxproj.

Copy link
Contributor

@knight9999 knight9999 left a comment

Choose a reason for hiding this comment

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

It looks good.

@tobilarscheid
Copy link

Hey guys, do you know when this will be released?

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.

5 participants