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

Cocoapods support improvement, using podspec tag in plugin.xml #405

Merged
merged 1 commit into from
Dec 10, 2018

Conversation

knight9999
Copy link
Contributor

@knight9999 knight9999 commented Sep 12, 2018

Platforms affected

ios

What does this PR do?

  • Cocoapods Support Improvement
  • Support Custom Cocoapods Repository
  • Added Test Code

See apache/cordova-discuss#108

The following PR is needed to be merged first
apache/cordova-common#48

What testing has been done on this change?

  • Local Plugin Test (cordova plugin add which uses podspec tags to local PC)
  • npm run test

Because this PR depends on PR (apache/cordova-common#48), the test fails without editing package.json.
With modified package.json, both the travis and appveyor tests passes.

Checklist

  • Added automated test coverage as appropriate for this change.

@shazron
Copy link
Member

shazron commented Sep 12, 2018

Thanks @knight9999 but the builds are failing:

npm run unit-tests
> cordova-ios@5.0.0-dev unit-tests C:\projects\cordova-ios
> jasmine --config=tests/spec/unit.json
module.js:478
    throw err;
    ^
Error: Cannot find module 'fs-extra'
    at Function.Module._resolveFilename (module.js:476:15)

@knight9999
Copy link
Contributor Author

knight9999 commented Sep 12, 2018

@shazron Thanks for testing, and I am sorry to trouble you.
This PR depends on PR apache/cordova-common#48.
Therefore please change package.json as follows only for testing.

diff --git a/package.json b/package.json
index 566720fd..bc7cd729 100644
--- a/package.json
+++ b/package.json
@@ -51,7 +51,7 @@
   },
   "engineStrict": true,
   "dependencies": {
-    "cordova-common": "2.1.0",
+    "cordova-common": "https://github.com/cordova-develop/cordova-common.git#dev_improve_cocoapod_support",
     "ios-sim": "^6.1.2",
     "nopt": "^3.0.6",
     "plist": "^1.2.0",

@knight9999 knight9999 force-pushed the dev_improve_cocoapod_support branch from cb43839 to 53c2526 Compare September 13, 2018 05:01
@knight9999
Copy link
Contributor Author

knight9999 commented Sep 13, 2018

I have rebased master.
The above my comments are also updated.
For tests, only cordova-common in package.json should be edited.

@shazron
Copy link
Member

shazron commented Sep 13, 2018

Thanks! This can be reviewed and tested, but can't be accepted to be merged until the production release of cordova-common that has the changes needed is out for final testing.

We've had this situation before, and we just had to wait until the dependency is updated in npm. Thus, let us try to get cordova-common that has these dependent changes out for a new release, then we update this PR.

@knight9999
Copy link
Contributor Author

@shazron Thanks a lot.
It's exactly as you said. We have to wait until the dependency is updated.
I think waiting is OK.

@knight9999 knight9999 force-pushed the dev_improve_cocoapod_support branch from 53c2526 to 465e4bb Compare September 13, 2018 07:06
@knight9999
Copy link
Contributor Author

knight9999 commented Sep 13, 2018

Sorry, I have updated this PR again to fix the issue that the swift-version in CocoaPods is reseted when adding other CoacoaPods plugin.

janpio
janpio previously requested changes Sep 13, 2018
Copy link
Member

@janpio janpio left a comment

Choose a reason for hiding this comment

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

(just so this doesn't get merged accidentally)
needs cordova-common release and update in package.json

@knight9999 knight9999 force-pushed the dev_improve_cocoapod_support branch from 465e4bb to bcdf347 Compare September 25, 2018 03:30
@knight9999 knight9999 mentioned this pull request Sep 25, 2018
2 tasks
@knight9999 knight9999 force-pushed the dev_improve_cocoapod_support branch from bcdf347 to e02c3c7 Compare November 8, 2018 03:30
@codecov-io
Copy link

codecov-io commented Nov 8, 2018

Codecov Report

Merging #405 into master will increase coverage by 1.06%.
The diff coverage is 75.52%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #405      +/-   ##
==========================================
+ Coverage   74.29%   75.36%   +1.06%     
==========================================
  Files          12       12              
  Lines        1564     1802     +238     
==========================================
+ Hits         1162     1358     +196     
- Misses        402      444      +42
Impacted Files Coverage Δ
...ates/scripts/cordova/lib/plugman/pluginHandlers.js 90.2% <100%> (+2.26%) ⬆️
bin/templates/scripts/cordova/lib/Podfile.js 73.36% <67.59%> (-6.98%) ⬇️
bin/templates/scripts/cordova/Api.js 71.79% <73.58%> (+12.62%) ⬆️
bin/templates/scripts/cordova/lib/PodsJson.js 95.04% <93.65%> (-4.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 62ebfbd...e02c3c7. Read the comment docs.

@erisu
Copy link
Member

erisu commented Nov 8, 2018

@janpio common 3 had been released and master's package.json had been bumped.
Additional, the PR had been rebased and tests are all green. I believe we can remove the block.

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.

Ran following tests:

$ cordova-nightly create podtest com.erisu.podtest
$ cordova-nightly platform add github:cordova-develop/cordova-ios#dev_improve_cocoapod_support
$ cordova-nightly prepare ios
$ cordova-nightly compile ios
$ cordova-nightly build ios
$ cordova-nightly run ios
$ cordova-nightly plugin add onesignal-cordova-plugin
>>> This uses the old pattern <framework src="OneSignal" type="podspec" spec="2.9.3" />
$ cordova-nightly build ios
>>> Test build with plugin added with old pattern
$ cordova-nightly plugin rm onesignal-cordova-plugin
$ cordova-nightly build ios
>>> Verify build works after Podfile and pods.json was updated.

I also tested with the fake plugin that uses the new podspec pattern. This was only to validate that the new pattern added the Pods properly and built. Was not intended to test the running of the app with this fake plugin.

$ cordova-nightly plugin add github:cordova-develop/cordova-plugin-pods3
$ cordova-nightly build ios

@janpio janpio dismissed their stale review November 8, 2018 10:59

obsolete

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.

👍 from me

@erisu erisu merged commit 65015c1 into apache:master Dec 10, 2018
erisu pushed a commit to erisu/cordova-ios that referenced this pull request Jan 16, 2019
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.

7 participants