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

Explicit dependency on Racc #1988

Closed
voxik opened this issue Feb 4, 2020 · 9 comments · Fixed by #2104
Closed

Explicit dependency on Racc #1988

voxik opened this issue Feb 4, 2020 · 9 comments · Fixed by #2104
Milestone

Comments

@voxik
Copy link
Contributor

voxik commented Feb 4, 2020

Since Nokogiri depends on Racc in runtime, it would be nice if the dependency was explicitly specified. I know that Racc is part of Ruby StdLib as default gem, OTOH one day Racc will release incompatible version and Nokogiri won't be usable with certain older versions of Ruby without older gemified version of Racc.

@flavorjones
Copy link
Member

@voxik Thanks for pointing this out! As you mentioned, because it's in the Ruby stdlib, we don't today need to explicitly require it as a dependency.

I think I'd prefer to wait until a problem arises due to a backwards-incompatible difference between the Racc version in stdlib and what's generated in the Nokogiri code base. If it happens, I'd be happy to cut a patch release specifying a dependency on a particular version of Racc. In the meantime, we have CI pipelines running against all supported Ruby versions to detect any brokenness around Racc behavior.

I'm curious if you think we should also declare a runtime dependency on yaml, psych, strscan, and stringio, which are also gems in the stdlib?

@voxik
Copy link
Contributor Author

voxik commented Feb 11, 2020

I'm curious if you think we should also declare a runtime dependency on yaml, psych, strscan, and stringio, which are also gems in the stdlib?

I really think every project should properly specify their dependencies, so this applies to every gem. You can never know who else is using any of these libraries and might want to use some specific version. E.g. use yaml with syck instead of psych. These are things out of your control. Also, for example, now I have fix several packages failing in Fedora due to e2mmap removal from Ruby StdLib. If the projects specified this dependency explicitly, it would be much easier to track the possible issues.

@tenderlove
Copy link
Member

How does this help nokogiri users as opposed to package maintainers? I am sure it will cause users to spend more time resolving gems and downloading packages, so what do they get out of it?

I'm not super opposed to this, but I'd really like to know the upside for actual users. Thanks.

@voxik
Copy link
Contributor Author

voxik commented Feb 27, 2020

Think of it this way. If there is gem which specifies s.add_dependency "racc", "1.4.6" and it is mixed with Nokogiri in one app, what will be the result? What will tell the developer/user, that this is probably not good idea?

Or another case. If next Ruby is released with incompatible version of Racc 2.x, all old versions of Nokogiri will be broken just because it does not specify its dependencies properly. Some new "emergency" release might fix this, but you can't fix the old releases.

Also, if Ruby decides to drop racc, all Nokogiri versions will be broken (unnoticeably). Or Ruby will be forced to carry racc around although it is obsolete.

It seems to me that the trading "users to spend more time resolving gems and downloading packages" for possibly hard to debug bugs is not the best tradeoff. Not mentioning that so far the racc is still part of StdLib, so I am not sure where the "downloading packages" part comes from (to be precise, this applies to Ruby 2.7, where the racc was gemified, so I might be wrong for older releases). Also the "resolving gems" part is arguable, because these days, the versions are typically resolved in Gemfile.lock, so there is not resolving every time the app is running.

@flavorjones
Copy link
Member

@voxik I'm open to making this change. Would you be interested in drafting a pull request that we can collaborate on?

voxik added a commit to voxik/nokogiri that referenced this issue Oct 26, 2020
Nokogiri always have had dependency on Racc, but it was never stated
explicitly. This is possible issue for several reasons:

1) There is no way RubyGems/Bundler could avoid loading wrong version of
   Racc if other package in application specifies different version of
   Racc then Nokogiri needs.
2) If there is released incompatible Racc 2.x, all old versions of
   Nokogiri will be broken just because it does not specify its
   dependencies properly.
3) If Ruby decides to drop Racc from StdLib, all Nokogiri versions will
   be broken (unnoticeably). Or Ruby will be forced to carry Racc around
   around although it is possibly obsolete.

Fixes sparklemotion#1988
@flavorjones flavorjones added this to the v1.11.0 milestone Oct 26, 2020
flavorjones pushed a commit to voxik/nokogiri that referenced this issue Oct 26, 2020
Nokogiri always have had dependency on Racc, but it was never stated
explicitly. This is possible issue for several reasons:

1) There is no way RubyGems/Bundler could avoid loading wrong version of
   Racc if other package in application specifies different version of
   Racc then Nokogiri needs.
2) If there is released incompatible Racc 2.x, all old versions of
   Nokogiri will be broken just because it does not specify its
   dependencies properly.
3) If Ruby decides to drop Racc from StdLib, all Nokogiri versions will
   be broken (unnoticeably). Or Ruby will be forced to carry Racc around
   around although it is possibly obsolete.

Fixes sparklemotion#1988
@wlipa
Copy link

wlipa commented Jan 4, 2021

Just wanted to note that this change adds a new gem with native extensions that need to be built.

Fetching racc 1.5.2
Installing racc 1.5.2 with native extensions

@flavorjones
Copy link
Member

@wlipa Thanks for commenting. I've tried to reproduce this with Ruby 2.7 and Ruby 3.0 and cannot:

# ruby 2.7
juno ruby-2.7.2 go1.14.6 (master)
nokogiri $ gem uninstall racc nokogiri
Gem 'nokogiri' is not installed
Gem racc-1.4.16 cannot be uninstalled because it is a default gem

Select gem to uninstall:
 1. racc-1.5.1
 2. racc-1.5.2
 3. All versions
> 3
Successfully uninstalled racc-1.5.1
Successfully uninstalled racc-1.5.2

juno ruby-2.7.2 go1.14.6 (master)
nokogiri $ gem install nokogiri
Fetching nokogiri-1.11.0-x86_64-linux.gem
Successfully installed nokogiri-1.11.0-x86_64-linux
1 gem installed
# ruby 3.0
nokogiri $ gem uninstall racc nokogiri
Remove executables:
	nokogiri

in addition to the gem? [Yn]  y
Removing nokogiri
Successfully uninstalled nokogiri-1.11.0.rc3-x86_64-linux
Gem racc-1.5.1 cannot be uninstalled because it is a default gem
Successfully uninstalled racc-1.5.2

juno ruby-3.0.0 go1.14.6 (master)
nokogiri $ gem install nokogiri
Fetching nokogiri-1.11.0-x86_64-linux.gem
Successfully installed nokogiri-1.11.0-x86_64-linux
1 gem installed

This is because racc is a default gem in both versions of Ruby.

If you have more questions, I'd prefer to open a new issue for it, if you wouldn't mind -- with all the information we ask for in the template about your system. Thanks!

@flavorjones
Copy link
Member

Ah, @wlipa I'm guessing you're running Ruby 2.6 or earlier, which doesn't declare Racc as a gem. In that case, yes, this is a known side effect of the new explicit dependency.

@wlipa
Copy link

wlipa commented Jan 4, 2021

I'm using 2.7.2. It was via a 'bundle update' command, not invoking 'gem install' directly. I'll open another issue for it.

$ ruby --version
ruby 2.7.2p137 (2020-10-01 revision 5445e04352) [x86_64-darwin20]

This was referenced Mar 15, 2021
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 a pull request may close this issue.

4 participants