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

bundler: 1.17.3 -> 2.1.4 #81442

Merged
merged 3 commits into from
Apr 8, 2020
Merged

bundler: 1.17.3 -> 2.1.4 #81442

merged 3 commits into from
Apr 8, 2020

Conversation

manveru
Copy link
Contributor

@manveru manveru commented Mar 1, 2020

Motivation for this change

Bundler in a wrappedRuby was broken. Bundler is pretty outdated, so this might fix other issues too (and cause more... who knows :)

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • 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 nixpkgs-review --run "nixpkgs-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)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@aanderse
Copy link
Member

aanderse commented Mar 1, 2020

@GrahamcOfBorg test redmine.mysql

@thefloweringash
Copy link
Member

@manveru did you have any success using the bundlerEnv, etc, with this change? As I understand it, nix's ruby infrastructure doesn't yet handle bundler 2. Hopefully the changes required are minimal.

The first thing I've found is that bundler seems to append a ruby_scope to the value of path (set via BUNDLER_PATH), that includes the ruby engine and version. Poking around with strace seems to confirm this, note the lib/ruby/gems/2.7.0/ruby/2.7.0:

$ strace bundle ls
stat("/nix/store/lm0f2scqkwnpzzkkyr0a8rz5yi0pgssq-redactd-env/lib/ruby/gems/2.7.0/ruby/2.7.0", 0x7ffe054cc3d0) = -1 ENOENT (No such file or directory)

It doesn't add the suffix when path.system is true, but according to the validator spec, this is not supported when using path.

I applied this horrible hack to see if there were further problems, and so far things appear to be working.

diff --git a/pkgs/development/ruby-modules/bundled-common/default.nix b/pkgs/development/ruby-modules/bundled-common/default.nix
index 0812ff590a5..c7b021bee67 100644
--- a/pkgs/development/ruby-modules/bundled-common/default.nix
+++ b/pkgs/development/ruby-modules/bundled-common/default.nix
@@ -107,7 +107,10 @@ let
     postBuild = genStubsScript (defs // args // {
       inherit confFiles bundler groups;
       binPaths = envPaths;
-    }) + lib.optionalString (postBuild != null) postBuild;
+    }) + lib.optionalString (postBuild != null) postBuild + ''
+      mkdir $out/${ruby.gemPath}/${ruby.rubyEngine}
+      ln -s .. $out/${ruby.gemPath}/${ruby.rubyEngine}/${ruby.version.libDir}
+    '';

     meta = { platforms = ruby.meta.platforms; } // meta;

I'd really like to spend some time to look at this properly, but I find my hands full at the moment. Thank you for working on this.

@manveru
Copy link
Contributor Author

manveru commented Mar 2, 2020

I haven't used bundlerEnv other than for testing such changes in quite a while, so excuse me if I'm not really up to speed on what's happening in Bundler-land.

I checked that Ruby could find the gems when they are required, and that gem list lists them. I don't use Bundler for anything other than its dependency resolution via bundler lock, which was also still working as before.

Looking into this a bit more, we might be able to shift to using the BUNDLE_APP_CONFIG variable, which seems to avoid having ruby versions in the path at all. That means it'll work in much the same way as the current setup for ruby.withPackages, just with that variable in addition to the GEM_HOME and probably the wrapping of executables for macos...

I really can't promise I'll have the time or motivation required to migrate this, but at this point I'm not sure we can stick with older Bundler for much longer anyway, and I would much prefer having a simpler setup.

@thefloweringash
Copy link
Member

It seems like removing BUNDLE_PATH and going with GEM_HOME and GEM_PATH is sufficient to be compatible with bundler 1.x and bundler 2.x. I've opened a PR (linked) to do so.

@worldofpeace worldofpeace added the 1.severity: blocker This is preventing another PR or issue from being completed label Apr 3, 2020
@worldofpeace worldofpeace added this to the 20.03 milestone Apr 3, 2020
@manveru manveru closed this Apr 3, 2020
@manveru manveru reopened this Apr 3, 2020
@manveru
Copy link
Contributor Author

manveru commented Apr 3, 2020

@GrahamcOfBorg test redmine.mysql

@ofborg ofborg bot added 6.topic: ruby 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux 10.rebuild-darwin: 101-500 10.rebuild-linux: 101-500 and removed 6.topic: ruby 10.rebuild-darwin: 101-500 10.rebuild-linux: 101-500 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux labels Apr 3, 2020
@thefloweringash
Copy link
Member

@manveru you’re missing a source of gems in the bin stub generator, which is breaking asciidoctor which is required for the test-vm closure. In my PR I set GEM_HOME and GEM_PATH to match the shell script wrappers. These two lines:

ENV["GEM_HOME"] = #{bundle_path.dump}
ENV["GEM_PATH"] = #{bundle_path.dump}

This is enough to start the test vm, but redmine itself fails due to requiring bundler 1.x. This is probably the auto switch feature added in bundler2. Changing the bundler version in the lock file like so:

diff --git a/pkgs/applications/version-management/redmine/Gemfile.lock b/pkgs/applications/version-management/redmine/Gemfile.lock
index 806847a298e..66326b28235 100644
--- a/pkgs/applications/version-management/redmine/Gemfile.lock
+++ b/pkgs/applications/version-management/redmine/Gemfile.lock
@@ -225,4 +225,4 @@ DEPENDENCIES
   yard

 BUNDLED WITH
-   1.16.3
+   2.1.4

And now the redmine tests pass.

It seems like we have two options:

  • update all the Gemfile.lock files in tree
  • provide both bundler gems

The first is less complex but is harsher on out of tree consumers which still specify bundler 1.x in their gemfiles. The second is an easier upgrade path, but getting away from bundler 1’s mysterious breakage is what started this.

@thefloweringash
Copy link
Member

thefloweringash commented Apr 4, 2020

I’ve done some more digging, but haven’t reached the bottom of this yet.

The bundle version switching logic happens in rubygems (!!). When activating the “bundler” gem it will check for a Gemfile.lock (among other things) and use the version specified there. This logic will only apply to stubs that activate an ambient bundler like ${ruby}/bin/bundle and ${redmine}/share/redmine/bin/bundle. For the external bundler gem it has an explicit version as part of the activation in its bin stub so no version switching will take place. This applies to ${bundler}/bin/bundle.

So to be compatible with version switching, we can provide both bundler 1.x and 2.x in the default bundlerEnv / bundlerApp, but this does introduce some potential for confusion since the bin stub scripts use their own method of activating bundler. Making this change locally I verify that redmine will activate bundler 1 (which immediately breaks).

I’ll keep digging.

@manveru
Copy link
Contributor Author

manveru commented Apr 6, 2020

@thefloweringash I updated all the Gemfile.lock files to the new version, seems like that might work for now.

@manveru
Copy link
Contributor Author

manveru commented Apr 6, 2020

@GrahamcOfBorg test redmine.mysql

@manveru
Copy link
Contributor Author

manveru commented Apr 6, 2020

Nevermind, the bin stubs are still broken... setting GEM_HOME before calling the bin stub works, but not setting the variable before requiring bundler, I assume it's read once when rubygems is required on ruby startup and not afterwards... meaning we need to have a wrapper for our wrapper or disable rubygems in the shebang, which won't work cross-platform.

Will work on a fix.

@manveru
Copy link
Contributor Author

manveru commented Apr 6, 2020

So this makes asciidoctor work again, one of the indirect redmine dependencies, I'm very hopeful this works for all other gems as well.

@manveru
Copy link
Contributor Author

manveru commented Apr 6, 2020

@GrahamcOfBorg test redmine.mysql

@aanderse
Copy link
Member

aanderse commented Apr 8, 2020

@manveru I upgraded an existing redmine install to run from this branch and redmine works flawlessly. Thank you for your time and effort on this, it is much appreciated by myself and others 👍 🎉

ping @NixOS/nixos-release-managers

@disassembler disassembler merged commit 72cb7f8 into NixOS:master Apr 8, 2020
@disassembler
Copy link
Member

cherry picked to release branch!

@jdelStrother
Copy link
Contributor

jdelStrother commented Apr 9, 2020

@manveru Is it expected that we get this sudo warning from bundler when using bundlerEnv? Is there anything we can do about it?

image

[nix-shell:~/Developer/web]$ cat $(which rails)
#!/nix/store/q96lzix9isfmh9cbhb7ah65naq2v82z2-ruby-2.6.6/bin/ruby
#
# This file was generated by Nix.
#
# The application 'rails' is installed as part of a gem, and
# this file is here to facilitate running it.
#

ENV["BUNDLE_GEMFILE"] = "/nix/store/mnsbdlw174p05d8civb7ggixndl3wk5r-gemfile-and-lockfile/Gemfile"
ENV.delete 'BUNDLE_PATH'
ENV['BUNDLE_FROZEN'] = '1'

Gem.paths = { 'GEM_HOME' => "/nix/store/2hz3bsbi0ny5d7chsysac1m5avm7x9mf-gems-for-audioboom/lib/ruby/gems/2.6.0" }

$LOAD_PATH.unshift "/nix/store/vwmskqd95mq5hxb6jzgxplmmb79srf0n-bundler-2.1.4/lib/ruby/gems/2.6.0/gems/bundler-2.1.4/lib"

require 'bundler'
Bundler.setup("default", "assets", "development", "test")

load Gem.bin_path("railties", "rails")

[nix-shell:~/Developer/web]$ rails console
Following files may not be writable, so sudo is needed:
  /nix/store/2hz3bsbi0ny5d7chsysac1m5avm7x9mf-gems-for-audioboom/lib/ruby/gems/2.6.0
  /nix/store/2hz3bsbi0ny5d7chsysac1m5avm7x9mf-gems-for-audioboom/lib/ruby/gems/2.6.0/bin
  /nix/store/2hz3bsbi0ny5d7chsysac1m5avm7x9mf-gems-for-audioboom/lib/ruby/gems/2.6.0/bin
  /nix/store/2hz3bsbi0ny5d7chsysac1m5avm7x9mf-gems-for-audioboom/lib/ruby/gems/2.6.0/build_info
  /nix/store/2hz3bsbi0ny5d7chsysac1m5avm7x9mf-gems-for-audioboom/lib/ruby/gems/2.6.0/build_info/libv8-7.3.492.27.1.info
  /nix/store/2hz3bsbi0ny5d7chsysac1m5avm7x9mf-gems-for-audioboom/lib/ruby/gems/2.6.0/build_info/mini_racer-0.2.9.info
  /nix/store/2hz3bsbi0ny5d7chsysac1m5avm7x9mf-gems-for-audioboom/lib/ruby/gems/2.6.0/build_info/nokogiri-1.10.9.info
  /nix/store/2hz3bsbi0ny5d7chsysac1m5avm7x9mf-gems-for-audioboom/lib/ruby/gems/2.6.0/build_info/sassc-2.2.1.info
  /nix/store/2hz3bsbi0ny5d7chsysac1m5avm7x9mf-gems-for-audioboom/lib/ruby/gems/2.6.0/bundler
  /nix/store/2hz3bsbi0ny5d7chsysac1m5avm7x9mf-gems-for-audioboom/lib/ruby/gems/2.6.0/cache
  /nix/store/2hz3bsbi0ny5d7chsysac1m5avm7x9mf-gems-for-audioboom/lib/ruby/gems/2.6.0/doc
  /nix/store/2hz3bsbi0ny5d7chsysac1m5avm7x9mf-gems-for-audioboom/lib/ruby/gems/2.6.0/extensions
  /nix/store/2hz3bsbi0ny5d7chsysac1m5avm7x9mf-gems-for-audioboom/lib/ruby/gems/2.6.0/gems
  /nix/store/2hz3bsbi0ny5d7chsysac1m5avm7x9mf-gems-for-audioboom/lib/ruby/gems/2.6.0/specifications
Running via Spring preloader in process 34287
Loading development environment (Rails 6.0.2.2)
irb(main):001:0>
It seems harmless but annoying, pops up whenever you run a bundled gem executable.

@ghost
Copy link

ghost commented Apr 12, 2020

@manveru Is it expected that we get this sudo warning from bundler when using bundlerEnv? Is there anything we can do about it?
It seems harmless but annoying, pops up whenever you run a bundled gem executable.

I have the same issue here, I did not find any way to silence those warning. I have the feeling that's something directly in bundler that would requires some tweaks in it to silence this message.

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.

6 participants