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

[bug] require_relative requires binary extention in unexpected location #2300

Closed
ojab opened this issue Aug 3, 2021 · 7 comments
Closed

Comments

@ojab
Copy link

ojab commented Aug 3, 2021

Please describe the bug

In general binary extensions are residing in a separate directory that is added to $LOAD_PATH, so it works fine. But
0cee8f0#diff-a610b0ddcdf14966b304b7f6ffc87a1c5866f232f995a0a27d1618b23658b12aR25 changed require (which searched this directory) to require_relative, which looks only in gems/nokogiri-*/lib/nokogiri.

Currently after build on ruby platform we have

$ find vendor/bundle/ -name nokogiri.so
vendor/bundle/ruby/3.0.0/extensions/x86_64-linux/3.0.0/nokogiri-1.12.0/nokogiri/nokogiri.so
vendor/bundle/ruby/3.0.0/gems/nokogiri-1.12.0/ext/nokogiri/nokogiri.so
vendor/bundle/ruby/3.0.0/gems/nokogiri-1.12.0/lib/nokogiri/nokogiri.so

so it works fine.

But for size-cutting some people are using find ${LOCAL_BUNDLE_PATH}/gems/ -name '*.so' -delete that removes seemingly excessive binary exts, after that we have .so only in extension_dir, so require_relative from gems/nokogiri-*/lib/nokogiri/extension.rb fails, even though we have ext in $LOAD_PATH.
I can provide reproducer Dockerfile if needed and I can make a PR that reverts this particular require_relative if it's acceptable.

Expected behavior

Nokogiri works fine when binary ext is removed from gems directory.

Environment

Nokogiri (1.12.0) for ruby platform built for x86_86, docker image build from ruby:3.0.2-slim with find ${LOCAL_BUNDLE_PATH}/gems/ -name '*.so' -delete after gems installation.

@ojab ojab added the state/needs-triage Inbox for non-installation-related bug reports or help requests label Aug 3, 2021
@flavorjones
Copy link
Member

@ojab Thanks for reporting this, and I'm sorry if you're having problems.

Can you help me understand why "some people" are doing this? Specifically, is this officially recommended somewhere to maintain smaller install sizes? I do want to make sure Nokogiri is following best practices for C extensions, and I feel like I need to understand this a bit better.

@flavorjones
Copy link
Member

Ah, hmm, OK, I'm reading through the rubygems code and I'm just honestly confused why C extension files are being copied around to multiple places. But I agree the presence of extension_dir in full_require_paths (see https://github.com/rubygems/rubygems/blob/master/lib/rubygems/basic_specification.rb#L152) means it should be the canonical location for extension files.

@ojab
Copy link
Author

ojab commented Aug 3, 2021

…and I stopped writing reply after you reply 😄
I suppose it's used because you can have multiple architectures in one archive/filesystem, so they don't clash, but I'm surely not authority here, it's just observation that every gem has binary ext in extensions/x86_64-linux/ and most of the gems has copy somewhere in gems/.

@flavorjones
Copy link
Member

flavorjones commented Aug 3, 2021

@ojab Can you reply with the output from nokogiri -v on your platform?

I'm inferring that you're not using the precompiled native gem, and I'm curious a) if that's true, and b) why you're choosing to compile rather than use the precompiled package? (Regardless, we should fix this, but I'm curious.)

@ojab
Copy link
Author

ojab commented Aug 3, 2021

It's official ruby docker image (i. e. debian slimmed down a bit), nothing special:

$ bundle exec nokogiri -v
# Nokogiri (1.12.0)
    ---
    warnings: []
    nokogiri:
      version: 1.12.0
      cppflags:
      - "-I/app/vendor/bundle/ruby/3.0.0/gems/nokogiri-1.12.0/ext/nokogiri"
      - "-I/app/vendor/bundle/ruby/3.0.0/gems/nokogiri-1.12.0/ext/nokogiri/include"
      - "-I/app/vendor/bundle/ruby/3.0.0/gems/nokogiri-1.12.0/ext/nokogiri/include/libxml2"
      ldflags: []
    ruby:
      version: 3.0.2
      platform: x86_64-linux
      gem_platform: x86_64-linux
      description: ruby 3.0.2p107 (2021-07-07 revision 0db68f0233) [x86_64-linux]
      engine: ruby
    libxml:
      source: packaged
      precompiled: false
      patches:
      - 0001-Remove-script-macro-support.patch
      - 0002-Update-entities-to-remove-handling-of-ssi.patch
      - 0003-libxml2.la-is-in-top_builddir.patch
      - 0004-use-glibc-strlen.patch
      - 0005-avoid-isnan-isinf.patch
      - 0006-update-automake-files-for-arm64.patch
      - 0007-Fix-XPath-recursion-limit.patch
      libxml2_path: "/app/vendor/bundle/ruby/3.0.0/gems/nokogiri-1.12.0/ext/nokogiri"
      memory_management: ruby
      iconv_enabled: true
      compiled: 2.9.12
      loaded: 2.9.12
    libxslt:
      source: packaged
      precompiled: false
      patches:
      - 0001-update-automake-files-for-arm64.patch
      - 0002-Fix-xml2-config-check-in-configure-script.patch
      datetime_enabled: true
      compiled: 1.1.34
      loaded: 1.1.34
    other_libraries:
      libgumbo: 1.0.0-nokogiri

Precompiled nokogiri also carries multiple copies of binary ext (just for different ruby versions). It's because of obsession with the docker image size, with single copy we're saving 4% of microservice space (final image is around 170 MB currently).

@flavorjones
Copy link
Member

flavorjones commented Aug 3, 2021

@ojab OK, just wanted to verify that you're intentionally opting in to compiling during installation. If you wanted the best of both worlds (speed and size), you could delete the shared library files for versions of Ruby that you don't care about.

In any case. #2301 is a PR that should fix this, going through CI now, and I'll release a patch version of v1.12 when it's green.

@flavorjones flavorjones added this to the v1.12.x patch releases milestone Aug 3, 2021
@flavorjones flavorjones added topic/installation Installation difficulties packaging/native-gem packaging/compile-from-source and removed state/needs-triage Inbox for non-installation-related bug reports or help requests topic/installation Installation difficulties packaging/native-gem labels Aug 3, 2021
@flavorjones
Copy link
Member

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants