Look for absolute path when removing bundler/setup from RUBYOPT in Bundler.unbundled_env method #3877

rreinhardt9 · 2020-08-04T22:59:08Z

Why?

Resolves #3705

Bundler no longer supplies the require in RUBYOPT as a relative path but as an absolute path (see afc00b1 )

What?

This updates unbundled_env to match that behavior and looks for a require with an absolute path to bundler/setup when removing traces of bundler from the environment variable RUBYOPT. It will also continue to look for relative paths to remove as well in order to not create issues with existing scripts people might have that sets it as a relative path.

Testing Notes

In the examples below, to follow along you'll have to change the absolute paths in the commands based on where your ruby gems and bundler repo are located on your computer. I found $ gem list -d bundler helpful to see the path bundler is installed at.

I would expect this issue to appear only on 2.2.0.rc.1 and later because that is the release the commit was in changing RUBYOPT to have an absolute and not relative path and it was:

$ RUBYOPT=-r/Users/ross/.rbenv/versions/2.7.0/lib/ruby/gems/2.7.0/gems/bundler-2.2.0.rc.1/lib/bundler/setup bundler/bin/bundle exec ruby -rbundler -e'puts Bundler::VERSION'
=> 2.2.0.rc.1

$ RUBYOPT=-r/Users/ross/.rbenv/versions/2.7.0/lib/ruby/gems/2.7.0/gems/bundler-2.2.0.rc.1/lib/bundler/setup bundle exec ruby -rbundler -e'puts Bundler.unbundled_env["RUBYOPT"]'
=> -r/Users/ross/.rbenv/versions/2.7.0/lib/ruby/gems/2.7.0/gems/bundler-2.2.0.rc.1/lib/bundler/setup

I then confirmed that the -r statement in RUBYOPT was removed when running the bundler code from this feature branch:

$ RUBYOPT=-r/Users/ross/dev/rubygems/bundler/lib/bundler/setup bundle exec ruby -rbundler -e'puts Bundler::VERSION'
=> 2.2.0.rc.2

$ RUBYOPT=-r/Users/ross/dev/rubygems/bundler/lib/bundler/setup bundle exec ruby -rbundler -e'puts Bundler.unbundled_env["RUBYOPT"]'
=> # Nothing returned

I went back to version 2.1.2 to see what the behavior was before:

$ bundle exec ruby -rbundler -e'puts Bundler::VERSION'
=> 2.1.2

$ bundle exec ruby -rbundler -e'puts ENV["RUBYOPT"]'
=> -r/Users/ross/.rbenv/versions/2.7.0/lib/ruby/gems/2.7.0/gems/bundler-2.1.4/lib/bundler/setup

$ bundle exec ruby -rbundler -e'puts Bundler.unbundled_env["RUBYOPT"]'
=> # Nothing

$ RUBYOPT=-rbundler/setup bundle exec ruby -rbundler -e'puts Bundler.unbundled_env["RUBYOPT"]'
=> # Still nothing

# Doesn't remove an absolute path
$ RUBYOPT=-r/Users/ross/.rbenv/versions/2.7.0/lib/ruby/gems/2.7.0/gems/bundler-2.1.4/lib/bundler/setup bundle exec ruby -rbundler -e'puts Bundler.unbundled_env["RUBYOPT"]'
=> -r/Users/ross/.rbenv/versions/2.7.0/lib/ruby/gems/2.7.0/gems/bundler-2.1.4/lib/bundler/setup

This gives me confidence that:

The issue was only present in and after 2.2.0.rc.1
The change in this behavior addresses the issues seen in 2.2.0.rc.1

Caveats

One caveat to mention is that before using unbundled_env would always remove bundler/setup from RUBYOPT regardless of the version of bundler the entry in RUBYOPT was targeted at. After this change, unbundled_env only removes the absolute path for bundler/setup that points to the currently activated version of bundler. We discussed making a broader regex for trying to clean up any -r for bundler/setup but were concerned that might be too broad and might lead to trouble.

bundler/lib/bundler.rb

bundler/spec/runtime/with_unbundled_env_spec.rb

bundler/lib/bundler.rb

deivid-rodriguez · 2020-08-28T10:42:28Z

Hi @rreinhardt9. After a second look, I don't think this is correct.

There's two helpers here, meant to be used for different things:

Bundler.with_unbundled_env is meant to cleanup every bundler related variable, not matter what the original environment is.
Bundler.with_original_env is meant to restore the environment as it was before bundler was loaded.

So, for example,

$ RUBYOPT=-rbundler/setup ruby -rbundler -e'puts Bundler.unbundled_env["RUBYOPT"]'

should print nothing, as opposed to -rbundler/setup. I believe your changes essentially remove the distinction between these two helpers.

rreinhardt9 · 2020-08-28T15:56:06Z

@deivid-rodriguez Ah, gotcha thanks!!! That helps give me clarity... so the intention of Bundler.with_unbundled_env is to go even further than Bundler.with_original_env and try to clear out any bundler related environment variables that were set, even if they were present in the original environment.

I'll look at it again from that perspective, I'm thinking in that case we for RUBYOPT we change:

      if env.key?("RUBYOPT")
        env["RUBYOPT"] = env["RUBYOPT"].sub "-rbundler/setup", ""
      end

so that the regex matches any path ending in bundler/setup (something like `env["RUBYOPT"].sub /-r.*bundler/setup/, "")

I'll:

Put back check for RUBYOPT and update the regex
Undo the changes to the RUBYLIB and MANPATH code
move the tests back to where they were/ update RUBYOPT test for the new regex check

deivid-rodriguez · 2020-08-28T16:57:21Z

Yes, exactly.

Only thing I'm not sure about is whether to use a regexp to detect bundler/setup like you suggested, or add a separate substitution to look for an exact match of the expanded bundler/setup path that we previously added.

rreinhardt9 · 2020-08-28T22:00:51Z

I'm wondering about that too...

On one hand, if the method is intended to remove any traces of bundler that were in the original environment the regex would catch bundler/setup entries that might not be the same as that from the currently running version of bundler. But I'd be a little concerned the regex could carry some risk because its effects would be more broad.

Additionally, while dealing with RUBYLIB in the same method it looks for an exact expanded match to remove; so doing the same for RUBYOPT would be consistent with that behavior.

Based on that removing the exact expanded path for RUBYOPT seems like the way to go. 🤔

deivid-rodriguez · 2020-08-29T09:07:22Z

Agreed!

deivid-rodriguez · 2020-08-29T09:23:17Z

If you remember, when you update the PR, could you also update the title to reflect what it's finally doing?

bundler/lib/bundler.rb

bundler/spec/runtime/with_unbundled_env_spec.rb

rreinhardt9 · 2020-08-30T01:46:10Z

@deivid-rodriguez Thanks so much for your patience helping me through this! I've made updates we talked about and updated the PR description/title to reflect the new approach and it's ready for another look 🎉

deivid-rodriguez · 2020-08-30T11:45:36Z

Looks good, but I feel we should still keep the old -rbundler/setup check as well. I believe people putting -rbundler/setup in RUBYOPT on their scripts will still expect Bundler.with_unbundled_env to remove that.

rreinhardt9 · 2020-08-30T13:51:25Z

That is a great point 😬 🤦 thanks for catching that!!! I just added a commit handling relative in addition to absolute paths. Once you get a chance to check it out let me know if you want that squashed before the merge and I can do that 🎉

deivid-rodriguez

Looks good!

If you wanted to curate the commit history, I'd split the three different improvements you ended up making into separate commits (and move your comments on the code to the commit message of each individual commit). Namely:

Improve space handling when removing entries from RUBYOPT, as you explained here.
Improve coverage of existing specs to better simulate the original presence of -rbundler/setup in RUBYOPT, as you explained here.
The bug fix itself with the new spec.

But I'm aware of my overpickyness about git history, so this is also fine as is 😅.

rreinhardt9 · 2020-08-30T15:35:04Z

I love it! I'm a fan of an informative git history as well and don't mind splitting those 🎉 I'll do that!

Using `sub` to replace the entry with an empty string "" will only work cleanly if the entry we are removing is the ONLY entry in RUBYOPT. If it's not the only entry, it will leave behind stray whitespace. For example, if RUBYOPT contained-r/path/to/bundler/setup -r/something/else removing -r/path/to/bundler/setup using `sub` would leave a leading space in RUBYOPT. If we tried to include a trailing space when using `sub`, it would solve that issue for when there are more than one entry but it would not work when the entry we are removing is the ONLY entry or when it's the last entry. The new method of splitting on " " helps us avoid all of that because it will "just work" for any of the above cases and it won't leave the leading space behind in RUBYOPT. It also is consistent with the method used to clean RUBYLIB in `unbundled_env` directly beneath this code. The default for String#split is " " so it not technically necessary to specify it, but it's explicitly stated here to give clarity to future readers of this line.

Because we are trying to simulate that the RUBYOPT contained this bundler/setup value IN the original env, we actually have to set it in BUNDLER_ORIG_RUBYOPT for the test. We are simulating that this bundler/setup entry in RUBYOPT existed BEFORE bundler was activated (even though it didn't). If we set it in RUBYOPT it will already be removed even without the code in unbundled_env (the spec still passed when the code in unbundled_env for handling RUBYOPT was commented out). That is because unbundled_env calls original_env as it's starting point, which will remove whatever we set here in RUBYOPT because it was not in the original env when the test suite first activated bundler.

Bundler no longer supplies the require in RUBYOPT as a relative path but as an absolute path (see rubygems@afc00b1 ) This updates `unbundled_env` to match that behavior and looks for a require with an absolute path to `bundler/setup` when removing traces of bundler from the environment variable RUBYOPT. It will also continue to look for relative paths to remove as well in order to not create issues with existing scripts people might have that set it as a relative path.

deivid-rodriguez

Looks great!

deivid-rodriguez · 2020-09-01T13:24:21Z

Thanks!!!

Look for absolute path when removing bundler/setup from RUBYOPT in Bundler.unbundled_env method (cherry picked from commit 43ecbe8)

bundlerbot added the Bundler label Aug 4, 2020

rreinhardt9 force-pushed the unbundled-env-rubyopts branch from b8ec159 to 11877f4 Compare August 28, 2020 02:33

rreinhardt9 changed the title ~~Strip out bundle/setup from RUBYOPT for unbundled_env~~ Remove specific manipulation of RUBYLIB, RUBYOPT, and MANPATH Aug 28, 2020

rreinhardt9 commented Aug 28, 2020

View reviewed changes

rreinhardt9 marked this pull request as ready for review August 28, 2020 12:24

deivid-rodriguez added the bundler: bug fix label Aug 29, 2020

rreinhardt9 changed the title ~~Remove specific manipulation of RUBYLIB, RUBYOPT, and MANPATH~~ Look for absolute path when removing bundler/setup from RUBYOPT in Bundler.unbundled_env method Aug 29, 2020

rreinhardt9 force-pushed the unbundled-env-rubyopts branch from 11877f4 to 2e674f1 Compare August 29, 2020 21:59

rreinhardt9 commented Aug 30, 2020

View reviewed changes

bundler/lib/bundler.rb Show resolved Hide resolved

bundler/spec/runtime/with_unbundled_env_spec.rb Show resolved Hide resolved

deivid-rodriguez approved these changes Aug 30, 2020

View reviewed changes

rreinhardt9 added 3 commits August 30, 2020 11:43

rreinhardt9 force-pushed the unbundled-env-rubyopts branch from bb90389 to 2e355b8 Compare August 30, 2020 16:19

deivid-rodriguez approved these changes Aug 30, 2020

View reviewed changes

deivid-rodriguez merged commit 43ecbe8 into rubygems:master Sep 1, 2020

rreinhardt9 deleted the unbundled-env-rubyopts branch September 1, 2020 22:41

deivid-rodriguez added a commit that referenced this pull request Oct 5, 2020

Merge pull request #3877 from rreinhardt9/unbundled-env-rubyopts

2fa6ab6

Look for absolute path when removing bundler/setup from RUBYOPT in Bundler.unbundled_env method (cherry picked from commit 43ecbe8)

deivid-rodriguez added a commit that referenced this pull request Oct 5, 2020

Merge pull request #3877 from rreinhardt9/unbundled-env-rubyopts

d4ccc69

Look for absolute path when removing bundler/setup from RUBYOPT in Bundler.unbundled_env method (cherry picked from commit 43ecbe8)

deivid-rodriguez added a commit that referenced this pull request Oct 5, 2020

Merge pull request #3877 from rreinhardt9/unbundled-env-rubyopts

d4bc666

Look for absolute path when removing bundler/setup from RUBYOPT in Bundler.unbundled_env method (cherry picked from commit 43ecbe8)

deivid-rodriguez added a commit that referenced this pull request Oct 6, 2020

Merge pull request #3877 from rreinhardt9/unbundled-env-rubyopts

ea8a3d2

Look for absolute path when removing bundler/setup from RUBYOPT in Bundler.unbundled_env method (cherry picked from commit 43ecbe8)

deivid-rodriguez added a commit that referenced this pull request Oct 6, 2020

Merge pull request #3877 from rreinhardt9/unbundled-env-rubyopts

6c2a475

Look for absolute path when removing bundler/setup from RUBYOPT in Bundler.unbundled_env method (cherry picked from commit 43ecbe8)

deivid-rodriguez added a commit that referenced this pull request Oct 6, 2020

Merge pull request #3877 from rreinhardt9/unbundled-env-rubyopts

ff40d41

Look for absolute path when removing bundler/setup from RUBYOPT in Bundler.unbundled_env method (cherry picked from commit 43ecbe8)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Look for absolute path when removing bundler/setup from RUBYOPT in Bundler.unbundled_env method #3877

Look for absolute path when removing bundler/setup from RUBYOPT in Bundler.unbundled_env method #3877

rreinhardt9 commented Aug 4, 2020 •

edited

deivid-rodriguez commented Aug 28, 2020

rreinhardt9 commented Aug 28, 2020 •

edited

deivid-rodriguez commented Aug 28, 2020

rreinhardt9 commented Aug 28, 2020 •

edited

deivid-rodriguez commented Aug 29, 2020

deivid-rodriguez commented Aug 29, 2020

rreinhardt9 commented Aug 30, 2020

deivid-rodriguez commented Aug 30, 2020

rreinhardt9 commented Aug 30, 2020

deivid-rodriguez left a comment

rreinhardt9 commented Aug 30, 2020

deivid-rodriguez left a comment

deivid-rodriguez commented Sep 1, 2020

Look for absolute path when removing bundler/setup from RUBYOPT in Bundler.unbundled_env method #3877

Look for absolute path when removing bundler/setup from RUBYOPT in Bundler.unbundled_env method #3877

Conversation

rreinhardt9 commented Aug 4, 2020 • edited

Why?

What?

Testing Notes

Caveats

deivid-rodriguez commented Aug 28, 2020

rreinhardt9 commented Aug 28, 2020 • edited

deivid-rodriguez commented Aug 28, 2020

rreinhardt9 commented Aug 28, 2020 • edited

deivid-rodriguez commented Aug 29, 2020

deivid-rodriguez commented Aug 29, 2020

rreinhardt9 commented Aug 30, 2020

deivid-rodriguez commented Aug 30, 2020

rreinhardt9 commented Aug 30, 2020

deivid-rodriguez left a comment

Choose a reason for hiding this comment

rreinhardt9 commented Aug 30, 2020

deivid-rodriguez left a comment

Choose a reason for hiding this comment

deivid-rodriguez commented Sep 1, 2020

rreinhardt9 commented Aug 4, 2020 •

edited

rreinhardt9 commented Aug 28, 2020 •

edited

rreinhardt9 commented Aug 28, 2020 •

edited