-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Prioritize path.system
over path
when it makes sense
#3933
Conversation
To not mention deprecated `--path` flag.
This message is an informational piece, not a warning.
Since the behaviour is not specific to bundler 3.
babdec6
to
6ab88b2
Compare
Always fetch the value, since we'll be using it right after.
Keys should be defined in "ENV variable format", since that's what other helpers expect, and makes it consistent with how the other configurations are accessed. This fixes the issue of the `Bundler.settings.locations` hash never returning a `:default` key. It doesn't seem to cause any issues in practice, but still.
Makes code more consistent, and allows further refactoring.
Since all settings are hashes now.
By extracting a `configs` helper which holds the different configuration levels in the right order.
If `path.system` is configured locally, and `path` is configured globally, gems should be installed to `path.system`.
6ab88b2
to
418998c
Compare
Is this also a refactor? It's hard to spot what exactly the "fix" for the linked issue is. |
Yes, I had to introduce quite a bit of refactoring to figure this out. The fix is the last commit: 418998c (the one including a regression spec, the rest of commits should keep specs green, but they don't fix anything). The fix is essentially, instead of resolving |
@colby-swandale Do you plan on reviewing this? I was thinking of going ahead and merging if not. I could split the refactoring to a separate changeset but since the refactoring is aimed at making the fix easier, and backporting the fix will require backporting the refactoring, I figured it was best to keep them together. I think a "commit by commit" review should work. |
Sorry but I don't have the mental bandwidth to review at the moment. |
No problem. I'll merge for now, and we can iterate if any problems are found. |
Prioritize `path.system` over `path` when it makes sense (cherry picked from commit 257005f)
Prioritize `path.system` over `path` when it makes sense (cherry picked from commit 257005f)
Prioritize `path.system` over `path` when it makes sense (cherry picked from commit 257005f)
Prioritize `path.system` over `path` when it makes sense (cherry picked from commit 257005f)
Prioritize `path.system` over `path` when it makes sense (cherry picked from commit 257005f)
Prioritize `path.system` over `path` when it makes sense (cherry picked from commit 257005f)
What was the end-user or developer problem that led to this PR?
If a user configures
path.system true
locally, andpath foo
globally, gems are still installed tofoo
, even if they should be installed toGEM_PATH
, give that local configuration should have more priority.What is your fix for the problem, implemented in this PR?
My fix is to remove some special logic that was preventing this case to work, and also to relax the
path
andpath.system
validation that makes sure there are no conflicts between them to allow this case.Fixes #3931.
Tasks:
I will abide by the code of conduct.