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

Preserve BUNDLE_APP_CONFIG on worker fork #2688

Merged
merged 1 commit into from Sep 3, 2021

Conversation

jdelStrother
Copy link
Contributor

Without this, if puma is launched with BUNDLE_APP_CONFIG set, that will
be lost inside of unbundled_env, which causes the worker processes to use a
potentially different bundler configuration.

Closes #2687

Your checklist for this pull request

  • I have reviewed the guidelines for contributing to this repository.
  • I have added (or updated) 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.

@MSP-Greg MSP-Greg added the bug label Sep 2, 2021
@cjlarose cjlarose self-assigned this Sep 2, 2021
@@ -39,6 +39,25 @@ def test_usr2_restart_preserves_bundler_environment
assert_match("Gemfile.bundle_env_preservation_test", new_reply)
end

def test_worker_forking_preserves_bundler_config_path
Copy link
Member

Choose a reason for hiding this comment

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

Locally, if I undo the change introduced to prune_bundler,

git restore --source HEAD^ -- lib

This test is still green. In other words, the test does not appear to guard against a regression for the new functionality.

Do you think it is possible to write this test in a way that it fails on master in the way you observed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Bah, I was testing with ruby -Ilib:test test/test_preserve_bundler_env.rb which does fail on master, but looks like the same thing inside of bundle exec doesn't.

I've force-pushed an update that sets BUNDLE_GEMFILE in the test, which should persuade it to fail in either scenario (and then subsequently be fixed by the BUNDLE_APP_CONFIG change).

Copy link
Member

Choose a reason for hiding this comment

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

Awesome, thanks!

Without this, if puma is launched with BUNDLE_APP_CONFIG set, that will
be lost inside of unbundled_env, which causes the worker processes to use a
potentially different bundler configuration.
@cjlarose cjlarose merged commit 3c5d12d into puma:master Sep 3, 2021
JuanitoFatas pushed a commit to JuanitoFatas/puma that referenced this pull request Sep 9, 2022
Without this, if puma is launched with BUNDLE_APP_CONFIG set, that will
be lost inside of unbundled_env, which causes the worker processes to use a
potentially different bundler configuration.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

with_bundled_env causes trouble if BUNDLE_APP_CONFIG was set
3 participants