Nokogumbo should be able to compile against Nokogiri by finding headers in extensions directory #1788
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
I just noticed that |
This was referenced Aug 22, 2018
Closed
|
@stevecheckoway Apologies for my slow reply and lack of attention. This PR looks good, I need to resolve the merge conflicts but will adopt your testing approach. I've got it from here! I would like to block this PR on #1845, though, which is how I'd like to test gem installation. And then this is targetted for release with v1.11.0 when it ships (see the milestone). |
|
Update: Things to do to make nokogumbo work well with Nokogiri v1.11.x+:
Of course, we will add a CI task to test installing nokogumbo against the gem release under test to drive the above out. NOTE that nokogumbo refuses to install when built against the Nokogiri v1.11.0 prereleases because of the absence of |
This was referenced Sep 1, 2020
flavorjones
changed the title
This was referenced Dec 1, 2020
flavorjones
added a commit
that referenced
this pull request
Dec 3, 2020
…tive-things native gems are built with `ExtensionTask.no_native=true` --- **What problem is this PR intended to solve?** Specifically, following the advice of @kou at rake-compiler/rake-compiler#171, we set `Rake::ExtensionTask.no_native=true` within the rake-compiler-dock container ("guest") so that: - the ExtensionTask `cross_compiling` block is called - so that we don't package the dependencies' tarballs in /ports - so that we don't have mini_portile2 as a dependency (#2078) - so that we don't have an extra nokogiri.so/nokogiri.bundle built and packaged (#2076) - we no longer have to hotfix rake-compiler at build time This also will enable us to more easily do things like removing the C extension source code from the native gem package (#2077). This patch set also breaks out `lib/nokogiri/version.rb` into two new files: - `lib/nokogiri/version/constant.rb` - `lib/nokogiri/version/info.rb` and `require_relative`s them both from `version.rb`. This is so that Hoe doesn't pull in `REQUIRED_LIBXML_VERSION` from `extconf.rb` after 652c6fd extracted that value into a constant. This patch set also updates how Darwin native gems are built, to mirror the same rake task structure that's used for Linux and Windows; and it renames to "builder" the rake tasks that used to be "guest". **Have you included adequate test coverage?** I'm satisfied with the level of testing we have now on different platforms, though it could always be better. I will add some testing to the packaged/installed gem when I merge #1788 which introduces that test coverage pretty nicely. **Does this change affect the behavior of either the C or the Java implementations?** Should only impact how the native (precompiled) gems are built and packaged.
flavorjones
added a commit
that referenced
this pull request
Dec 22, 2020
This addresses part of #1788
flavorjones
added a commit
that referenced
this pull request
Dec 22, 2020
Also, restore VERSION_INFO["libxml"]["libxml2_path"] when using packaged libraries, so Nokogumbo can find all the headers. Fixes #1788
Merged
flavorjones
added a commit
that referenced
this pull request
Dec 23, 2020
…kaging fine tune packaging --- **What problem is this PR intended to solve?** There are a few open issues and pull requests about the packaging and installation of Nokogiri which this PR is trying to address: - [Native gems: eliminate the "extra" dll · Issue #2076 · sparklemotion/nokogiri](#2076) - [Native gems: ensure that mini_portile2 is not a dependency · Issue #2078 · sparklemotion/nokogiri](#2078) - [Nokogumbo should be able to compile against Nokogiri by finding headers in extensions directory by stevecheckoway · Pull Request #1788 · sparklemotion/nokogiri](#1788) More specifically, the code that's been doing the gem packaging is ad-hoc, brittle, and not very discoverable, and has little-to-no test coverage. This PR embarks on cleaning up some of that code, and erecting tests around the files that are packaged and installed, so that I have the confidence to refactor and update this code going forward. I want to give a shoutout to @stevecheckoway whose original PR at #1788 inspired this approach, though unfortunately I didn't use most of that PR because the precompiled native gems, added since that PR, complicated things. **Have you included adequate test coverage?** Ayuh. Notably two new scripts have been added and will be run in CI: - `scripts/test-gem-file-contents` which makes assertions on the contents of the tarball and the packaged gemspec - `scripts/test-gem-installation` which asserts on the filesystem left behind by the gem installation process **Does this change affect the behavior of either the C or the Java implementations?** This affects only installation-time, the functional behavior of Nokogiri is not changed.
|
@stevecheckoway I've made commits in #2143 to fix this, and I've tested against nokogumbo and it seems like everything's working to me. Likely will cut a v1.11.0.rc4 today or tomorrow for some more tire kicking ahead of a final release by the end of the year. |
|
Great! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is an alternative implementation to #1325. The major differences are
nokogiri/includeinside the nokogiri extension directory rather than directly innokogiri.libxml2andlibxsltheaders are placed innokogiri/include/libxml2andnokogiri/include/libxslt(andnokogiri/include/libexslt) if nokogiri used the packaged rather than system libraries.This will assist downstream projects (like nokogumbo) which need to build against a matching version of these libraries.
cc: @flavorjones