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

ruby: fix cross-build #51842

Merged
merged 1 commit into from Dec 11, 2018
Merged

ruby: fix cross-build #51842

merged 1 commit into from Dec 11, 2018

Conversation

Mic92
Copy link
Member

@Mic92 Mic92 commented Dec 10, 2018

Enable autoreconfHook by default: The build tried to execute autoconf, which was
in the wrong build input. Regenerating autotools configure files is always a good
idea since it delivers fixes.
Also move groff to the native since it is only used at build-time

Motivation for this change
Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Assured whether relevant documentation is up to date
  • Fits CONTRIBUTING.md.

Enable autoreconfHook by default: The build tried to execute autoconf, which was
in the wrong build input. Regenerating autotools configure files is always a good
idea since it delivers fixes.
Also move groff to the native since it is only used at build-time
@Mic92 Mic92 requested a review from zimbatm as a code owner December 10, 2018 19:37
@@ -149,7 +145,7 @@ let
postInstall = ''
# Update rubygems
pushd rubygems
${buildRuby} setup.rb
${buildRuby} setup.rb --destdir $GEM_HOME
Copy link
Member Author

@Mic92 Mic92 Dec 10, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without --destdir it tries to install gemspec files in buildRuby.

@Mic92 Mic92 added the 6.topic: cross-compilation Building packages on a different sort platform than than they will be run on label Dec 10, 2018
Copy link
Member

@zimbatm zimbatm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice! I was bracing myself for a big patch but it's surprisingly simple.

@zimbatm zimbatm merged commit 10ba787 into NixOS:staging Dec 11, 2018
@Mic92 Mic92 deleted the cross-ruby branch December 11, 2018 11:04
@Mic92
Copy link
Member Author

Mic92 commented Dec 11, 2018

The cross-build existed already before but was just broken. buildRubyGems will be more complex since we would need something like https://github.com/rake-compiler/rake-compiler

@immae
Copy link
Contributor

immae commented Mar 31, 2019

After a very long git bisect session, I found that this commit, more specifically the --destdir $GEM_HOME is responsible for breaking bundlerEnv in cases where the ruby (minor) version is not the latest one. I tested it with https://git.immae.eu/?p=perso/Immae/Config/Nix.git;a=tree;f=nixops/modules/websites/tools/diaspora;hb=61199e9367515ec28f2a24903ea12a7b6747c9c0 , but all my bundler applications in a similar situation (not the latest ruby version) broke at once after an upgrade, and reverting the destdir addition makes them work again ( https://git.immae.eu/?p=perso/Immae/Config/Nix.git;a=commitdiff;h=3345e58db4e8364a9561dc3d488da2438aca982a ).

The error only happens at runtime (in the systemd service, defined in default.nix in my example), while similar commands at build time (for instance railsRoot in diaspora.nix) work correctly. I tried multiple options and flags in vain, but I may have missed the "right" way to use bundlerEnv that would make it work...

Before reporting it as a regression issue, I’d appreciate a confirmation that it is actually a regression and not just me misusing  bundlerEnv.

@samueldr
Copy link
Member

samueldr commented Apr 3, 2019

I'm hitting an issue relating to this change. Before this change, bundler was made available in the output, while after the change it doesn't. It seems related to how the bundled gems (site_ruby) stuff is now in probably erroneously nested subpaths of the output. See the following diff of the outputs with the commit, and with it reverted.

Though, I'm unsure whether this is really an issue or not, but it breaks what was previousy working, and (while quite late) the backport to 18.09 changes its behaviour compared to the branch off point.

Should bundle have been available there? Does it make sense when the globally available bundler might not be using the same ruby as a specific one? (Though other than closure size issues, not sure it's an issue.)

Though, one thing is clear, this breaks assumptions between 18.03 and 18.09 and apparently broke an assumption between the release of 18.09 and now :/.

@immae
Copy link
Contributor

immae commented Apr 3, 2019

@samueldr the symptom is the same for me, but I think it comes from hardcoded paths somewhere (see my comment above your). Does it appear only when you use a version of bundler that is different from the latest (currently 2.6) one? If so, you should try the "patch" that I quote there: #54303 (comment)

@samueldr
Copy link
Member

samueldr commented Apr 3, 2019

@immae I'm not using it with bundlerEnv, it's for providing bundler as an executable on the CLI, so the issue for me is the disappearance of bundler. Though, it might also be closely related to your issue, I just haven't really checked.

@samueldr
Copy link
Member

samueldr commented Apr 3, 2019

I actually jumped the gun here. I wasn't done with the complete testing and assumed that bundler + bundler deps installing would be fine.

With this merged, and a buildEnv:

buildEnv {
  name = "ruby-with-bundler";
  paths = [ bundler ruby ];
};

Installing using bundle install works fine as it did previously. Running the ruby thing doesn't.

Traceback (most recent call last):
        2: from bin/cap:15:in `<main>'
        1: from /nix/store/rwp5fpzqssf5m9dzbgbwsfgdzw8xajra-ruby-2.5.5/lib/ruby/2.5.0/rubygems/core_ext/kernel_require.rb:59:in `require'
/nix/store/rwp5fpzqssf5m9dzbgbwsfgdzw8xajra-ruby-2.5.5/lib/ruby/2.5.0/rubygems/core_ext/kernel_require.rb:59:in `require': cannot load such file -- bundler/setup (LoadError)

While with this reverted locally it works fine. Ruby 2.6 (without this reverted) works fine too. This makes me think there really is a regression here.

@samueldr
Copy link
Member

samueldr commented Apr 3, 2019

Tried a build without --destdir $GEM_HOME and the native build's file listing is not identical to the previously working one.

diff --git a/pkgs/development/interpreters/ruby/default.nix b/pkgs/development/interpreters/ruby/default.nix
index e7aedfc0750..4d1775db8c7 100644
--- a/pkgs/development/interpreters/ruby/default.nix
+++ b/pkgs/development/interpreters/ruby/default.nix
@@ -146,7 +146,7 @@ let
         postInstall = ''
           # Update rubygems
           pushd rubygems
-          ${buildRuby} setup.rb --destdir $GEM_HOME
+          ${buildRuby} setup.rb
           popd

           # Remove unnecessary groff reference from runtime closure, since it's big

@immae if you're interested in trying.

Waiting on a cross aarch64 build to see. how it reacts.

Though, Quoting @Mic92

Without --destdir it tries to install gemspec files in buildRuby.

It is an issue, it won't build without --destdir; though confirming that a cross-built ruby will have the same deeply nested paths.


Here's where I'm at: making --destdir conditional to cross-compilation is not the way to go, since it would (AFAIUI) cause the same gemdir breakage, but only when cross-compiling. Played around with GEM_HOME and --destdir and it really looks like there's a bad interaction with setting both of them.

@samueldr
Copy link
Member

Sorry to bump this, but wondering if anyone took a look and see what's going on.

@alyssais
Copy link
Member

alyssais commented Apr 27, 2019 via email

@samueldr
Copy link
Member

What seems to happen, is that since this patch added --destdir $GEM_HOME, bundler/setup cannot be found when required. In my use case, it's for standard bundle install imperative use when working on a ruby codebase. It seems to also affect bundlerEnv, at least according to #54303, where @immae found the same cause to the issue.

This seems to have been caused by how gems are installed into what looks like an incorrectly nested absolute path:

-result/lib/ruby/gems/2.5.0/gems/bundler-1.16.2
+result/lib/ruby/gems/2.5.0/nix/store/rwp5fpzqssf5m9dzbgbwsfgdzw8xajra-ruby-2.5.5/lib/ruby/gems/2.5.0/nix/store/rwp5fpzqssf5m9dzbgbwsfgdzw8xajra-ruby-2.5.5/lib/ruby/gems/2.5.0/specifications/gems/bundler-1.16.2

@alyssais
Copy link
Member

Thanks very much — that gives me a much better place to start. I’ll look into this when I have a chance.

alyssais added a commit to alyssais/nixpkgs that referenced this pull request May 18, 2019
This works just fine, and means we don't run into an issue with RubyGems
trying to install into a different Ruby's prefix when cross-compiling.
See NixOS#51842 (comment).
alyssais added a commit to alyssais/nixpkgs that referenced this pull request May 24, 2019
This works just fine, and means we don't run into an issue with RubyGems
trying to install into a different Ruby's prefix when cross-compiling.
See NixOS#51842 (comment).
alyssais added a commit that referenced this pull request Aug 31, 2019
This works just fine, and means we don't run into an issue with RubyGems
trying to install into a different Ruby's prefix when cross-compiling.
See #51842 (comment).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: cross-compilation Building packages on a different sort platform than than they will be run on 6.topic: ruby 10.rebuild-darwin: 101-500 10.rebuild-linux: 501+
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants