Replace deprecated `Bundler.with_clean_env` with `with_unbundled_env` #2120

AlexWayfer · 2020-02-20T12:26:59Z

Also add tests for GEM_HOME environment variable preserving.

Rework of #1794

Also drop BUNDLE_GEMFILE preserving, resolve #2133

Also disable local path for bundle install in CI, it makes nio4r unavailable.

Also install (update) bundler in CI, old versions are incompatible.

Your checklist for this pull request

I have reviewed the guidelines for contributing to this repository.
I have added an entry to History.md if this PR fixes a bug or adds a feature. If it doesn't need an entry to HISTORY.md, I have added [changelog skip] the pull request title.
I have added appropriate tests if this PR fixes a bug or adds a feature.
My pull request is 100 lines added/removed or less so that it can be easily reviewed.
If this PR doesn't need tests (docs change), I added [ci skip] to the title of the PR.
If this closes any issues, I have added "Closes #issue" to the PR description or my commit messages.
I have updated the documentation accordingly.
All new and existing tests passed, including Rubocop.

AlexWayfer · 2020-02-20T12:38:02Z

Strange fail: https://travis-ci.org/puma/puma/jobs/652963711?utm_medium=notification&utm_source=github_status

AlexWayfer · 2020-02-20T12:43:58Z

I don't see new checks, and I don't understand maintenance label (it has no description), but I'll mark this PR as "Ready for review".

AlexWayfer · 2020-02-20T13:22:04Z

I don't understand why Ruby 2.2 fails, the job doesn't look like mentioned problem.

nateberkopec · 2020-02-20T15:35:40Z

lib/puma/launcher.rb

@@ -296,11 +296,7 @@ def prune_bundler
      deps, dirs = dependencies_and_files_to_require_after_prune

      log '* Pruning Bundler environment'
-      home = ENV['GEM_HOME']


I thought original_env only looked at BUNDLE_ environment variables.

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

#653 looks like no test exists

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.

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.

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

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

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

nateberkopec · 2020-02-25T13:27:18Z

Questions/concerns:

Does the new Bundler method mess with GEM_HOME? May be able to read Bundler source to determine.
Can we write a test to catch our own behavior re: GEM_HOME?
Can we fix the failure on 2.2 (it is related IMO)?

AlexWayfer · 2020-02-25T14:46:36Z

Does the new Bundler method mess with GEM_HOME? May be able to read Bundler source to determine.

Bundler.with_original_env uses Bundler.original_env, which uses EnvironmentPreserver with BUNDLER_KEYS, which includes BUNDLE_GEMFILE and GEM_HOME.

But tests are somewhy failing without manual reassign at least GEM_HOME.

Oh… it's passing without bundle exec for tests running. So, the original env is when you calling bundle exec rake. What we should to do?

I think, we can re-do something similar to with_original_env, but with env before the call, not the first one. And it's possible, but with_env(our_collected_env) is private method. I'll try to contribute to Bundler itself to add something like with_previous_env. Sorry, but I can't mark this PR as Draft, as I know.

Can we write a test to catch our own behavior re: GEM_HOME?

Yes, done.

Can we fix the failure on 2.2 (it is related IMO)?

Let's look. Maybe it's not related, due to #2018

AlexWayfer · 2020-02-25T14:58:17Z

I've removed "WIP" status because I don't think that we'll get Bundler.with_previous_env shortly, and now code is correct as I understand.

We can patch or work-around private Bundler.with_env method, at least with send :with_env, env. As you wish.

AlexWayfer · 2020-02-25T15:22:25Z

OK, I've understood that I'm not completely understand prune-bundler option. What kind of ENV is expected inside.

AlexWayfer · 2020-02-25T15:30:00Z

master process of puma should be… pristine by which bundler env? Should it be original or clean at all? With unbundled tests are failing (example of job).

Like… when we use bundle exec puma --prune-bundler — the master process is always in current (local) bundle context and it's incorrect, conflicting. And bundle exec rake (for tests) has similar behavior of this conflict with with_original_env.

AlexWayfer · 2020-02-25T15:39:22Z

Right now we have 100% reproducable case of #2018. 😅

nateberkopec · 2020-02-26T17:55:53Z

Is it? I think this is a different exception class: https://github.com/puma/puma/pull/2120/checks?check_run_id=467555536#step:8:78

nateberkopec

Adding a "review" to clear my request.

AlexWayfer · 2020-02-28T09:15:11Z

Adding a "review" to clear my request.

Sorry, I just don't know what should I do now.

I see problems, but I don't know in which direction I should move. My questions from February 25th are still actual.

nateberkopec · 2020-02-28T18:22:39Z

Yikes, I just started running tests with a newer bundler version, and the output from all deprecation notices is so ugly. This just got bumped up a notch on the priority list!

nateberkopec · 2020-03-05T20:54:46Z

I'm honestly not sure what to do here, but I think we should be crosstalking with the people in #2133.

AlexWayfer · 2020-03-05T23:03:51Z

I'm honestly not sure what to do here, but I think we should be crosstalking with the people in #2133.

Thank you, I'm involved into discussion.

AlexWayfer · 2020-03-06T01:25:18Z

I don't know what to do (I hate old versions of software): with (unsupported) Ruby 2.2 this PR is excess:

ERROR:  Error installing bundler:
	The last version of bundler (>= 0) to support your Ruby & RubyGems was 1.17.3. Try installing it with `gem install bundler -v 1.17.3`
	bundler requires Ruby version >= 2.3.0. The current ruby version is 2.2.0.

So… we should use an old version of Bundler for Ruby 2.2, but we should change a code for a new version of Bundler for Ruby 2.7. It seems like two chairs, I suggest to drop Ruby 2.2 support, or at least left this PR open until it'll be happen.

Also add tests for `GEM_HOME` environment variable preserving. Also drop `BUNDLE_GEMFILE` preserving, resolve puma#2133 Also disable local `path` for `bundle install` in CI, it makes `nio4r` unavailable. Also install (update) `bundler` in CI, old versions are incompatible.

seven1m · 2020-03-06T02:03:33Z

test/test_preserve_bundler_env.rb

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


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.

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

Here is the situation as I understand it:

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.

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.

nateberkopec · 2020-03-06T21:35:14Z

.github/workflows/ruby.yml

@@ -40,8 +40,11 @@ jobs:
        run:  |
          gem update --system 2.7.10 --no-document

+      - name: Update Bundler
+        run: gem install bundler


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.

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

nateberkopec · 2020-03-06T21:52:12Z

Oh I see.

Ruby 2.2 does not support Bundler 2. It supports 1.17.3.
In Bundler 1.17.3, clean_env is basically the original environment variables from process start MINUS any env variables starting with BUNDLE_
So, Ruby 2.2 won't work with unbundled_env, because it won't exist. That's fine, we'll just need a conditional on the Bundler 2.1+. Also, lots of people still use Bundler 1 and Bundler 2.0.
For Bundler 2.1+ users, we can use with_unbundled_env, and I believe the implementation is exactly the same as far as I can tell

IMO we should scale back this PR to the above and leave out any other changes.

nateberkopec · 2020-03-07T00:16:42Z

Closing in favor of #2151, lets keep discussion about BUNDLE_GEMFILE on #2133

AlexWayfer · 2020-03-08T22:18:37Z

There are tests… but, as you wish. Thank you for clarification and suggestions.

AlexWayfer force-pushed the replace_deprecated_with_clean_env_for_bundler branch from bb06b8b to a189b7f Compare February 20, 2020 12:37

nateberkopec added the maintenance label Feb 20, 2020

AlexWayfer force-pushed the replace_deprecated_with_clean_env_for_bundler branch from a189b7f to d30d837 Compare February 20, 2020 12:43

AlexWayfer marked this pull request as ready for review February 20, 2020 12:44

AlexWayfer force-pushed the replace_deprecated_with_clean_env_for_bundler branch from d30d837 to 563bdd5 Compare February 20, 2020 12:44

nateberkopec reviewed Feb 20, 2020

View reviewed changes

nateberkopec added the waiting-for-changes Waiting on changes from the requestor label Feb 21, 2020

AlexWayfer force-pushed the replace_deprecated_with_clean_env_for_bundler branch from 563bdd5 to 9fe396c Compare February 25, 2020 14:46

AlexWayfer changed the title ~~Replace deprecated Bundler.with_clean_env with with_original_env~~ Replace deprecated Bundler.with_clean_env with with_unbundled_env Feb 25, 2020

AlexWayfer changed the title ~~Replace deprecated Bundler.with_clean_env with with_unbundled_env~~ WIP: Replace deprecated Bundler.with_clean_env with with_unbundled_env Feb 25, 2020

AlexWayfer force-pushed the replace_deprecated_with_clean_env_for_bundler branch from 9fe396c to abf3e42 Compare February 25, 2020 14:56

AlexWayfer changed the title ~~WIP: Replace deprecated Bundler.with_clean_env with with_unbundled_env~~ Replace deprecated Bundler.with_clean_env with with_unbundled_env Feb 25, 2020

AlexWayfer force-pushed the replace_deprecated_with_clean_env_for_bundler branch from abf3e42 to 4f1ade1 Compare February 25, 2020 15:30

AlexWayfer requested a review from nateberkopec February 25, 2020 15:39

nateberkopec reviewed Feb 27, 2020

View reviewed changes

nateberkopec added waiting-for-review Waiting on review from anyone and removed waiting-for-changes Waiting on changes from the requestor labels Feb 28, 2020

nateberkopec mentioned this pull request Mar 5, 2020

BUNDLE_GEMFILE is incorrectly set to parent Gemfile in worker processes (when not setting BUNDLE_GEMFILE explicitly) #2133

Closed

AlexWayfer force-pushed the replace_deprecated_with_clean_env_for_bundler branch 5 times, most recently from 5288943 to 54b516e Compare March 6, 2020 01:21

AlexWayfer force-pushed the replace_deprecated_with_clean_env_for_bundler branch from 54b516e to 0669fb5 Compare March 6, 2020 01:29

seven1m reviewed Mar 6, 2020

View reviewed changes

nateberkopec reviewed Mar 6, 2020

View reviewed changes

nateberkopec closed this Mar 7, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace deprecated `Bundler.with_clean_env` with `with_unbundled_env` #2120

Replace deprecated `Bundler.with_clean_env` with `with_unbundled_env` #2120

AlexWayfer commented Feb 20, 2020 •

edited

AlexWayfer commented Feb 20, 2020

AlexWayfer commented Feb 20, 2020

AlexWayfer commented Feb 20, 2020

nateberkopec Feb 20, 2020

AlexWayfer Feb 20, 2020

nateberkopec Feb 20, 2020

AlexWayfer Feb 20, 2020

AlexWayfer Feb 25, 2020

AlexWayfer Feb 25, 2020

nateberkopec Feb 25, 2020

AlexWayfer Feb 25, 2020 •

edited

nateberkopec commented Feb 25, 2020

AlexWayfer commented Feb 25, 2020

AlexWayfer commented Feb 25, 2020

AlexWayfer commented Feb 25, 2020

AlexWayfer commented Feb 25, 2020 •

edited

AlexWayfer commented Feb 25, 2020

nateberkopec commented Feb 26, 2020

nateberkopec left a comment

AlexWayfer commented Feb 28, 2020

nateberkopec commented Feb 28, 2020

nateberkopec commented Mar 5, 2020

AlexWayfer commented Mar 5, 2020

AlexWayfer commented Mar 6, 2020

seven1m Mar 6, 2020

AlexWayfer Mar 6, 2020

seven1m Mar 6, 2020

nateberkopec Mar 6, 2020

MSP-Greg Mar 6, 2020

nateberkopec commented Mar 6, 2020

nateberkopec commented Mar 7, 2020

AlexWayfer commented Mar 8, 2020

Replace deprecated Bundler.with_clean_env with with_unbundled_env #2120

Replace deprecated Bundler.with_clean_env with with_unbundled_env #2120

Conversation

AlexWayfer commented Feb 20, 2020 • edited

Your checklist for this pull request

AlexWayfer commented Feb 20, 2020

AlexWayfer commented Feb 20, 2020

AlexWayfer commented Feb 20, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlexWayfer Feb 25, 2020 • edited

Choose a reason for hiding this comment

nateberkopec commented Feb 25, 2020

AlexWayfer commented Feb 25, 2020

AlexWayfer commented Feb 25, 2020

AlexWayfer commented Feb 25, 2020

AlexWayfer commented Feb 25, 2020 • edited

AlexWayfer commented Feb 25, 2020

nateberkopec commented Feb 26, 2020

nateberkopec left a comment

Choose a reason for hiding this comment

AlexWayfer commented Feb 28, 2020

nateberkopec commented Feb 28, 2020

nateberkopec commented Mar 5, 2020

AlexWayfer commented Mar 5, 2020

AlexWayfer commented Mar 6, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nateberkopec commented Mar 6, 2020

nateberkopec commented Mar 7, 2020

AlexWayfer commented Mar 8, 2020

Replace deprecated `Bundler.with_clean_env` with `with_unbundled_env` #2120

Replace deprecated `Bundler.with_clean_env` with `with_unbundled_env` #2120

AlexWayfer commented Feb 20, 2020 •

edited

AlexWayfer Feb 25, 2020 •

edited

AlexWayfer commented Feb 25, 2020 •

edited