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

Call cross_compiling block for native platform too #171

Closed
wants to merge 1 commit into from

Conversation

larskanis
Copy link
Member

Currently the cross_compiling block isn't called when cross compiling for the build platform.
This is due to the fact that define_native_tasks() is called first for the build platform
and later on for the cross platforms. The task "native::" is defined
only once
and therefore only without the callback. The callback is therefore not called
for the cross platform that equals the build platform.

OMHO the root issue is that rake-compiler doesn't has a clear separation between
native and cross tasks. However I think calling cross_compiling for any binary gem
(native or cross) is a suitable workaround.

This fixes bug sparklemotion/nokogiri#1991 (comment)

Currently the cross_compiling block isn't called when cross compiling for the build platform.
This is due to the fact that define_native_tasks() is called first for the build platform
and later on for the cross platforms. The task "native:<gemname>:<platform>" is defined
only once and therefore only without the callback. The callback is therefore not called
for the cross platform that equals the build platform.

OMHO the root issue is that rake-compiler doesn't has a clear separation between
native and cross tasks. However I think calling cross_compiling for any binary gem
(native or cross) is a suitable workaround.

This fixes bug sparklemotion/nokogiri#1991 (comment)
larskanis added a commit to larskanis/nokogiri that referenced this pull request Mar 3, 2020
The cross_compiling block wasn't called for the x86_64-linux platform
leading to an additional dependency to mini_portile2 and missing
post install message.

Patch to rake-compiler is here: rake-compiler/rake-compiler#171

Fixes sparklemotion#1991
flavorjones added a commit to sparklemotion/nokogiri that referenced this pull request Mar 31, 2020
and comment the two places we now have this code with the original
github issue rake-compiler/rake-compiler#171

[skip ci]
@kou
Copy link
Member

kou commented Jul 9, 2020

Sorry for late response...
Could you show how to reproduce this with Nokogiri?

@larskanis
Copy link
Member Author

Build on x86_64-linux:

$ git clone https://github.com/sparklemotion/nokogiri
$ cd nokogiri
$ git checkout 33e5b426f099da152904617e4e0be3c63df1f5dd~ # commit before the above fix
$ bundle
$ rake gem:linux
$ gem inst -l pkg/nokogiri-1.11.0.rc1-x86-linux.gem 
Nokogiri is built with the packaged libraries: libxml2-2.9.10, libxslt-1.1.34, zlib-1.2.11, libiconv-1.15.
Successfully installed nokogiri-1.11.0.rc1-x86-linux

$ gem inst -l pkg/nokogiri-1.11.0.rc1-x86_64-linux.gem 
Successfully installed nokogiri-1.11.0.rc1-x86_64-linux

Note the missing message Nokogiri is built with the packaged libraries of the x86_64-linuxversion. This is because the cross_compiling hook wasn't called.

@flavorjones
Copy link
Contributor

flavorjones commented Dec 1, 2020

Hi, I'm still struggling with this issue in Nokogiri, see sparklemotion/nokogiri#2078 for context and help seeing the problem in the codebase.

I'd like to gently ask that this be merged, so that I can reliably build native gems for Nokogiri in the upcoming v1.11.0 release (that will coincide with Ruby 3). Thank you!

@kou
Copy link
Member

kou commented Dec 2, 2020

Sorry for my lazy response again...

I could reproduce this by #171 (comment) . Thanks.

The cross_compiling hook isn't called because it's not cross compiling. rake-compiler-dock uses x86_64-linux environment. So building gem for x86_64-linux isn't cross compiling.

How about using Rake::ExtensionTask#no_native=?

diff --git a/Rakefile b/Rakefile
index 6c740a90..7b9d5f44 100644
--- a/Rakefile
+++ b/Rakefile
@@ -128,6 +128,7 @@ else
   Rake::ExtensionTask.new("nokogiri", HOE.spec) do |ext|
     ext.lib_dir = File.join(*['lib', 'nokogiri', ENV['FAT_DIR']].compact)
     ext.config_options << ENV['EXTOPTS']
+    ext.no_native = (ENV["FORCE_CROSS_COMPILING"] == "true")
     ext.cross_compile  = true
     ext.cross_platform = CROSS_RUBIES.map(&:platform).uniq
     ext.cross_config_options << "--enable-cross-build"
diff --git a/tasks/cross-ruby.rb b/tasks/cross-ruby.rb
index b77d7b43..78cc6fd8 100644
--- a/tasks/cross-ruby.rb
+++ b/tasks/cross-ruby.rb
@@ -166,7 +166,7 @@ namespace "gem" do
       RakeCompilerDock.sh <<-EOT, platform: plat
         gem install bundler --no-document &&
         bundle &&
-        rake native:#{plat} pkg/#{HOE.spec.full_name}-#{plat}.gem MAKE='nice make -j`nproc`' RUBY_CC_VERSION=#{ENV["RUBY_CC_VERSION"]}
+        rake native:#{plat} pkg/#{HOE.spec.full_name}-#{plat}.gem MAKE='nice make -j`nproc`' RUBY_CC_VERSION=#{ENV["RUBY_CC_VERSION"]} FORCE_CROSS_COMPILING=true
       EOT
     end
   end

@flavorjones
Copy link
Contributor

@kou Thank you for responding!

I tried your suggestion, and ✨ it totally worked ✨. It also addressed sparklemotion/nokogiri#2076 which resulted in a "native" nokogiri.so being generated and packaged in addition to the "cross-compiled" libraries.

I'm going to play with this a bit more to make sure it's totally working the way I expect, but I think you've solved multiple problems for us. Thank you! 💕

flavorjones added a commit to sparklemotion/nokogiri that referenced this pull request Dec 2, 2020
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 to sparklemotion/nokogiri that referenced this pull request Dec 2, 2020
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).
@kou
Copy link
Member

kou commented Dec 3, 2020

Thanks for confirming it.
Can we close this?

@flavorjones
Copy link
Contributor

@kou If you'd like to close, OK. I'm pretty confident I can get sparklemotion/nokogiri#2125 to pass CI.

Thank you again!

@kou
Copy link
Member

kou commented Dec 3, 2020

OK.
Closing.

@kou kou closed this Dec 3, 2020
flavorjones added a commit to sparklemotion/nokogiri that referenced this pull request Dec 3, 2020
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 to sparklemotion/nokogiri that referenced this pull request Dec 3, 2020
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 to sparklemotion/nokogiri 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.
larskanis added a commit to rake-compiler/rake-compiler-dock that referenced this pull request Dec 20, 2020
@larskanis
Copy link
Member Author

@kou Thank you for the hint to use no_native=true! I added this to the rake-compiler-dock readme in rake-compiler/rake-compiler-dock@8a0c7ce .

What do you think about adding this directly to rake-compiler? The environment variable RCD_HOST_RUBY_PLATFORM is always defined by rake-compiler-dock.

@larskanis larskanis deleted the cross_compiling branch December 20, 2020 12:03
@kou
Copy link
Member

kou commented Dec 20, 2020

OK. You can use RAKE_EXTENSION_TASK_NO_NATIVE=true with 15f630a.

@larskanis
Copy link
Member Author

Looks good - thanks!

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

3 participants