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

GH-542 Adding Podspec for Cordova library #543

Merged
merged 24 commits into from
Mar 15, 2020
Merged

Conversation

bhariharan
Copy link
Contributor

@bhariharan bhariharan commented Feb 21, 2019

Platforms affected

cordova-ios

Motivation and Context

Description

  • Adds a podspec file that allows consumers to fetch the Cordova library through Cocoapods.

Testing

  • I have tested this by pulling this through Cocoapods in my own app and running it.

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

@codecov-io
Copy link

Codecov Report

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

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #543   +/-   ##
=======================================
  Coverage   74.75%   74.75%           
=======================================
  Files          11       11           
  Lines        1822     1822           
=======================================
  Hits         1362     1362           
  Misses        460      460

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 5e688ca...0996e39. Read the comment docs.

@codecov-io
Copy link

codecov-io commented Feb 21, 2019

Codecov Report

Merging #543 into master will not change coverage by %.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #543   +/-   ##
=======================================
  Coverage   74.20%   74.20%           
=======================================
  Files          13       13           
  Lines        1849     1849           
=======================================
  Hits         1372     1372           
  Misses        477      477           

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 20248c1...8b9399e. Read the comment docs.

Cordova.podspec Outdated Show resolved Hide resolved
Cordova.podspec Outdated Show resolved Hide resolved
Cordova.podspec Outdated Show resolved Hide resolved
Cordova.podspec Show resolved Hide resolved
Cordova.podspec Outdated Show resolved Hide resolved
Cordova.podspec Outdated Show resolved Hide resolved
@dpogue dpogue added this to the 5.0.1 milestone Mar 15, 2019
@jcesarmobile
Copy link
Member

Before we do this, we should check if we are allowed to do this, not sure if Apache will allow us to distribute through CocoaPods.

BTW, some people has been publishing it already https://cocoapods.org/pods/Cordova, so we probably will need permissions from them, cc @shazron

@janpio
Copy link
Member

janpio commented Mar 26, 2019

Afaik Apache doesn't really care about distribution method, as long as you also create normal binaries that are kept in SVN as the base of truth.

@dpogue dpogue modified the milestones: 5.0.1, 5.0.2 Apr 17, 2019
Copy link
Contributor Author

@bhariharan bhariharan left a comment

Choose a reason for hiding this comment

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

@dpogue I have addressed all the feedback. The version number and list of headers are auto-populated and updated whenever a change is made. There are no more manual steps required. This PR is ready to go. Would you mind taking a look and getting it merged so that I can consume it back in your next release? Thanks!

Cordova.podspec Show resolved Hide resolved
Cordova.podspec Show resolved Hide resolved
);
runOnlyForDeploymentPostprocessing = 0;
shellPath = /bin/sh;
shellScript = "\"${PROJECT_DIR}/../update_podspec.sh\" -s cordova\n";
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added the shell script that updates podspec every time the library project is built. No more manual steps required to update the list of public headers in the podspec.

@@ -0,0 +1,70 @@
set -e
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Magic script to update podspec headers every time files are added/deleted in the library project.

@bhariharan
Copy link
Contributor Author

@erisu @jcesarmobile Could you help get this merged, please? All the feedback has been addressed and the podspec generation is completely automated now (no more manual update steps).

@bhariharan
Copy link
Contributor Author

bhariharan commented Dec 26, 2019

@jcesarmobile @dpogue @erisu Could you please review this and add it to the list for 6.0.0? I have addressed all the feedback and verified everything works as expected. Happy to answer any questions you may have. We need this to get off of our fork of Cordova. Thanks!

@brodycj brodycj removed this from the 5.0.2 milestone Dec 26, 2019
@brodycj brodycj added this to the 6.0.0 milestone Dec 26, 2019
@brodycj
Copy link
Contributor

brodycj commented Dec 26, 2019

@bhariharan I just pushed the milestone to 6.0.0 which we are now targeting on the master branch. I think many maintainers are taking a much deserved holiday break. I recommend that you ping us on Slack or on the mailing list in the new year, contact info is linked in the footer of cordova.io or cordova.apache.org.

@bhariharan
Copy link
Contributor Author

Thanks, @brodybits! Happy holidays!

@jcesarmobile
Copy link
Member

As I said, somebody already published it in CocoaPods so we don’t have permissions to publish. While it’s true that it can also be installed from GitHub, I find this mostly useless as most people will want the official release in CocoaPods.
I think this shouldn’t be merged until we get permissions to publish.

And please please please, stop pinging me.

@brodycj
Copy link
Contributor

brodycj commented Dec 27, 2019

I am with @jcesarmobile to not merge this before we obtain permission to publish updates to Cordova on Cocoapods. I do have another concern that I just added in #542 (comment).

I would like to apologize for all of the delays that this PR has gone through. I am sure that this proposal would help others in the user community and am grateful for your efforts on this.

I would like to highly recommend that you followup with us on Slack or on the mailing list, please follow the links in the footer of cordova.io or cordova.apache.org.

@shortstuffsushi
Copy link

Hi there.

I'm one of the former maintainers of this podspec as well as a few of the plugins. I was added as a contributor by the original author at the time, and added several people I was working with at the time as well, since there wasn't interest from any Apache folks in maintaining it at the time.

I've been in contacted with @erisu recently who got me interested in this PR. I'm no longer using Cordova, so I was looking to hand this over (hasn't been updated for a couple years now..), so I've added him as an author for now. You're all welcome to add / remove maintainers as you please as this point. If you need anything from me, I'll be around - but I'm not doing any Cordova development these days, or any mobile at all lately, so I may not be able to speak too much to some of the specific questions.

@bhariharan
Copy link
Contributor Author

@erisu I will update this PR and address the outstanding feedback once your PR for removing UIWebView goes in. I imagine a bunch of classes will need to be updated here once that gets merged.

@bhariharan
Copy link
Contributor Author

Alright, I have brought in the changes from master, and verified that the script works as expected. The podspec has been updated as well. I have verified it's working as expected using our plugin (see here). Our plugin uses the podspec from our fork of Cordova currently - I'll switch this to the upstream repo once this gets merged. Let me know if there are any questions/outstanding issues I should address.

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.

LGTM

@bhariharan
Copy link
Contributor Author

Could this be merged?

Cordova.podspec Outdated
s.default_subspec = 'Cordova'
s.subspec 'Cordova' do |cordova|
cordova.source_files = 'CordovaLib/Classes/**/*.{h,m}', 'CordovaLib/Cordova/Cordova.h'
cordova.public_header_files = 'CordovaLib/Classes/Public/CDV.h', 'CordovaLib/Classes/Public/CDVAppDelegate.h', 'CordovaLib/Classes/Public/CDVAvailability.h', 'CordovaLib/Classes/Public/CDVAvailabilityDeprecated.h', 'CordovaLib/Classes/Public/CDVCommandDelegate.h', 'CordovaLib/Classes/Public/CDVCommandDelegateImpl.h', 'CordovaLib/Classes/Public/CDVCommandQueue.h', 'CordovaLib/Classes/Public/CDVConfigParser.h', 'CordovaLib/Classes/Public/CDVInvokedUrlCommand.h', 'CordovaLib/Classes/Public/CDVPlugin+Resources.h', 'CordovaLib/Classes/Public/CDVPlugin.h', 'CordovaLib/Classes/Public/CDVPluginResult.h', 'CordovaLib/Classes/Public/CDVScreenOrientationDelegate.h', 'CordovaLib/Classes/Public/CDVTimer.h', 'CordovaLib/Classes/Public/CDVUserAgentUtil.h', 'CordovaLib/Classes/Public/CDVViewController.h', 'CordovaLib/Classes/Public/CDVWebViewEngineProtocol.h', 'CordovaLib/Classes/Private/Plugins/CDVWebViewEngine/CDVWebViewUIDelegate.h', 'CordovaLib/Classes/Public/CDVWhitelist.h', 'CordovaLib/Cordova/Cordova.h', 'CordovaLib/Classes/Public/NSDictionary+CordovaPreferences.h', 'CordovaLib/Classes/Public/NSMutableArray+QueueAdditions.h'
Copy link
Member

Choose a reason for hiding this comment

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

Can the public_header_files property also be written in the same pattern like source_files?

cordova.public_header_files = 'CordovaLib/Classes/Public/**/*.{h}', 'CordovaLib/Cordova/Cordova.h'

I just noticed that this list is manually maintained and that the list is now not matching what is in master.

  • CDVUserAgentUtil.h had been removed
  • CDVURLSchemeHandler.h has been added

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@erisu No - public headers have to be explicitly listed. However, I added a script to automatically update this list every time a change is made. This list is not manually maintained - it's updated whenever CordovaLib is built in Xcode. It was behind only because this branch is behind master. I updated it to merge the latest from master, and the update podspec is in this commit. Basically, it will be updated automatically as part of the build process once this pull request is merged to master.

@erisu erisu merged commit d8f4b62 into apache:master Mar 15, 2020
@bhariharan bhariharan deleted the podspec branch March 15, 2020 17:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Podspecs for the Cordova library project
8 participants