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.

Sign up for GitHub

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

Jump to bottom

Replace deprecated Bundler.with_clean_env with with_unbundled_env #2120

Closed
wants to merge 1 commit into from
Closed

Replace deprecated Bundler.with_clean_env with with_unbundled_env #2120

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
7 changes: 5 additions & 2 deletions .github/workflows/ruby.yml
View file
Open in desktop
Expand Up @@ -40,8 +40,11 @@ jobs:
run: |
gem update --system 2.7.10 --no-document

- name: Update Bundler
run: gem install bundler
Copy link
Member

Choose a reason for hiding this comment

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

Your PR fails on Ruby 2.2 because you're install Bundler 2 on Ruby 2.2. Instead, you need to change the Github action so that you're running gem install bundler -v 1.17.3 when running Ruby 2.2.

Copy link
Member

Choose a reason for hiding this comment

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

The 2.2 Rubies have Bundler 1.7.3 installed already. See the 2.2 jobs at:
https://github.com/ruby/setup-ruby/runs/491284436


- name: bundle install
run: bundle install --jobs 4 --retry 3 --path=.bundle/puma
run: bundle install --jobs 4 --retry 3
- name: compile
run: bundle exec rake compile

Expand Down Expand Up @@ -87,7 +90,7 @@ jobs:
ruby-version: ${{ matrix.ruby }}

- name: bundle install
run: bundle install --jobs 4 --retry 3 --path=.bundle/puma
run: bundle install --jobs 4 --retry 3

- name: compile
if: matrix.ruby >= '2.4'
Expand Down
1 change: 1 addition & 0 deletions History.md
View file
Open in desktop
Expand Up @@ -17,6 +17,7 @@
* Preserve `BUNDLE_GEMFILE` env var when using `prune_bundler` (#1893)
* Send 408 request timeout even when queue requests is disabled (#2119)
* Rescue IO::WaitReadable instead of EAGAIN for blocking read (#2121)
* Replace deprecated `Bundler.with_clean_env` with `with_unbundled_env` (#2120)

* Refactor
* Remove unused loader argument from Plugin initializer (#2095)
Expand Down
8 changes: 3 additions & 5 deletions lib/puma/launcher.rb
View file
Open in desktop
Expand Up @@ -297,11 +297,9 @@ def prune_bundler
deps, dirs = dependencies_and_files_to_require_after_prune

log '* Pruning Bundler environment'
home = ENV['GEM_HOME']
Copy link
Member

Choose a reason for hiding this comment

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

I thought original_env only looked at BUNDLE_ environment variables.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do we have any test for this? Or can we write it?

Copy link
Member

Choose a reason for hiding this comment

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

#653 looks like no test exists

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll try to implement tests for this, but if somebody can help me — I appreciate it, I'm not too familiar with Puma sources and environments.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also there are no tests for #1893. And I've found prune_bundler in test/ directory only in test/config/prune_bundler_with_deps.rb, and I see no usage of this file in test/.

I've added p ENV['GEM_HOME'] inside Launcher#prune_bundler and there is no output from tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, I've found test/test_preserve_bundler_env.rb, OK.

Copy link
Member

Choose a reason for hiding this comment

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

You won't see that output because the Puma tests create new processes whose output is not piped to STDOUT. Take a look at the helpers/integration test code for more info.

The test for #1893 is here: https://github.com/puma/puma/pull/1893/files#diff-1a1a7e313ee72a0ca0e8eb22a2524879

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Take a look at the helpers/integration test code for more info.

I've found :log option for wait_for_server_to_boot, yes, thank you.

Current results:

  • GEM_HOME was /home/alex/.rbenv/versions/2.7.0/lib/ruby/gems/2.7.0 and becomes nil
  • BUNDLE_GEMFILE was /home/alex/Projects/ruby/puma/test/bundle_preservation_test/Gemfile.bundle_env_preservation_test and becomes Gemfile.bundle_env_preservation_test.

I'll try to add correct test for GEM_HOME. I think the change of BUNDLE_GEMFILE is not critical (tests are passing).

bundle_gemfile = ENV['BUNDLE_GEMFILE']
Bundler.with_clean_env do
ENV['GEM_HOME'] = home
ENV['BUNDLE_GEMFILE'] = bundle_gemfile
gem_home = ENV['GEM_HOME']
Bundler.with_unbundled_env do
ENV['GEM_HOME'] = gem_home
ENV['PUMA_BUNDLER_PRUNED'] = '1'
args = [Gem.ruby, puma_wild_location, '-I', dirs.join(':'), deps.join(',')] + @original_argv
# Ruby 2.0+ defaults to true which breaks socket activation
Expand Down
12 changes: 11 additions & 1 deletion test/bundle_preservation_test/config.ru
View file
Open in desktop
@@ -1 +1,11 @@
run lambda { |env| [200, {'Content-Type'=>'text/plain'}, [ENV['BUNDLE_GEMFILE'].inspect]] }
run(
lambda do |env|
[
200,
{'Content-Type'=>'text/plain'},
[
[ENV['BUNDLE_GEMFILE'], ENV['GEM_HOME']].inspect
]
]
end
)
11 changes: 8 additions & 3 deletions test/test_preserve_bundler_env.rb
View file
Open in desktop
Expand Up @@ -12,9 +12,12 @@ def test_usr2_restart_preserves_bundler_environment
skip_unless_signal_exist? :USR2

@tcp_port = UniquePort.call
gem_home = "/home/bundle_env_preservation_test"
bundle_gemfile = "Gemfile.bundle_env_preservation_test"
env = {
"GEM_HOME" => gem_home,
# Intentionally set this to something we wish to keep intact on restarts
"BUNDLE_GEMFILE" => "Gemfile.bundle_env_preservation_test",
"BUNDLE_GEMFILE" => bundle_gemfile,
# Don't allow our (rake test's) original env to interfere with the child process
"BUNDLER_ORIG_BUNDLE_GEMFILE" => nil
}
Expand All @@ -27,9 +30,11 @@ def test_usr2_restart_preserves_bundler_environment
@pid = @server.pid
connection = connect
initial_reply = read_body(connection)
assert_match("Gemfile.bundle_env_preservation_test", initial_reply)
refute_match(bundle_gemfile, initial_reply)
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems this is a full revert of the work done in #1893, yes? The goal of #1893 was to allow a custom Gemfile name for apps that don't use the default.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems #2133 is conflicting with #1893, yes? I'm not sure what to do.

Copy link
Contributor

Choose a reason for hiding this comment

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

Here is the situation as I understand it:

  1. Prior to Preserve BUNDLE_GEMFILE and add a test for it #1893, it was not possible to run your app using a non-default Gemfile. Setting BUNDLE_GEMFILE=Gemfile.rails6 (for example, as a way to test an app using a different version of Rails), would work at first, but as soon as the app was reloaded/restarted, then the environment variable would be reset to default. Preserve BUNDLE_GEMFILE and add a test for it #1893 fixed that, but also introduced a bug: BUNDLE_GEMFILE is incorrectly set to parent Gemfile in worker processes (when not setting BUNDLE_GEMFILE explicitly) #2133.
  2. BUNDLE_GEMFILE is incorrectly set to parent Gemfile in worker processes (when not setting BUNDLE_GEMFILE explicitly) #2133 details the bug where child processes inherit the full path of the Gemfile of the master process. Instead of the child process seeing the value of BUNDLE_GEMFILE as simply "Gemfile" or "Gemfile.rails6", the child process gets the full path to the gemfile, after its path has been expanded. That's bad.

If there is a way to have both the value preservation of #1893, but without the full path sharing detailed in #2133, then we'd like to have both.

Whew! I hope that makes sense.

assert_match(gem_home, initial_reply)
restart_server connection
new_reply = read_body(connection)
assert_match("Gemfile.bundle_env_preservation_test", new_reply)
refute_match(bundle_gemfile, new_reply)
assert_match(gem_home, initial_reply)
end
end