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

Allow user to override ~/.bundle with environment variables #6024

Merged
merged 6 commits into from Sep 18, 2017

Conversation

Projects
None yet
6 participants
@gwerbin
Copy link

commented Sep 11, 2017

What was the end-user problem that led to this PR?

As in #4333, users wanted a way to make Bundler respect the XDG specification.

What was your diagnosis of the problem?

The directory ~/.bundle was hard-coded and could not be configured.

What is your fix for the problem, implemented in this PR?

Allow users to configure ~/.bundle and its relevant sub-directories by setting environment variables. The environment variables and their fallbacks are as follows:

variable fallback if unset
BUNDLE_USER_HOME $HOME/.bundle
BUNDLE_USER_CACHE $BUNDLE_USER_HOME/cache
BUNDLE_USER_CONFIG $BUNDLE_USER_HOME/config
BUNDLE_USER_PLUGIN $BUNDLE_USER_HOME/plugin

Why did you choose this fix out of the possible options?

Unlike #5787, This solution is not specific to the XDG specification. Users have all kinds of setups, and this is a very general system for allowing them to configure their development machines however they need. It tries to keep all files created by Bundler in the same place (as per #5787 (comment)), but allows the user to override that convention if they really want to and they know what they are doing.

If they want to use XDG for everything, they can do it by explicitly setting the BUNDLE_USER_* variables to the equivalent XDG_DATA_*. If they just want to get .bundle out of their home directory, they can do it by setting BUNDLE_USER_HOME and don't have to mess with the more specific env variables if they don't want to.

To me, this solution strikes the right balance between "fine-grained control for power users" and "simple, sane defaults for everyone else".

Please let me know if my tests can be improved. My only Ruby experience so far has been writing Homebrew formulas and configuring Jekyll, so I'm sure I have a lot to learn.

gwerbin added some commits Sep 11, 2017

Allow the user to set alternative to ~/.bundle
Recognize new environment variables, with the following fallbacks:

    $BUNDLE_USER_HOME   -> $HOME/.bundle
    $BUNDLE_USER_CACHE  -> $BUNDLE_USER_HOME/cache
    $BUNDLE_USER_CONFIG -> $BUNDLE_USER_HOME/config
    $BUNDLE_USER_PLUGIN -> $BUNDLE_USER_HOME/plugin

TODOs:
- Error handling in Bundler.user_bundle_path when an invalid option is
passed
- Add tests (see 47fbe99)
- Draft PR with reference to GitHub issue numbers (not linked here as
per contributing guidelines)
    + Issue: 4333
    + Pull request: 5787
Use tests from the old commit
Mostly unmodified tests from PR 5787 (not linked here to respect
contribution guidelines).
@bundlerbot

This comment has been minimized.

Copy link
Contributor

commented Sep 11, 2017

Thanks for opening a pull request and helping make Bundler better! Someone from the Bundler team will take a look at your pull request shortly and leave any feedback. Please make sure that your pull request has tests for any changes or added functionality.

We use Travis CI to test and make sure your change works functionally and uses acceptable conventions, you can review the current progress of Travis CI in the PR status window below.

If you have any questions or concerns that you wish to ask, feel free to leave a comment in this PR or join our #bundler channel on Slack.

For more information about contributing to the Bundler project feel free to review our CONTRIBUTING guide

@segiddins
Copy link
Member

left a comment

This seems reasonable to me. Thoughts @colby-swandale @indirect ?

@@ -193,8 +193,26 @@ def tmp_home_path(login, warning)
raise e.exception("#{warning}\nBundler also failed to create a temporary home directory at `#{path}':\n#{e}")
end

def user_bundle_path
Pathname.new(user_home).join(".bundle")
def user_bundle_path(dir="home")

This comment has been minimized.

Copy link
@segiddins

segiddins Sep 11, 2017

Member

space around =

gwerbin added some commits Sep 11, 2017

@colby-swandale

This comment has been minimized.

Copy link
Member

commented Sep 18, 2017

I have a few issues with this PR, mainly it's making things relating to the bundle path pretty confusing but this can be addressed in a later PR.

@bundlerbot r+

@bundlerbot

This comment has been minimized.

Copy link
Contributor

commented Sep 18, 2017

📌 Commit b3108fa has been approved by colby-swandale

@bundlerbot

This comment has been minimized.

Copy link
Contributor

commented Sep 18, 2017

⌛️ Testing commit b3108fa with merge e6cc7a2...

bundlerbot added a commit that referenced this pull request Sep 18, 2017

Auto merge of #6024 - gwerbin:custom-user-bundle-dir, r=colby-swandale
Allow user to override ~/.bundle with environment variables

### What was the end-user problem that led to this PR?

As in #4333, users wanted a way to make Bundler respect the XDG specification.

### What was your diagnosis of the problem?

The directory `~/.bundle` was hard-coded and could not be configured.

### What is your fix for the problem, implemented in this PR?

Allow users to configure `~/.bundle` and its relevant sub-directories by setting environment variables. The environment variables and their fallbacks are as follows:

| variable | fallback if unset |
|---|---|
| `BUNDLE_USER_HOME` | `$HOME/.bundle` |
| `BUNDLE_USER_CACHE`  | `$BUNDLE_USER_HOME/cache` |
| `BUNDLE_USER_CONFIG` | `$BUNDLE_USER_HOME/config` |
| `BUNDLE_USER_PLUGIN` | `$BUNDLE_USER_HOME/plugin` |

### Why did you choose this fix out of the possible options?

Unlike #5787, This solution is not specific to the XDG specification. Users have all kinds of setups, and this is a very general system for allowing them to configure their development machines however they need. It tries to keep all files created by Bundler in the same place (as per #5787 (comment)), but allows the user to override that convention _if they really want to and they know what they are doing_.

If they want to use XDG for everything, they can do it by explicitly setting the `BUNDLE_USER_*` variables to the equivalent `XDG_DATA_*`. If they just want to get `.bundle` out of their home directory, they can do it by setting `BUNDLE_USER_HOME` and don't have to mess with the more specific env variables if they don't want to.

To me, this solution strikes the right balance between "fine-grained control for power users" and "simple, sane defaults for everyone else".

Please let me know if my tests can be improved. My only Ruby experience so far has been writing Homebrew formulas and configuring Jekyll, so I'm sure I have a lot to learn.
@bundlerbot

This comment has been minimized.

Copy link
Contributor

commented Sep 18, 2017

☀️ Test successful - status-travis
Approved by: colby-swandale
Pushing e6cc7a2 to master...

@bundlerbot bundlerbot merged commit b3108fa into bundler:master Sep 18, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@gwerbin

This comment has been minimized.

Copy link
Author

commented Sep 18, 2017

@colby-swandale what issues do you have in mind? Obviously I'm happy to have my PR merged ("it works for me"), but I don't want to be responsible for any design debt in the future.

@jasonkarns

This comment has been minimized.

Copy link
Contributor

commented May 8, 2018

@gwerbin The discussion above (mainly the env var fallbacks table) seemed to imply that the vars all indicate directories.

Allow users to configure ~/.bundle and its relevant sub-directories by setting environment variables.

The code seems to indicate that BUNDLE_USER_CONFIG should point to a file, not a directory.

def global_config_file
if ENV["BUNDLE_CONFIG"] && !ENV["BUNDLE_CONFIG"].empty?
Pathname.new(ENV["BUNDLE_CONFIG"])
else
begin
Bundler.user_bundle_path("config")
rescue PermissionError, GenericSystemCallError
nil
end
end
end

Is this the case? Can this be documented somewhere?

@gwerbin

This comment has been minimized.

Copy link
Author

commented May 8, 2018

@jasonkarns the relevant diff from my patch corresponding to that snippet is here: e6cc7a2#diff-75219bd6bc0defc0e7f4ba322d92f950

As you can see, I didn't change anything that was previously stored under ~/.bundle. My patch just allows the user to move the contents around.

It would have been more correct if I had written:

Allow users to relocate ~/.bundle, or specific contents thereof, by setting environment variables.

In the documentation it's implied but not explicitly stated that config is a file. So maybe the documentation could be updated to reflect that, but my patch didn't affect anything along those lines.

@jasonkarns

This comment has been minimized.

Copy link
Contributor

commented May 8, 2018

@gwerbin cool. Since this feature was significantly driven by (but not solely by) the XDG request, I was easily mislead by the directories vs files. Was having an issue with BUNDLE_USER_CONFIG not working when set to a directory: $XDG_CONFIG_HOME/bundler. I think the added confusion was because BUNDLE_CONFIG already existed as an env var. Since that var already existed, it didn't make sense to me why BUNDLE_USER_CONFIG would be created with identical use/value. (Therefore I assumed that BUNDLE_USER_CONFIG would be a directory which would contain a file. ie BUNDLE_CONFIG essentially being the same as: $BUNDLE_USER_CONFIG/config.yml or something.

Anyway, thanks for the clarification.

@jasonkarns

This comment has been minimized.

Copy link
Contributor

commented May 8, 2018

Welp, I've found my problem!!! This apparently hasn't been released yet? I figured that since it was merged in September, and both 1.16.0 and 1.16.1 were released in October, December respectively, that this would have been included.

Does anyone have any rough ideas on when this might land?

@soc

This comment has been minimized.

Copy link

commented May 9, 2018

@jasonkarns I'm pretty confused about the whole state of this, too. I also checked this a while ago and assumed the change wasn't shipped because it failed to fix the problem people reported in #4333, and devs wanted to rework this before officially releasing it.

@segiddins

This comment has been minimized.

Copy link
Member

commented May 9, 2018

We cut the 1.16 branch before this was merged, and have been focusing on getting the 2.0 train out the door, and tend to not backport new features after a branch cut

@soc

This comment has been minimized.

Copy link

commented May 9, 2018

@segiddins Is there a chance to fix #4333 before 2.0 is released?

@segiddins

This comment has been minimized.

Copy link
Member

commented May 9, 2018

Since its a feature rather than a bug fix, it's likely it wont ship before 2.0

@soc

This comment has been minimized.

Copy link

commented May 9, 2018

Sorry, my comment could be interpreted two ways. What I meant is whether #4333 will be actually fixed in 2.0.

@colby-swandale colby-swandale added this to the 1.17.0 milestone Oct 2, 2018

colby-swandale added a commit that referenced this pull request Oct 9, 2018

Auto merge of #6024 - gwerbin:custom-user-bundle-dir, r=colby-swandale
Allow user to override ~/.bundle with environment variables

### What was the end-user problem that led to this PR?

As in #4333, users wanted a way to make Bundler respect the XDG specification.

### What was your diagnosis of the problem?

The directory `~/.bundle` was hard-coded and could not be configured.

### What is your fix for the problem, implemented in this PR?

Allow users to configure `~/.bundle` and its relevant sub-directories by setting environment variables. The environment variables and their fallbacks are as follows:

| variable | fallback if unset |
|---|---|
| `BUNDLE_USER_HOME` | `$HOME/.bundle` |
| `BUNDLE_USER_CACHE`  | `$BUNDLE_USER_HOME/cache` |
| `BUNDLE_USER_CONFIG` | `$BUNDLE_USER_HOME/config` |
| `BUNDLE_USER_PLUGIN` | `$BUNDLE_USER_HOME/plugin` |

### Why did you choose this fix out of the possible options?

Unlike #5787, This solution is not specific to the XDG specification. Users have all kinds of setups, and this is a very general system for allowing them to configure their development machines however they need. It tries to keep all files created by Bundler in the same place (as per #5787 (comment)), but allows the user to override that convention _if they really want to and they know what they are doing_.

If they want to use XDG for everything, they can do it by explicitly setting the `BUNDLE_USER_*` variables to the equivalent `XDG_DATA_*`. If they just want to get `.bundle` out of their home directory, they can do it by setting `BUNDLE_USER_HOME` and don't have to mess with the more specific env variables if they don't want to.

To me, this solution strikes the right balance between "fine-grained control for power users" and "simple, sane defaults for everyone else".

Please let me know if my tests can be improved. My only Ruby experience so far has been writing Homebrew formulas and configuring Jekyll, so I'm sure I have a lot to learn.

(cherry picked from commit e6cc7a2)

bundlerbot bot pushed a commit that referenced this pull request Oct 23, 2018

Merge #6754
6754: Add docs for configuring bundler folders through ENV vars r=colby-swandale a=colby-swandale

### Overview

This PR is just adding documentation for the feature in #6024.

Co-authored-by: Colby Swandale <me@colby.fyi>

colby-swandale added a commit that referenced this pull request Oct 24, 2018

Merge #6754
6754: Add docs for configuring bundler folders through ENV vars r=colby-swandale a=colby-swandale

### Overview

This PR is just adding documentation for the feature in #6024.

Co-authored-by: Colby Swandale <me@colby.fyi>
(cherry picked from commit 97a0430)

colby-swandale added a commit that referenced this pull request Oct 25, 2018

Merge branch '1-17-stable'
* 1-17-stable: (38 commits)
  Version 1.17.0 with changelog
  Merge #6754
  Version 1.17.0.pre.2 with changelog
  fix failing bundle remove specs
  Still document the `--force` option
  scope specs testing bundler 2 deprecations to bundler 1 only
  Merge #6718
  Merge #6707
  Merge #6702
  Merge #6316
  Auto merge of #6447 - agrim123:agr-update-error-message, r=segiddins
  Auto merge of #6513 - agrim123:agr-bundler-remove, r=indirect
  Auto merge of #6318 - jhawthorn:fix_incorrect_test_in_requires_sudo, r=segiddins
  Auto merge of #6450 - bundler:segiddins/bundle-binstubs-all, r=colby-swandale
  Auto merge of #6024 - gwerbin:custom-user-bundle-dir, r=colby-swandale
  Version 1.17.0.pre.1 with changelog
  Auto merge of #5964 - bundler:colby/deprecate-viz-command, r=segiddins
  Auto merge of #5986 - bundler:seg-jobs-count, r=indirect
  Auto merge of #5995 - bundler:seg-gvp-major, r=indirect
  Auto merge of #5803 - igorbozato:path_relative_to_pwd, r=indirect
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.