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.

Sign up for GitHub

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

Jump to bottom

Native gems: eliminate the "extra" dll #2076

Closed
flavorjones opened this issue Sep 8, 2020 · 2 comments
Closed

Native gems: eliminate the "extra" dll #2076

flavorjones opened this issue Sep 8, 2020 · 2 comments
Labels
packaging/native-gem release-blocker Blocking a milestone release
Milestone
v1.11.0

Comments

@flavorjones
Copy link
Member

When building a native Linux or OSX gem, we package a fifth nokogiri.so or nokogiri.bundle that appears to be generated by the "host" ruby (not "cross-compiled").

This additional shared library file should not be present.

Related to #2075
@flavorjones flavorjones added this to the v1.11.0 milestone Sep 8, 2020
@flavorjones flavorjones mentioned this issue Sep 8, 2020
Closed
@flavorjones
Copy link
Member Author

I looked into this a few weeks back and forgot to dump my notes here. The TL;DR is that the RakeCompilerExtension task, when set to cross-build mode, is iterating through all of the .cross_rubies versions, but also builds the "local" platform version. It wasn't immediately obvious to me how to patch rake-compiler to fix this, but maybe @larskanis has some ideas?

@flavorjones flavorjones added the release-blocker Blocking a milestone release label Dec 1, 2020
@flavorjones flavorjones mentioned this issue Dec 2, 2020
Closed
flavorjones added a commit that referenced this issue Dec 2, 2020
@flavorjones
fix: native gems are built with ExtensionTask.no_native=true
9e38a0d 
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 (#2089)
  - 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).
flavorjones added a commit that referenced this issue Dec 2, 2020
@flavorjones
fix: native gems are built with ExtensionTask.no_native=true
0336d36 
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).
@flavorjones flavorjones mentioned this issue Dec 2, 2020
Merged
flavorjones added a commit that referenced this issue Dec 3, 2020
@flavorjones
fix: native gems are built with ExtensionTask.no_native=true
c50a658 
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).
flavorjones added a commit that referenced this issue Dec 3, 2020
@flavorjones
fix: native gems are built with ExtensionTask.no_native=true
17c84fc 
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).
flavorjones added a commit that referenced this issue Dec 3, 2020
@flavorjones
Merge pull request #2125 from sparklemotion/2078-fix-rake-compiler-na…
b54c3fb 
…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
Copy link
Member Author

#2125 fixed this, but I'd like to add test coverage before closing.

flavorjones added a commit that referenced this issue Dec 22, 2020
@flavorjones
test: scripts to assert on packaged gem files and installed gems
8118e97 
some tests are skipped because they assert new behavior which will be
implemented in the next few commits.

Closes #2076
Closes #2078
@flavorjones flavorjones mentioned this issue Dec 23, 2020
Merged
flavorjones added a commit that referenced this issue Dec 23, 2020
@flavorjones
Merge pull request #2143 from sparklemotion/flavorjones-fine-tune-pac…
813d119 
…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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
packaging/native-gem release-blocker Blocking a milestone release
Projects
None yet
Milestone
v1.11.0
Development

No branches or pull requests
1 participant
@flavorjones