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

upgrade Nokogiri to latest version and add overrides for older Ruby versions #4339

Conversation

flavorjones
Copy link
Contributor

@flavorjones flavorjones commented Aug 23, 2022

AsciiDoctor currently requires Nokogiri ~> 1.10.0, but Nokogiri v1.10 has been out of support since January 2021, and this requirement may prevent AsciiDoctor tests from running on Ruby 3.1.

This PR updates the gemspec to allow for modern versions of Nokogiri.

A side benefit is that Nokogiri >= 1.11 ships precompiled native gems for most platforms.

flavorjones added a commit to sparklemotion/nokogiri that referenced this pull request Aug 23, 2022
@flavorjones flavorjones changed the title dep: loosen the dependency on Nokogiri to allow supported versions dep: loosen the development dependency on Nokogiri Aug 23, 2022
@flavorjones flavorjones force-pushed the flavorjones-update-nokogiri-dependency branch from 3f5ec92 to 86a64c7 Compare August 23, 2022 13:31
@ggrossetie
Copy link
Member

Related issue: #4310

The problem with upgrading is that it requires Ruby >= 2.6 and we test back to Ruby 2.3.
We are reevaluating how we drop old versions of Ruby, but suffice to say, core has to be prudent with the minimum Ruby version to support all the Linux distributions and macOS. So I don't think we can make this jump yet.

@KartikSoneji
Copy link

KartikSoneji commented Aug 23, 2022

The problem with upgrading is that it requires Ruby >= 2.6 and we test back to Ruby 2.3.

Hmm my understanding was nokogiri ~> 1.10 automatically selects the highest nokogiri version above 1.10 that is compatible with the current ruby version?

@ggrossetie
Copy link
Member

~> 2.0.3 is identical to >= 2.0.3 and < 2.1. ~> 2.1 is identical to >= 2.1 and < 3.0

https://guides.rubygems.org/patterns/#pessimistic-version-constraint

Currently, we are using ~> 1.10.0 which is equivalent to >= 1.10.0 and < 1.11 (i.e., take the latest patch version available)

@flavorjones
Copy link
Contributor Author

Ah, I see, I wasn't aware of the testing matrix. I think we can loosen it without breaking compatibility by updating to ~> 1.10, I'll update this PR.

@flavorjones flavorjones force-pushed the flavorjones-update-nokogiri-dependency branch from 86a64c7 to c2ad5c9 Compare August 23, 2022 13:47
@mojavelinux
Copy link
Member

Since this is effectively the 2.1.x branch, we can revisit the minimum version of Ruby we support. Currently, that is Ruby 2.5 based on what is still being used by supported versions of Linux. That would allow us to set the version of Nokogiri as follows:

~> 1.11.0

(This will select 1.11.7, which requires Ruby >= 2.5)

We can also change the minimum version of Ruby in .github/workflows/ci.yml

I'm baffled why nokogiri is being so overly-specific about versions of Ruby that they support. It's making using the library very difficult. Fortunately, it's only for tests, so we could set per-Ruby version overrides in Gemfile.

Btw, we know Asciidoctor works fine with Ruby 3.1 because we test on it in CI. See https://github.com/asciidoctor/asciidoctor/blob/main/.github/workflows/ci.yml#L28

@flavorjones
Copy link
Contributor Author

I'm baffled why nokogiri is being so overly-specific about versions of Ruby that they support

Nokogiri drops support for versions of Ruby that are more than a year past end-of-life. I don't think this is an unreasonable policy. If you feel differently, please open an issue and we can discuss it!

@flavorjones
Copy link
Contributor Author

I'm happy to update this PR however you want -- however changing the CI matrix feels like a policy decision that maybe could be visited in a separate PR by the project maintainers?

Also feel free to close this without merging if it's not something you're interested in, no worries! Thanks for your time.

@KartikSoneji
Copy link

Btw, we know Asciidoctor works fine with Ruby 3.1 because we test on it in CI. See https://github.com/asciidoctor/asciidoctor/blob/main/.github/workflows/ci.yml#L28

Hmm. I'm getting an error on Windows, ruby 3.1.2p20 (2022-04-12 revision 4491bb740a) [x64-mingw-ucrt].

Also, the changelog specifies net/ftp was moved to a bundled gem.
How does that work with the current Gemfile?
image

@mojavelinux
Copy link
Member

mojavelinux commented Aug 23, 2022

I find it very hostile to Linux packagers to use hard version ranges, especially ones that are so tight. The reason we are in the situation we are in is because I got great lengths to make it easy on Linux packagers to create packages for Asciidoctor gems. The biggest problem I've run into over the years is the premature dropping of older versions of Ruby. Just because the Ruby project marks a version of Ruby as EOL doesn't mean it is not supported by a Linux vendor. (see https://pkgs.org/download/ruby) I have to play nice with them or else my gem won't be packaged. And they aren't going to move any faster.

I get saying that you need Ruby >= 2.5 or Ruby >= 2.7, but having the version range be Ruby >= 3.1 <= 3.2.dev is overkill IMO.

@mojavelinux
Copy link
Member

A side benefit is that Nokogiri >= 1.11 ships precompiled native gems for most platforms.

This has bee a feature well before 1.11.

@flavorjones
Copy link
Contributor Author

having the version range be Ruby >= 3.1 <= 3.2.dev is overkill

Ah, this pattern is used ONLY in the precompiled native libraries (and the ranges are broader than you're representing, generally the last 4 ruby minors). The "ruby" platform library for Nokogiri will only give a minimum Ruby version. And modern bundler is smart enough to sort this all out for you.

@flavorjones
Copy link
Contributor Author

e.g., https://rubygems.org/gems/nokogiri/versions/1.13.8 supports ruby >= 2.6.0 but https://rubygems.org/gems/nokogiri/versions/1.13.8-x86-linux supports ruby >= 2.6, < 3.2.DEV because those are the versions that were precompiled.

@mojavelinux
Copy link
Member

e.g., rubygems.org/gems/nokogiri/versions/1.13.8 supports ruby >= 2.6.0 but rubygems.org/gems/nokogiri/versions/1.13.8-x86-linux supports ruby >= 2.6, < 3.2.DEV because those are the versions that were precompiled.

My mistake. I misread the metadata on rubygems.org. I now see it is Ruby >= 2.6 for the latest release.

@flavorjones
Copy link
Contributor Author

A side benefit is that Nokogiri >= 1.11 ships precompiled native gems for most platforms.

This has bee a feature well before 1.11.

Nokogiri < 1.11 only shipped precompiled for Windows. 1.11 and later also ships precompiled for linux and macos of various architectures.

@mojavelinux
Copy link
Member

I'm getting an error on Windows, ruby 3.1.2p20 (2022-04-12 revision 4491bb740a) [x64-mingw-ucrt].

At the time Ruby 3.1 was released, it was not available on windows-latest in GitHub Actions, so it is currently disabled. It looks like we can reenable it now.

However, I'm confused why it would not work on Windows but would work on Linux. The unbundling of gems hasn't been a smooth transition, but we'll get through it.

@mojavelinux
Copy link
Member

We have been planning to up the minimum version of Ruby to 2.5 in this branch. I'll apply that change separately. I'll also reenable the window-latest 3.1 job (and make any updates necessary to ensure that works).

We can then proceed with the change in this PR to set the version of nokogiri to ~> 1.11.0. If you'd like, you can wait until I've made those other updates to update the PR.

@mojavelinux
Copy link
Member

Nokogiri < 1.11 only shipped precompiled for Windows. 1.11 and later also ships precompiled for linux and macos of various architectures.

Good to know. And definitely a welcomed improvement for users.

@mojavelinux
Copy link
Member

It's important to keep in mind that nokogiri is only used in the test suite for this gem. None of this affects users of Asciidoctor. We only need to consider how it impacts what versions of Ruby we can test.

I have used exceptions for old versions of Ruby in the Gemfile in the past. If possible, I like to avoid doing that as it requires more maintenance. But ultimately I'll do what is necessary.

@flavorjones
Copy link
Contributor Author

Sure, hey, if it's easier to keep the gemspec as-is I won't feel bad if y'all close this without merging. I apologize if this has been an unwanted distraction.

@mojavelinux
Copy link
Member

mojavelinux commented Aug 23, 2022

I don't mind the tough conversation. We do periodically need be challenged about our setup and test matrix. Plus, we rediscovered that tests weren't running on Ruby 3.1 for Windows...so it has been fruitful. Some updates are going to come from this.

@KartikSoneji
Copy link

However, I'm confused why it would not work on Windows but would work on Linux. The unbundling of gems hasn't been a smooth transition, but we'll get through it.

Maybe it has something to do with this?

if: matrix.os == 'macos-latest' || matrix.os == 'ubuntu-latest'
run: echo 'BUNDLE_BUILD__NOKOGIRI=--use-system-libraries' >> $GITHUB_ENV

require mini_portile2 might not be called at all while building nokogiri.

@mojavelinux mojavelinux force-pushed the flavorjones-update-nokogiri-dependency branch from c2ad5c9 to c69e20d Compare August 24, 2022 09:17
@mojavelinux
Copy link
Member

I bumped the minimum version for Ruby to 2.5 and reenabled the Ruby 3.1 on Windows job. I'm now attempting to bump the version of Nokogiri, but there seems to be some unrelated issues. Whenever we touch the CI jobs, it's always a bit of a wrestling match, so just bear with me ;)

@mojavelinux
Copy link
Member

mojavelinux commented Aug 24, 2022

For context, one of the reasons I'm always relunctant to upgrade Nokogiri is because it usually ends up taking me a couple of hours to figure out how to get it to run on our full matrix. I just discovered that the precompiled binaries are not available for TruffleRuby, which is one of the Ruby implementations we have to test on. I appreciate how hard it is to support the full spectrum of Ruby implementations since I have to go through it as well. But the fact remains that it takes a lot of time and attention to make this upgrade.

@mojavelinux mojavelinux force-pushed the flavorjones-update-nokogiri-dependency branch from c69e20d to f5e4880 Compare August 24, 2022 09:40
@mojavelinux mojavelinux changed the title dep: loosen the development dependency on Nokogiri upgrade Nokogiri to latest version and add overrides for older Ruby versions Aug 24, 2022
@mojavelinux
Copy link
Member

mojavelinux commented Aug 24, 2022

It looks like we have something that's going to work. We need some overrides, but those should only ever be used in CI (and for packagers).

@mojavelinux mojavelinux merged commit 3ebedff into asciidoctor:main Aug 26, 2022
@mojavelinux
Copy link
Member

Thanks again!

@flavorjones flavorjones deleted the flavorjones-update-nokogiri-dependency branch August 27, 2022 16:48
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

4 participants