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

load realpath for require paths symlinked #1295

Conversation

Martin288
Copy link

fix #1272

@Martin288 Martin288 changed the title load realpath for gems symlinked load realpath for require paths symlinked May 15, 2017
@grosser
Copy link
Contributor

grosser commented Jun 3, 2017

FYI trying to fix this whole bundler mess by unsetting bundler ENV vars on restart ... would that solve your issue too ?

# config/puma.rb
Puma::Runner.prepend(Module.new do
  def before_restart
    ENV.replace(Bundler.clean_env)
    super
  end
end)

@Martin288
Copy link
Author

@grosser thanks. but I don't know if there is any side effect that unsetting bundler ENV vars, have you tried it?

@grosser
Copy link
Contributor

grosser commented Jun 5, 2017

yes, using that in prod since ~1 year
only issue is that restarts might not know how to restart so you have to use --restart-cmd 'bundle exec puma'

@Martin288
Copy link
Author

Martin288 commented Jun 5, 2017

@grosser great! Will use it soon. But I still hope it can be solved from the puma repository, however, your solution is a good temporarily solution.

@grosser
Copy link
Contributor

grosser commented Jun 5, 2017

once rubygems/bundler#5700 is resolved puma will work like this ... but there will be solve fallout with puma forgetting how to restart ... so ideally we'd fix that up properly until then :)

@stereobooster
Copy link
Contributor

stereobooster commented Jul 1, 2017

@grosser by idea this is same as your code, but less hacky

# config/puma.rb
on_restart do
  ENV.replace(Bundler.clean_env)
end

@x-yuri
Copy link

x-yuri commented Jul 5, 2018

@grosser I'd rather avoid cleaning env. By the way, it's deprecated for one reason or another. Additionally, it makes restart_command mandatory.

Also, I can confirm that PR by @Martin288 fixes it for cluster mode + prune_bundler. Since in this case puma restarts itself before starting workers. But unfortunately not for other cases. The solution might be to set instance variable on Puma::Launcher.initialize with Pathname($0).realpath.to_s. And use that instead of $0 down the road.

UPD And at first glance the following changes fixes it for single mode, cluster mode, cluster mode + prune_bundler when puma is invoked with bundle exec puma:

--- restart-puma/bundle/ruby/2.5.0/gems/puma-3.11.4/lib/puma/launcher.rb.orig	2018-07-06 09:52:32.450769132 +0300
+++ restart-puma/bundle/ruby/2.5.0/gems/puma-3.11.4/lib/puma/launcher.rb	2018-07-06 09:55:35.604106620 +0300
@@ -8,6 +8,8 @@
 
 require 'puma/binder'
 
+require 'pathname'
+
 module Puma
   # Puma::Launcher is the single entry point for starting a Puma server based on user
   # configuration. It is responsible for taking user supplied arguments and resolving them
@@ -47,6 +49,7 @@
       @argv          = launcher_args[:argv] || []
       @original_argv = @argv.dup
       @config        = conf
+      @arg0 = Pathname($0).realpath.to_s
 
       @binder        = Binder.new(@events)
       @binder.import_from_env
@@ -354,10 +357,10 @@
       # it the same, otherwise add -S on there because it was
       # picked up in PATH.
       #
-      if File.exist?($0)
-        arg0 = [Gem.ruby, $0]
+      if File.exist?(@arg0)
+        arg0 = [Gem.ruby, @arg0]
       else
-        arg0 = [Gem.ruby, "-S", $0]
+        arg0 = [Gem.ruby, "-S", @arg0]
       end
 
       # Detect and reinject -Ilib from the command line, used for testing without bundler

Additionally, I've discovered that prune_bundler makes puma from ~/.gem/ruby/2.5.1 start, not from vendor/bundle/ruby/2.5.0. Is that expected?

@grosser
Copy link
Contributor

grosser commented Jul 8, 2018

yeah either way is fine, this whole realpath mess goes away when deploying using docker anyway, so meh :)

@x-yuri
Copy link

x-yuri commented Mar 8, 2019

With the following puma config (prune_bundler and workers are from the issue):

require 'puma/runner'
Puma::Runner.prepend(Module.new do
    def before_restart
        ENV.replace(Bundler.clean_env)
    super
  end
end)
workers 2
prune_bundler

It (puma-3.12.0) fails to restart but for another reason:

/home/user/app/current/config/puma/production.rb:19:in `before_restart': uninitialized constant #<Class:#<Puma::DSL:0x007fe32aeb7ef8>>::Bundler (NameError)
    from /home/user/app/releases/11/vendor/bundle/ruby/2.3.0/gems/puma-3.12.0/lib/puma/launcher.rb:194:in `run'
    from /home/user/app/releases/11/vendor/bundle/ruby/2.3.0/gems/puma-3.12.0/lib/puma/cli.rb:78:in `run'
    from /home/user/app/releases/11/vendor/bundle/ruby/2.3.0/gems/puma-3.12.0/bin/puma-wild:31:in `<main>

So, I remove prune_bunder, and this error goes away, but we're back at square one. The issue is not resolved. I try it without workers 2, same result. The result is also identical with the following block:

on_restart do
  ENV.replace(Bundler.clean_env)
end

So, it must be safe to assume, that rubygems/bundler#5700 has nothing to do with it. And if you look at the report closely enough, you'll see that it has nothing to do with environment variables.

@nateberkopec
Copy link
Member

Closing this, as I think we're going to go with a different approach from #1105 to fix this code.

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.

Puma::Launcher#prune_bundler should load target path instead of symlink for puma_lib_dir
5 participants