Skip to content

Implement config file location 'base' to config all environments of an interpreter #11487

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.

Sign up for GitHub

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

Merged
merged 5 commits into from
Jan 31, 2023
Merged

Implement config file location 'base' to config all environments of an interpreter #11487

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
b777bcd
Add the "base" config level concept, to allow configuration of all vi…
pelson Oct 5, 2022
4a87ab0
Merge branch 'main' into feature/base-prefix-config
uranusjr Nov 16, 2022
93ade85
Update docs/html/topics/configuration.md
pelson Nov 16, 2022
81d6053
Use the VIRTUAL_ENV environment variable in the configuration documen…
pelson Nov 17, 2022
8d34a11
Merge branch 'main' into feature/base-prefix-config
uranusjr Nov 25, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 16 additions & 5 deletions docs/html/topics/configuration.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,13 +19,14 @@ and how they are related to pip's various command line options.

## Configuration Files

Configuration files can change the default values for command line option.
They are written using a standard INI style configuration files.
Configuration files can change the default values for command line options.
They are written using standard INI style configuration files.

pip has 3 "levels" of configuration files:
pip has 4 "levels" of configuration files:

- `global`: system-wide configuration file, shared across users.
- `user`: per-user configuration file.
- `global`: system-wide configuration file, shared across all users.
- `user`: per-user configuration file, shared across all environments.
- `base` : per-base environment configuration file, shared across all virtualenvs with the same base. (available since pip 23.0)
- `site`: per-environment configuration file; i.e. per-virtualenv.

### Location
Expand All @@ -47,6 +48,9 @@ User

The legacy "per-user" configuration file is also loaded, if it exists: {file}`$HOME/.pip/pip.conf`.

Base
: {file}`\{sys.base_prefix\}/pip.conf`

Site
: {file}`$VIRTUAL_ENV/pip.conf`
```
Expand All @@ -63,6 +67,9 @@ User

The legacy "per-user" configuration file is also loaded, if it exists: {file}`$HOME/.pip/pip.conf`.

Base
: {file}`\{sys.base_prefix\}/pip.conf`

Site
: {file}`$VIRTUAL_ENV/pip.conf`
```
Expand All @@ -81,6 +88,9 @@ User

The legacy "per-user" configuration file is also loaded, if it exists: {file}`%HOME%\\pip\\pip.ini`

Base
: {file}`\{sys.base_prefix\}\\pip.ini`

Site
: {file}`%VIRTUAL_ENV%\\pip.ini`
```
Expand All @@ -102,6 +112,7 @@ order:
- `PIP_CONFIG_FILE`, if given.
- Global
- User
- Base
- Site

Each file read overrides any values read from previous files, so if the
Expand Down
1 change: 1 addition & 0 deletions news/9752.feature.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
In the case of virtual environments, configuration files are now also included from the base installation.
18 changes: 15 additions & 3 deletions src/pip/_internal/configuration.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,12 +36,20 @@
kinds = enum(
USER="user", # User Specific
GLOBAL="global", # System Wide
SITE="site", # [Virtual] Environment Specific
BASE="base", # Base environment specific (e.g. for all venvs with the same base)
SITE="site", # Environment Specific (e.g. per venv)
ENV="env", # from PIP_CONFIG_FILE
ENV_VAR="env-var", # from Environment Variables
)
OVERRIDE_ORDER = kinds.GLOBAL, kinds.USER, kinds.SITE, kinds.ENV, kinds.ENV_VAR
VALID_LOAD_ONLY = kinds.USER, kinds.GLOBAL, kinds.SITE
OVERRIDE_ORDER = (
kinds.GLOBAL,
kinds.USER,
kinds.BASE,
kinds.SITE,
kinds.ENV,
kinds.ENV_VAR,
)
VALID_LOAD_ONLY = kinds.USER, kinds.GLOBAL, kinds.BASE, kinds.SITE

logger = getLogger(__name__)

Expand Down Expand Up @@ -70,6 +78,7 @@ def get_configuration_files() -> Dict[Kind, List[str]]:
os.path.join(path, CONFIG_BASENAME) for path in appdirs.site_config_dirs("pip")
]

base_config_file = os.path.join(sys.base_prefix, CONFIG_BASENAME)
site_config_file = os.path.join(sys.prefix, CONFIG_BASENAME)
legacy_config_file = os.path.join(
os.path.expanduser("~"),
Expand All @@ -78,6 +87,7 @@ def get_configuration_files() -> Dict[Kind, List[str]]:
)
new_config_file = os.path.join(appdirs.user_config_dir("pip"), CONFIG_BASENAME)
return {
kinds.BASE: [base_config_file],
kinds.GLOBAL: global_config_files,
kinds.SITE: [site_config_file],
kinds.USER: [legacy_config_file, new_config_file],
Expand Down Expand Up @@ -344,6 +354,8 @@ def iter_config_files(self) -> Iterable[Tuple[Kind, List[str]]]:
# The legacy config file is overridden by the new config file
yield kinds.USER, config_files[kinds.USER]

yield kinds.BASE, config_files[kinds.BASE]
Copy link
Member

Choose a reason for hiding this comment

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

The "base" location should be skipped outside of virtual environments, where base_prefix and prefix are the same.

Copy link
Member

Choose a reason for hiding this comment

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

We should also provide a (temporary) opt-out from this, for users who might find this change to be disruptive.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The "base" location should be skipped outside of virtual environments, where base_prefix and prefix are the same.

I considered this before opening the PR, but the problem is that we don't know what config files need to be loaded at this point - there may be a load-only of BASE, which would mean we don't get that config file loaded in non-venv contexts. Perhaps that is OK though - I would add a caveat to the docs to say BASE is only considered when a virtual environment is active.

I was further dissuaded from my implementation by the todo at the top of the function, which asks for less logic in what config files to choose https://github.com/pypa/pip/pull/11487/files#diff-eadf79f17f8c7b56d3743fb4bb0ef3c2c2e062e493e8c25962f3319ef264f8bfR335.

We should also provide a (temporary) opt-out from this, for users who might find this change to be disruptive.

I agree. I could add something specific to BASE, or I could look into allowing the precedence order to be overridden (and to disallow certain levels). Something like:

config-precedence =
    env
    global
    user
    site
    env_var

That would amount to quite a big change though - and might require some thought about doing the config loading in two passes (first to determine the precedence, then to actually load it in the desired order) - this would reduce to a single pass in the case that the precedence isn't overridden. I would be happy to talk about this offline/elsewhere if this is something you would like me to pursue.

If you want me to keep it BASE scoped (and simpler), then suggestions are very welcome on the best approach - a simple env var, for example?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I took a poke at implementing config-precedence config at pelson@0553eb1. I think it turned out quite well in fact - might be a nice alternative option for allowing users to opt-out of BASE loading.

I would be happy to discuss this on a separate issue, or on discord, if this is interesting to pursue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since the temporary opt-out comment is withdrawn in #11487 (comment), the remainging discussion is:

The "base" location should be skipped outside of virtual environments, where base_prefix and prefix are the same.

In my response, I highlight that this particular function has a comment:

# SMELL: Move the conditions out of this function

I agree with the suggestion that the function smells, as the function's objective should be to unconditionally enumerate the locations that configuration may come from, with _load_config_files determining if those files should be loaded or not.

For this reason, I added the BASE config file unconditionally. If we want to optimise and avoid BASE and SITE being loaded if they are the same, then I would put this logic in the _load_config_files method. Would be happy to add this, if it is considered important.


# finally virtualenv configuration first trumping others
yield kinds.SITE, config_files[kinds.SITE]

Expand Down
33 changes: 31 additions & 2 deletions tests/unit/test_configuration.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,12 +24,18 @@ def test_user_loading(self) -> None:
self.configuration.load()
assert self.configuration.get_value("test.hello") == "2"

def test_site_loading(self) -> None:
self.patch_configuration(kinds.SITE, {"test.hello": "3"})
def test_base_loading(self) -> None:
self.patch_configuration(kinds.BASE, {"test.hello": "3"})

self.configuration.load()
assert self.configuration.get_value("test.hello") == "3"

def test_site_loading(self) -> None:
self.patch_configuration(kinds.SITE, {"test.hello": "4"})

self.configuration.load()
assert self.configuration.get_value("test.hello") == "4"

def test_environment_config_loading(self, monkeypatch: pytest.MonkeyPatch) -> None:
contents = """
[test]
Expand Down Expand Up @@ -107,6 +113,15 @@ def test_no_such_key_error_message_missing_option(self) -> None:
with pytest.raises(ConfigurationError, match=pat):
self.configuration.get_value("global.index-url")

def test_overrides_normalization(self) -> None:
# Check that normalized names are used in precedence calculations.
# Reminder: USER has higher precedence than GLOBAL.
self.patch_configuration(kinds.USER, {"test.hello-world": "1"})
self.patch_configuration(kinds.GLOBAL, {"test.hello_world": "0"})
self.configuration.load()

assert self.configuration.get_value("test.hello_world") == "1"


class TestConfigurationPrecedence(ConfigurationMixin):
# Tests for methods to that determine the order of precedence of
Expand All @@ -133,6 +148,13 @@ def test_env_overides_global(self) -> None:

assert self.configuration.get_value("test.hello") == "0"

def test_site_overides_base(self) -> None:
self.patch_configuration(kinds.BASE, {"test.hello": "2"})
self.patch_configuration(kinds.SITE, {"test.hello": "1"})
self.configuration.load()

assert self.configuration.get_value("test.hello") == "1"

def test_site_overides_user(self) -> None:
self.patch_configuration(kinds.USER, {"test.hello": "2"})
self.patch_configuration(kinds.SITE, {"test.hello": "1"})
Expand All @@ -147,6 +169,13 @@ def test_site_overides_global(self) -> None:

assert self.configuration.get_value("test.hello") == "1"

def test_base_overides_user(self) -> None:
self.patch_configuration(kinds.USER, {"test.hello": "2"})
self.patch_configuration(kinds.BASE, {"test.hello": "1"})
self.configuration.load()

assert self.configuration.get_value("test.hello") == "1"

def test_user_overides_global(self) -> None:
self.patch_configuration(kinds.GLOBAL, {"test.hello": "3"})
self.patch_configuration(kinds.USER, {"test.hello": "2"})
Expand Down
2 changes: 1 addition & 1 deletion tests/unit/test_options.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -588,7 +588,7 @@ def test_venv_config_file_found(self, monkeypatch: pytest.MonkeyPatch) -> None:
for _, val in cp.iter_config_files():
files.extend(val)

assert len(files) == 4
assert len(files) == 5

@pytest.mark.parametrize(
"args, expect",
Expand Down