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 3 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
27 changes: 19 additions & 8 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,8 +48,11 @@ 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`
: {file}`\{sys.prefix\}/pip.conf`
```

```{tab} MacOS
Expand All @@ -63,8 +67,11 @@ 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`
: {file}`\{sys.prefix\}/pip.conf`
Copy link
Member

Choose a reason for hiding this comment

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

Don't change this, since VIRTUAL_ENV is more readily understood to be the directory containing the virtualenv.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

```

```{tab} Windows
Expand All @@ -81,8 +88,11 @@ 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`
: {file}`\{sys.prefix\}\\pip.ini`
Copy link
Member

Choose a reason for hiding this comment

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

Don't change this, since VIRTUAL_ENV is more readily understood to be the directory containing the virtualenv.

Copy link
Contributor Author

@pelson pelson Oct 6, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I agree on the understanding, but until I re-read https://docs.python.org/3/library/venv.html#creating-virtual-environments, thought you were technically it is wrong.

It turns out that it is indeed documented that %VIRTUAL_ENV% must be set (this has been documented since Python 3.8). Unfortunately that is a bug in the Python docs, AFAICS - there is no actual implementation to ensure that VIRTUAL_ENV is actually set:

$ python3.9 -m venv /tmp/a-new-venv
$ /tmp/a-new-venv/bin/python -c "import os; print(f'Val: {os.getenv(\"VIRTUAL_ENV\")}')"
Val: None

Therefore the CPython documentation that states:

When a virtual environment is active, the VIRTUAL_ENV environment variable is set to the path of the virtual environment. This can be used to check if one is running inside a virtual environment.

Is sadly misguided - it isn't safe to use that environment variable to determine virtual-environment-ness IMO (and using env-vars to determine that sounds like a bad idea anyway - residual environment variables are a concern there).

(btw: Recommended it gets reverted in python/cpython#21970 (comment))

OK that is a bit of an aside. I'm open to reverting to %VIRTUAL_ENV%, but want to recognise that it is not strictly accurate, and that to document the base location, I will be using sys.base_prefix anyway.

Do you still want me to revert?

Copy link
Member

Choose a reason for hiding this comment

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

As this is documentation, and $VIRTUAL_ENV is commonly understood (right or not) to mean the virtual environment's directory, I agree with @pradyunsg that this would be better reverted (in the 3 places it occurs)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the clarity. Commit added to this tune.
FWIW, in the meantime, I also clarified the Python documentation on the use of the VIRTUAL_ENV environment variable at python/cpython#98350.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

```

### `PIP_CONFIG_FILE`
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