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

Add missing rexml dependency #638

Merged
merged 1 commit into from Feb 13, 2020
Merged

Add missing rexml dependency #638

merged 1 commit into from Feb 13, 2020

Conversation

deivid-rodriguez
Copy link
Contributor

@deivid-rodriguez deivid-rodriguez commented Jan 14, 2020

The next ruby version won't have rexml as a default gem, so it will need to be explicitly added as a dependency.

See https://bugs.ruby-lang.org/issues/16485#change-83794.

The next ruby version won't have rexml as a default gem, so it will need
to be explicitly added as a dependency.
@ashmaroli
Copy link
Contributor

@ashmaroli ashmaroli commented Jan 15, 2020

The next ruby version won't have rexml as a default gem

Just curious..

Isn't the next ruby version Ruby 3.0?
In that case wouldn't it be better to have all Ruby 3.0 compatibility changes in one PR..? Or does Ruby 2.7 throw deprecation warnings regarding rexml as well?

@deivid-rodriguez
Copy link
Contributor Author

@deivid-rodriguez deivid-rodriguez commented Jan 15, 2020

I test my library against ruby-head to be able to early address any ruby-core incompatibilities. My build against ruby-head broke because of the mentioned change, since under current ruby-head kramdown can no longer be required unless you manually depend on rexml.

This change should be correct regardless, because rexml is a gem, and kramdown depends on it. That said, I understand if you don't want to fix this this early, and I can easily workaround this by adding rexml to my Gemfile to account for kramdown lacking the dependency untill kramdown catches up.

Just let me know and I'll close the PR accordingly.

@ashmaroli
Copy link
Contributor

@ashmaroli ashmaroli commented Jan 15, 2020

@deivid-rodriguez I was just asking out of curiosity. I'm not a maintainer of this project..

@deivid-rodriguez
Copy link
Contributor Author

@deivid-rodriguez deivid-rodriguez commented Jan 15, 2020

Oh, ok, sorry. Well, I hope I answered your question in any case :)

@deivid-rodriguez
Copy link
Contributor Author

@deivid-rodriguez deivid-rodriguez commented Jan 15, 2020

I should probably also add that this change does have an impact. For example, if rexml makes a disastrous release that breaks everything, kramdown would be affected. But on the other hand, if rexml fixes a security vulnerability, kramdown will automatically pick that up :)

@deivid-rodriguez
Copy link
Contributor Author

@deivid-rodriguez deivid-rodriguez commented Jan 15, 2020

Actually, those same things can already happen currently, but the changes only happen accross ruby releases. With this PR, any change in the rexml dependency becomes more "agile" because it only requires a gem release, not a release of the full ruby language.

@gettalong gettalong merged commit c1aa6ad into gettalong:master Feb 13, 2020
1 check passed
@gettalong
Copy link
Owner

@gettalong gettalong commented Feb 13, 2020

@deivid-rodriguez Thank you for your contribution!

@deivid-rodriguez deivid-rodriguez deleted the add_missing_rexml_dependency branch Feb 13, 2020
@deivid-rodriguez
Copy link
Contributor Author

@deivid-rodriguez deivid-rodriguez commented Feb 13, 2020

Thanks for merging! ❤️

@caser
Copy link

@caser caser commented Mar 5, 2020

I'm still getting this error:

/.rvm/gems/ruby-head/gems/kramdown-1.17.0/lib/kramdown/parser/html.rb:10:in require': cannot load such file -- rexml/parsers/baseparser (LoadError)`

Is this related to above?

@deivid-rodriguez
Copy link
Contributor Author

@deivid-rodriguez deivid-rodriguez commented Mar 5, 2020

I don't think this fix has been released yet, so could be related, yeah. Does it get fixed if you manually install rexml or add it to your Gemfile?

@gettalong
Copy link
Owner

@gettalong gettalong commented Mar 5, 2020

@caser Please try the latest version of kramdown and see if it works!

@JetForMe
Copy link

@JetForMe JetForMe commented Jan 27, 2021

Any progress on this? I just ran into it, as kramdown is a transitive dependency. I had to explicitly include rexml, which now makes my install a little more fragile.

@ashmaroli
Copy link
Contributor

@ashmaroli ashmaroli commented Jan 27, 2021

@JetForMe This change was shipped in kramdown-2.2.0. So, you may have to update your installed version.

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.

None yet

5 participants