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

Specify dependencies in gemspec, add GitHub Actions #59

Merged
merged 3 commits into from
Sep 21, 2021

Conversation

0fye
Copy link
Contributor

@0fye 0fye commented Sep 21, 2021

Ruby 3 promoted rexml to a bundled gem:

https://www.ruby-lang.org/en/news/2020/09/25/ruby-3-0-0-preview1-released/

Downstream users with Ruby 3 need to specify gem "rexml" when using this project, this actually affects many CocoaPods users:

By specifying dependencies in our gemspec, we eliminate the need for them to do so.

Also Github Actions is added as CI.

@0fye
Copy link
Contributor Author

0fye commented Sep 21, 2021

CI passes at my own fork: https://github.com/0fye/CFPropertyList/actions/runs/1257068418

@ckruse
Copy link
Owner

ckruse commented Sep 21, 2021

First of all: thank you very much for your contribution!

Second: by adding all three XML parser gems as a runtime dependency, doesn't that mean we now have hard requirements to all three of them?

@0fye
Copy link
Contributor Author

0fye commented Sep 21, 2021

Second: by adding all three XML parser gems as a runtime dependency, doesn't that mean we now have hard requirements to all three of them?

Good one!

3 gems are used in following three files

however per https://github.com/ckruse/CFPropertyList/blob/master/lib/cfpropertylist/rbCFPropertyList.rb#L81-L99

begin
  require dirname + '/rbLibXMLParser.rb'
  temp = LibXML::XML::Parser::Options::NOBLANKS # check if we have a version with parser options
  temp = false # avoid a warning
  try_nokogiri = false
  CFPropertyList.xml_parser_interface = CFPropertyList::LibXMLParser
rescue LoadError, NameError
  try_nokogiri = true
end

if try_nokogiri then
  begin
    require dirname + '/rbNokogiriParser.rb'
    CFPropertyList.xml_parser_interface = CFPropertyList::NokogiriXMLParser
  rescue LoadError
    require dirname + '/rbREXMLParser.rb'
    CFPropertyList.xml_parser_interface = CFPropertyList::ReXMLParser
  end
end

It seems we are trying libxml first, then fallback to nokogiri, and finally to rexml. This is safe for Ruby 2 since rexml is bundled

To be safe and have minimum impact, maybe only require runtime dependency for rexml?

Or even better, only for Ruby 3 with something like:

if Gem::Version.new(RUBY_VERSION) >= Gem::Version.new('3.0.0')
  s.add_runtime_dependency("rexml") # no longer bundled with Ruby 3
end

What do you think?

@ckruse
Copy link
Owner

ckruse commented Sep 21, 2021

Yes, I think that would be the best way to go. Nokogiri and LibXML both require native code, so the impact would be bigger. Depending on rexml for Ruby 3 seems like the best solution to me, too.

@0fye
Copy link
Contributor Author

0fye commented Sep 21, 2021 via email

@0fye
Copy link
Contributor Author

0fye commented Sep 21, 2021

Now it requires rexml only for Ruby 3. Keeping nokogiri and libxml as dev dependencies for CI to pass.

https://github.com/0fye/CFPropertyList/actions/runs/1257729268

@ckruse ckruse merged commit bb8d2a1 into ckruse:master Sep 21, 2021
@ckruse
Copy link
Owner

ckruse commented Sep 21, 2021

Just published 3.0.4 with your fix. Thanks for contributing! ❤️

@0fye 0fye deleted the ci/github-actions branch September 21, 2021 23:54
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.

2 participants