Make config location relative to the current working directory#3963
Closed
deivid-rodriguez wants to merge 24 commits intomasterfrom
Closed
Make config location relative to the current working directory#3963deivid-rodriguez wants to merge 24 commits intomasterfrom
deivid-rodriguez wants to merge 24 commits intomasterfrom
Conversation
deivid-rodriguez
force-pushed
the
config_location
branch
from
1f1958a to
82b36bb
Compare
deivid-rodriguez
force-pushed
the
config_location
branch
from
82b36bb to
debb6c1
Compare
This was referenced Dec 13, 2020
deivid-rodriguez
force-pushed
the
config_location
branch
from
debb6c1 to
4c970f2
Compare
deivid-rodriguez
force-pushed
the
config_location
branch
7 times, most recently
from
5e02d6f to
1a49c30
Compare
4 tasks
deivid-rodriguez
force-pushed
the
config_location
branch
3 times, most recently
from
b0aba60 to
5f5f4e6
Compare
4 tasks
deivid-rodriguez
force-pushed
the
config_location
branch
2 times, most recently
from
aebbf81 to
fa98e85
Compare
So we can instead call `parent` on it.
It's the only part that needs "root folder resultion" to figure out the folder for the cache, but it's only needed for some things, so run that logic lazily when needed. The motivation for this refactoring is a bit complicated: * In our development environment, we use `LockfileParser` to figure out the exact version of each development dependency we need to load. We could do this via `bundler/setup` but we have never got that to work for bundler specs themselves (use bundler to test bundler). * Our usage of `LockfileParser` relies on the presence of a `.bundle/config` file in the root of the repo, otherwise "root folder resolution" would fail to find both a `Gemfile` or a `.bundle/config` file and would fail. * An upcoming bug fix to make `bundler/setup` also consider gemfile configured through `.bundle/config`, will make it so that the `.bundle/config` file at the root of this repository will be used by bundler development tasks, and that will break things. So I want to remove the `.bundle/config` file, but to do that I need to make `LockfileParser` not do root folder resolution by default. Hence, this change.
Instead, use `ENV`, just like `bundler` tasks do.
Rubygems `Gem.user_home` never returns `nil`. It calls `Dir.home` internally which either raises, or returns something different from `nil`, and when `Dir.home` raises, `Gem.user_home` rescues the error and returns the root path.
Now the directory hierarchy is searched up for a ".bundle" directory. If none is found, we use the folder of the current Gemfile (which we know won't come from configuration, since none was found). If none is found either, the current folder is used. This is more intuitive than choosing the configuration location to be the folder of the Gemfile being used, but also it will allow us to respect the Gemfile configured via settings when requiring `bundler/setup`, since the current workaround to make that work doesn't apply to that case since it lives at `bundler/cli`.
Root is compulsory.
Previously it was being set when initializing the CLI, but that wouldn't apply to directly using `bundler` via `-rbundler/setup`. Since now resolving the settings root doesn't need a Gemfile, we can move the logic to the `SharedHelpers.find_gemfile` method, so that it happily applies to `bundler/setup` as well.
It shouldn't make a difference but CLI flags should map to temporary settings, which have higher priority than ENV.
It took me a while to figure out why we were doing this, add a comment to try clarify.
This seems to be causing some issues with the new test, but it does not seem needed. I don't recall why I added. As long as our development binstubs load the rubygems version switcher initially, the proper path to rubygems will be added to RUBYOPT and preserved for all future subprocesses. I don't think further manipulations are needed.
This reverts commit 23010a1.
deivid-rodriguez
force-pushed
the
config_location
branch
from
fa98e85 to
9eca72f
Compare
4 tasks
Contributor
|
what is missing here to get this merged? |
Contributor
Author
|
This needs a rebase and a second look to evaluate compatibility. I'll look into it next year. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR ports rubygems/bundler#6713 from the old repository.
What was the end-user problem that led to this PR?
There are two related end-user problems.
When a custom
Gemfileis configured viaBUNDLE_GEMFILEand we runbundle config, the local config is not generated in the current working directory. The reason is in the diagnosis section, but the end user problem is that it is very counterintuitive. On bundler 2 this is aggravated because of "sticky options". See --standalone generates file in incorrect PWD rubygems/bundler#2048 (comment) for an example.When configuring a custom gemfile via settings, it is not respected when requiring
bundler/setup. See using bundler plugins results in gem metadata always loading from non-default sources even in deployment mode #6589.What was your diagnosis of the problem?
My diagnosis was that the config location is currently resolved from the current
Gemfilelocation, and that leads to configuration being created on counterintuitive locations. For example, you haveBUNDLE_GEMFILEset to a particular non-root location, and runbundle configon the root folder, but no local config is apparently generated... until you figure out it's being generated in the same folderBUNDLE_GEMFILEis pointing to.Also, since the config location depends on the location of the current
Gemfile, we can't consistently resolve theGemfilefrom configuration, because of a chicken-and-egg problem. We need theGemfileto figure out where the config is, but we need the config to figure out where theGemfileis. This is why #3363 can't currently work.What is your fix for the problem, implemented in this PR?
My fix was to change how we resolve the configuration root, and instead look up in the directory hierarchy for an existing local config, and otherwise default to the current working directory. With that in place, fixing #6589 is easy. My changes led to a single spec failure. Even if it's just one, it is backwards incompatible if backwards compatibility is defined by our current set of specs. Since I'd like to be able to opt-in to the new behavior right now on bundler 1, I made a feature flag for this change. This feature flag needed to be different from the other feature flags we currently have, because it can't be config-based (chicken-and-egg problem again). So I introduced "env-based feature flags".
Once we have the new configuration root resolution in place, the fix for #6589 is to add knowledge to the
find_gemfilemethod to read from settings (and not just from env).Why did you choose this fix out of the possible options?
I chose this fix because changing the way we resolve configuration seemed like the only way to resolve the problems that I wanted to fix here.
Fixes #3363.
Fixes rubygems/bundler#2048 (the remaining part of it, rubygems/bundler#2048 (comment) and rubygems/bundler#2048 (comment)).
Fixes #3294.
Tasks:
I will abide by the code of conduct.