pip icon indicating copy to clipboard operation
pip copied to clipboard

Correctly assign configuration to origin files with `pip config`

Open chrysle opened this issue 2 years ago • 15 comments

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 avatar Aug 03 '23 09:08 chrysle

Thank you, updated!

chrysle avatar Aug 10 '23 17:08 chrysle

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

pradyunsg avatar Aug 10 '23 23:08 pradyunsg

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 avatar Aug 11 '23 18:08 chrysle

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

chrysle avatar Aug 14 '23 18:08 chrysle

Looks good. Can we add some tests for this?

uranusjr avatar Aug 31 '23 07:08 uranusjr

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

chrysle avatar Sep 05 '23 19:09 chrysle

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

uranusjr avatar Sep 07 '23 08:09 uranusjr

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?

chrysle avatar Sep 07 '23 17:09 chrysle

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 🙂

pfmoore avatar Sep 07 '23 17:09 pfmoore

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 avatar Sep 07 '23 22:09 pradyunsg

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

pradyunsg avatar Sep 09 '23 23:09 pradyunsg

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.

chrysle avatar Sep 10 '23 07:09 chrysle

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

Have you tried this yet?

pradyunsg avatar Oct 01 '23 13:10 pradyunsg

@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!

ichard26 avatar May 11 '24 20:05 ichard26

@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.

chrysle avatar May 12 '24 05:05 chrysle