ckruse / CFPropertyList

Read, write and manipulate both binary and XML property lists as defined by apple
MIT License
212 stars 47 forks source link

Specify dependencies in gemspec, add GitHub Actions #59

Closed 0fye closed 2 years ago

0fye commented 2 years ago

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 commented 2 years ago

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

ckruse commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

Great! I am away from computer now, will update this PR soon, thanks!

Christian Kruse @.***>於 2021年9月21日 週二,下午8:09寫道:

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

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/ckruse/CFPropertyList/pull/59#issuecomment-923921033, or unsubscribe https://github.com/notifications/unsubscribe-auth/AVUZXQC3TIL5Q3XW6BPQ46TUDBYYRANCNFSM5EN7UD2A . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

0fye commented 2 years ago

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 commented 2 years ago

Just published 3.0.4 with your fix. Thanks for contributing! :heart: