Correctly assign configuration to origin files with pip config

[chrysle]

Fixes #12099

This pull requests extends the pip configuration dictionary to be nested, allowing the configuration keys to be connected with the origin files. It will look like this:

{'/etc/xdg/pip/pip.conf': {'install.require-virtualenv': 'true'}, '/etc/pip.conf': {'global.timeout': '60'}, '/home/user/.pip/pip.conf': {'install.require-virtualenv': 'true'}, '/home/user/.config/pip/pip.conf': {'global.timeout': '60', 'test.blah': '1', 'test.blaha': '1'}}

pip config debug correctly separates the configuration:

$ python -m pip config debug
env_var:
env:
global:
  /etc/xdg/xdg-ubuntu/pip/pip.conf, exists: False
  /etc/xdg/pip/pip.conf, exists: True
    install.require-virtualenv: true
  /etc/pip.conf, exists: True
    global.timeout: 60
site:
  /home/user/.virtualenvs/pip-dev/pip.conf, exists: False
user:
  /home/user/.pip/pip.conf, exists: True
    install.require-virtualenv: true
  /home/user/.config/pip/pip.conf, exists: True
    global.timeout: 60
    test.blah: 1
    test.blaha: 1

I'm not satisfied with the implementation, though... Happy about review feedback!

[chrysle]

Thank you, updated!

[pradyunsg]

Thanks for filing this PR! ^>^

Could you update the output to indent the configuration values by one level more compared to the output line about the origin file?

i.e.

$ python -m pip config debug
env_var:
env:
global:
  /etc/xdg/xdg-ubuntu/pip/pip.conf, exists: False
  /etc/xdg/pip/pip.conf, exists: True
    install.require-virtualenv: true
  /etc/pip.conf, exists: True
    global.timeout: 60
site:
  /home/user/.virtualenvs/pip-dev/pip.conf, exists: False
user:
  /home/user/.pip/pip.conf, exists: True
    install.require-virtualenv: true
  /home/user/.config/pip/pip.conf, exists: True
    global.timeout: 60
    test.blah: 1
    test.blaha: 1

[chrysle]

Thanks for filing this PR! ^>^

You're welcome!

Could you update the output to indent the configuration values by one level more compared to the output line about the origin file?

I'm sorry, that's actually the default behaviour - my sed call messed that up :/

Updated the example. I sure agree it looks better!

[chrysle]

Thank you for the code refinery suggestions! I've applied them.

[uranusjr]

Looks good. Can we add some tests for this?

[chrysle]

@uranusjr I have added a test. Do you have a suggestion how to touch the configuration file platform-independent?

[uranusjr]

Looks like there’s some problem with test setup, the config is not where you expect it to be.

[chrysle]

Don't know why, but on Windows the the new_config_file is in a subdirectory of the one used for the preceding test_user_values test:

new_config_file = 'C:\\Temp\\pytest-of-runneradmin\\pytest-1\\popen-gw1\\test_user_values0\\home\\AppData\\Roaming\\pip\\pip.ini'

Can I run a simple replace on that?

[pfmoore]

Can I run a simple replace on that?

IMO, only if you add a comment to the test explaining why you needed to do so for future maintainers. And if you can write such a comment, you'd probably be better just fixing the test to look in the correct place 🙂

[pradyunsg]

I think that's actually state leaking between tests, which is what is causing the failure here. Doing a replace seems like a bodge and, ideally, we would instead change test_user_values to not leak state into other tests...

Maybe, that should delete new_config_file before exiting. @chrysle Could you try adding an os.remove(new_config_file) to the end of test_user_values?

[pradyunsg]

@chrysle Please run git pull locally on this branch, before making any more changes. :)

[chrysle]

Sure, done; I wanted to rebase after checking if your solution would work.

Do you have another suggestion how to handle the issue? I can't test on Windows, unfortunately.

[pradyunsg]

Could you try adding an os.remove(new_config_file) to the end of test_user_values?

Have you tried this yet?

[ichard26]

@chrysle are you interested in fixing the failing test on Windows CI and fixing the merge conflicts? This PR looks pretty close to be ready to be merged. It's okay if you aren't!

[chrysle]

@chrysle are you interested in fixing the failing test on Windows CI

I am, unfortunately I don't know how to do that. I don't use Windows and thus can not investigate this properly. Any help would be greatly appreciated.

[ichard26]

I was confused with the code duplication. I tried to remove it and realized that envvars were not migrated to the dict[Kind, dict[Filename, Any]] format. Fixing envvars to use a fake file was enough to eliminate all of the weird duplication. I've reviewed the rest of the changes and they look great!

I'm trying my darnest to fix the tests on Windows. It looks like state leaks between the tests conditionally (probably based off whether the OS global configuration directory is writable or not). Once that's fixed, I'll merge this.

Thank you @chrysle for the contribution!

[ichard26]

I think I figured it out, platformdirs is caching the Windows user site directory which obviously is not going to end well here. I added a teardown fixture that clears this cache. Sigh.

[ichard26]

That did not fix it. Whelp. I give up for the night. Will come back to this later.

[ichard26]

I'm going to add a replace call to this test because nothing in test_configuration.py seems to be suspect. However, on my Windows machine, I added an assert to check what the internal platformdirs functions are returning as the user configuration directory at test teardown, and the ctypes function seems to be caching, lagging on, or ignoring updates to envvar APPDATA.

The ctypes function is the default one that platformdirs uses when ctypes/windll is available.

It's also possible that our test suite isolation fixture is broken on Windows and perhaps APPDATA is an unsupported way of updating the user app data folder...

[ichard26]

Oh wait, updating APPDATA and friends is only going to affect the Windows APIs in a new subprocess. This is noted within the test isolation logic.

pip/tests/conftest.py

Lines 236 to 248 in 8c3a68d

	if sys.platform == "win32":
	# Note: this will only take effect in subprocesses...
	home_drive, home_path = os.path.splitdrive(home_dir)
	monkeypatch.setenv("USERPROFILE", home_dir)
	monkeypatch.setenv("HOMEDRIVE", home_drive)
	monkeypatch.setenv("HOMEPATH", home_path)
	for env_var, sub_path in (
	("APPDATA", "AppData/Roaming"),
	("LOCALAPPDATA", "AppData/Local"),
	):
	path = os.path.join(home_dir, *sub_path.split("/"))
	monkeypatch.setenv(env_var, path)
	os.makedirs(path)

The better fix is to run get_configuration_files() in a subprocess... great.

[ichard26]

Sorry. I didn't mean to use my time travel abilities on pip :P

In all seriousness. I dual boot Ubuntu and Windows and every time I switch OSes, the system time shifts 4 hours back or forth. Ubuntu resyncs the system time quickly, but Windows doesn't 😕. I've stopped manually digging through the settings to resync the time so now my commit times on Windows are all out of wack, haha.