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: --disable-rubygems for baseruby #61684

Merged
merged 2 commits into from Aug 31, 2019

Conversation

alyssais
Copy link
Member

Motivation for this change

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).

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 nix-review --run "nix-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](https://github.com/NixOS/nixpkgs/blob/master/.github/CONTRIBUTING.

Cc: @immae @samueldr @Mic92

@alyssais alyssais requested a review from zimbatm as a code owner May 18, 2019 17:51
@ofborg ofborg bot added the 6.topic: ruby label 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).
@immae
Copy link
Contributor

immae commented May 18, 2019

@alyssais Thanks for your fix. Just so that I can test properly, can you please confirm that your fix is another solution to what I suggested (i.e. removing the --destdir) as a temporary workaround?

@alyssais alyssais mentioned this pull request May 18, 2019
10 tasks
@alyssais
Copy link
Member Author

alyssais commented May 18, 2019 via email

@samueldr
Copy link
Member

At a glance, this looks safe for backporting to 19.03, since its rubies are exhibiting this issue. Do you think this is wrong?

@alyssais
Copy link
Member Author

alyssais commented May 18, 2019 via email

@samueldr
Copy link
Member

Hmm, having an issue. Ruby 2.6's native bundler apparently doesn't play well with that change.

Repro

~/tmp/tmp/repro $ nix-build --keep-going -I nixpkgs=/Users/samuel/tmp/nixpkgs/nixpkgs-PR61684
these derivations will be built:
  /nix/store/qkn0f4hiqkii9j11xnk4kjrw5mgjc4x9-test-with-ruby_2_6.drv
building '/nix/store/qkn0f4hiqkii9j11xnk4kjrw5mgjc4x9-test-with-ruby_2_6.drv' on 'ssh://samuel@duffman'...
ruby 2.6.3p62 (2019-04-16) [x86_64-linux]
`/homeless-shelter` is not a directory.
Bundler will use `/build/bundler/home/unknown' as your home directory temporarily.
Bundler version 1.17.2
/nix/store/s7x3gaf88bbi7p647jpbrbc29249f65m-ruby-2.6.3/lib/ruby/site_ruby/2.6.0/rubygems.rb:283:in `find_spec_for_exe': can't find gem bundler (>= 0.a) with executable bundler (Gem::GemNotFoundException)
        from /nix/store/s7x3gaf88bbi7p647jpbrbc29249f65m-ruby-2.6.3/lib/ruby/site_ruby/2.6.0/rubygems.rb:302:in `activate_bin_path'
        from /nix/store/s7x3gaf88bbi7p647jpbrbc29249f65m-ruby-2.6.3/bin/bundler:23:in `<main>'
builder for '/nix/store/qkn0f4hiqkii9j11xnk4kjrw5mgjc4x9-test-with-ruby_2_6.drv' failed with exit code 1
error: build of '/nix/store/qkn0f4hiqkii9j11xnk4kjrw5mgjc4x9-test-with-ruby_2_6.drv' on 'ssh://samuel@duffman' failed: builder for '/nix/store/qkn0f4hiqkii9j11xnk4kjrw5mgjc4x9-test-with-ruby_2_6.drv' failed with exit code 1
builder for '/nix/store/qkn0f4hiqkii9j11xnk4kjrw5mgjc4x9-test-with-ruby_2_6.drv' failed with exit code 1
error: build of '/nix/store/qkn0f4hiqkii9j11xnk4kjrw5mgjc4x9-test-with-ruby_2_6.drv' failed

The PATH order here matters. The other way around bundler comes from ruby_2_6. This means that if an end-user with ruby_2_6 could have a different bundler than expected. Additionally, I think the bundler shipped with ruby should work, otherwise it seems weird to me.

@immae
Copy link
Contributor

immae commented May 18, 2019

@samueldr : this is actually the error that I got with the destdir (bundler cannot find himself), but only with versions different from the latest one (see comment given in the description of the PR for details)

Does it work with other versions for you?

@samueldr
Copy link
Member

Yes, the default.nix when built only fails for 2.6 with the change; calling nix-build builds for 2_4, 2_5 and 2_6.

@immae
Copy link
Contributor

immae commented May 19, 2019

Ok, so it’s the exact reverse of the current state then

@alyssais
Copy link
Member Author

alyssais commented May 19, 2019 via email

This should prevent problems caused by trying to install our own
RubyGems over the top of the one that comes with Ruby.
@alyssais
Copy link
Member Author

alyssais commented May 19, 2019 via email

@samueldr
Copy link
Member

samueldr commented May 19, 2019

Oh, you're right it only fails with bundler. (Seems I never internalised there was bundle and bundler and that they could differ in behaviour?)

I'm have prepared a couple tests to see how the behaviour compares, it is descriptive of the situation, and not necessarily prescriptive; meaning that you do what you feel is right with the data.

Though, I am not confident that the test tests @immae's case.


The stuff required to run it:

Results:


My take on the results:

  • Between 18.09 and 19.03 there was a change that made bundle not work with the non-default ruby. (Excluding 2.6.)
  • The PR currently doesn't seem to cause regressions for using bundle and bundler imperatively compared to the current state of nixos-unstable.

Additionally, I do not know what should be happening with bundler vs bundle.

@alyssais
Copy link
Member Author

alyssais commented May 19, 2019 via email

@alyssais
Copy link
Member Author

alyssais commented May 19, 2019 via email

@alyssais
Copy link
Member Author

The current behaviour (as of the tip of this branch), which is that a Ruby elsewhere in the environment does not affect an application just because it happens to be written in Ruby, is desirable in almost all cases. The problem is just that, with Bundler, we actually want the exact opposite behaviour — it should use whatever Ruby is in the environment.

In the cases @samueldr has found where this works, it works because of a Bundler executable bundled with Ruby that finds and executes Bundler from Ruby’s LOAD_PATH. In these cases, Ruby and Bundler both have bundle executables, and the Ruby one wins purely by having a Nix store path that beats Bundler’s alphabetically. When run, assuming Bundler is loaded after Ruby (in arguments to nix-shell, for example), Ruby’s Bundler executable will load Bundler from the Bundler derivation, not from the Ruby one. This produces the desirable behaviour.

To have this work perfectly, we would need a way to ensure that nix-shell -p ruby_2_3 bundler put Ruby’s Bundler executable first in PATH, but set RUBYLIB with the Bundler derivation’s lib directory.

@zimbatm
Copy link
Member

zimbatm commented May 20, 2019

nice detective work yall :)

I think I'm the one that installed rubygems directly into the ruby installation. The reason was that rubygems had a security vulnerability and I wanted to make sure that all the ruby users would have the latest version available.

Regarding the priority, how about marking bundler with lib.hiPrio? bundlerEnv should respect that and prioritize it.

@alyssais
Copy link
Member Author

I think I'm the one that installed rubygems directly into the ruby installation. The reason was that rubygems had a security vulnerability and I wanted to make sure that all the ruby users would have the latest version available.

Nowadays Ruby upstream issues version bumps for Rubygems vulns (e.g. 2.6.2), so I think I'd like to go back to shipping the default version of RubyGems with Ruby, and making a seperate RubyGems package that could be used to get latest. Out of scope of this PR though.

Regarding the priority, how about marking bundler with lib.hiPrio? bundlerEnv should respect that and prioritize it.

In bundlerEnv, I don't think this is an issue, because it only exposes the gem executables (no Ruby anything). bundlerEnv's .env can be fixed by swapping the order of nativeBuildInputs.

But the case I'm really thinking of is this:

nix-shell -p ruby -p bundler

If you can get one where Ruby beats Bundler because it's hash is sorted after Bundler's, it doesn't make a difference if you do

nix-shell -p ruby -p 'hiPrio bundler'

You still get Ruby's Bundler.

Any ideas?

@samueldr
Copy link
Member

This makes me think, my initial testing (never shared here) with nix-shell failed because I had a bundler from a bundlerApp, hue-cli. This is not limited to that app, see nix-locate -1 --at-root --regex '/bin/bundler?'.

Just saying in case it is related or relevant to the current issue with the bundler executable being overriden.

@FRidh
Copy link
Member

FRidh commented Jun 18, 2019

What's the status of this PR?

@alyssais
Copy link
Member Author

What's the status of this PR?

Looks like it’s been incorporated into #61114. I still want to get the ruby and bundler derivations playing nice together. It looks from that PR that @manveru might have found a way to do it…

@alyssais
Copy link
Member Author

alyssais commented Aug 31, 2019 via email

@alyssais alyssais merged commit 01c4c15 into NixOS:staging Aug 31, 2019
@alyssais alyssais deleted the baseruby-disable-rubygems branch August 31, 2019 11:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants