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

Don't write spec to podfile when it's an empty string #577

Merged
merged 2 commits into from
Mar 20, 2019
Merged

Don't write spec to podfile when it's an empty string #577

merged 2 commits into from
Mar 20, 2019

Conversation

piotr-cz
Copy link
Contributor

Platforms affected

iOS

Motivation and Context

When plugin framework tag contains an empty spec value (spec=""), Podfile becomes malformed and pod install command fails with

ArgumentError - Illformed requirement `""`

Example:

    <framework src="Fabric" type="podspec" spec="~> 1.7.6"/>
    <framework src="Crashlytics" type="podspec" spec="~> 3.10.1"/>
    <framework src="Firebase/Core" type="podspec" spec=""/>

Creates

	pod 'Fabric', '~> 1.7.6'
	pod 'Crashlytics', '~> 3.10.1'
	pod 'Firebase/Core', ''

Proposed change checks for an empty spec value and doesn't write version number:

	pod 'Fabric', '~> 1.7.6'
	pod 'Crashlytics', '~> 3.10.1'
	pod 'Firebase/Core'

Description

I'm not sure if it's a bug in cordova-ios or specific plugin (cordova-plugin-firebase-crashlytics).
However cordova documentation for the framework tag doesn't specify if the spec attribute can be omitted so it seems valid to me to use empty value.

Testing

  • Install cordova-plugin-firebase-crashlytics plugin in verbose mode:
    cordova plugin add cordova-plugin-firebase-crashlytics --verbose
  • Installation doesn't fail

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

@piotr-cz piotr-cz changed the title Don't write specs when it's an empty string Don't write spec to podfile when it's an empty string Mar 20, 2019
@piotr-cz

This comment has been minimized.

@janpio

This comment has been minimized.

@codecov-io
Copy link

Codecov Report

Merging #577 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #577   +/-   ##
=======================================
  Coverage   74.58%   74.58%           
=======================================
  Files          11       11           
  Lines        1826     1826           
=======================================
  Hits         1362     1362           
  Misses        464      464
Impacted Files Coverage Δ
bin/templates/scripts/cordova/lib/Podfile.js 73.02% <100%> (ø) ⬆️

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 eede5c7...7e801d9. Read the comment docs.

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

@dpogue dpogue added this to the 5.0.1 milestone Mar 20, 2019
@dpogue
Copy link
Member

dpogue commented Mar 20, 2019

Thanks for finding and fixing this! I'll merge it in for the upcoming 5.0.1 patch release :)

@dpogue dpogue merged commit 5565497 into apache:master Mar 20, 2019
@piotr-cz piotr-cz deleted the patch-1 branch March 21, 2019 09:33
@piotr-cz
Copy link
Contributor Author

After thinking about it a little more, it's supposed to be considered as bug in cordova-ios v5 and not in plugins as v4 has evaluating emptiness of the spec value before adding it to the Podfile

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.

4 participants