search

pypa / pip

The Python package installer
https://pip.pypa.io/
MIT License
9.51k stars 3.02k forks source link

Debundled appdirs support #7784

Closed felixonmars closed 3 years ago

felixonmars commented 4 years ago

Environment

Description Looks like after https://github.com/pypa/pip/pull/7501 merged the vendored copy of appdirs is no longer behaving identically as the upstream one. The tests are failing after debundling appdirs.

I would like to know if debundled appdirs must be patched or are the test failures ignorable?

=================================== FAILURES ===================================
________________ TestSiteConfigDirs.test_site_config_dirs_linux ________________

self = <tests.unit.test_appdirs.TestSiteConfigDirs object at 0x7fb104365790>
monkeypatch = <_pytest.monkeypatch.MonkeyPatch object at 0x7fb104365820>

    def test_site_config_dirs_linux(self, monkeypatch):
        monkeypatch.setattr(_appdirs, "system", "linux2")
        monkeypatch.setattr(os, "path", posixpath)
        monkeypatch.delenv("XDG_CONFIG_DIRS", raising=False)
        monkeypatch.setattr(sys, "platform", "linux2")

>       assert appdirs.site_config_dirs("pip") == [
            '/etc/xdg/pip',
            '/etc'
        ]
E       AssertionError: assert ['/etc/xdg/pip'] == ['/etc/xdg/pip', '/etc']
E         Right contains one more item: '/etc'
E         Use -v to get the full diff

tests/unit/test_appdirs.py:122: AssertionError
___________ TestSiteConfigDirs.test_site_config_dirs_linux_override ____________

self = <tests.unit.test_appdirs.TestSiteConfigDirs object at 0x7fb1042ed8e0>
monkeypatch = <_pytest.monkeypatch.MonkeyPatch object at 0x7fb1042edcd0>

    def test_site_config_dirs_linux_override(self, monkeypatch):
        monkeypatch.setattr(_appdirs, "system", "linux2")
        monkeypatch.setattr(os, "path", posixpath)
        monkeypatch.setattr(os, "pathsep", ':')
        monkeypatch.setenv("XDG_CONFIG_DIRS", "/spam:/etc:/etc/xdg")
        monkeypatch.setattr(sys, "platform", "linux2")

>       assert appdirs.site_config_dirs("pip") == [
            '/spam/pip',
            '/etc/pip',
            '/etc/xdg/pip',
            '/etc'
        ]
E       AssertionError: assert ['/spam/pip',.../etc/xdg/pip'] == ['/spam/pip',.../pip', '/etc']
E         Right contains one more item: '/etc'
E         Use -v to get the full diff

tests/unit/test_appdirs.py:134: AssertionError
_____________ TestSiteConfigDirs.test_site_config_dirs_linux_empty _____________

self = <tests.unit.test_appdirs.TestSiteConfigDirs object at 0x7fb1043bd040>
monkeypatch = <_pytest.monkeypatch.MonkeyPatch object at 0x7fb1043bd4f0>

    def test_site_config_dirs_linux_empty(self, monkeypatch):
        monkeypatch.setattr(_appdirs, "system", "linux2")
        monkeypatch.setattr(os, "path", posixpath)
        monkeypatch.setattr(os, "pathsep", ':')
        monkeypatch.setenv("XDG_CONFIG_DIRS", "")
        monkeypatch.setattr(sys, "platform", "linux2")
>       assert appdirs.site_config_dirs("pip") == ['/etc/xdg/pip', '/etc']
E       AssertionError: assert ['/pip'] == ['/etc/xdg/pip', '/etc']
E         At index 0 diff: '/pip' != '/etc/xdg/pip'
E         Right contains one more item: '/etc'
E         Use -v to get the full diff

tests/unit/test_appdirs.py:147: AssertionError
____________________ TestUserDataDir.test_user_data_dir_osx ____________________

self = <tests.unit.test_appdirs.TestUserDataDir object at 0x7fb1043c2b80>
monkeypatch = <_pytest.monkeypatch.MonkeyPatch object at 0x7fb1043bd700>

    def test_user_data_dir_osx(self, monkeypatch):
        monkeypatch.setattr(_appdirs, "system", "darwin")
        monkeypatch.setattr(os, "path", posixpath)
        monkeypatch.setenv("HOME", "/home/test")
        monkeypatch.setattr(sys, "platform", "darwin")

        if os.path.isdir('/home/test/Library/Application Support/'):
            assert (appdirs.user_data_dir("pip") ==
                    "/home/test/Library/Application Support/pip")
        else:
>           assert (appdirs.user_data_dir("pip") ==
                    "/home/test/.config/pip")
E           AssertionError: assert '/home/test/L...n Support/pip' == '/home/test/.config/pip'
E             - /home/test/Library/Application Support/pip
E             + /home/test/.config/pip

tests/unit/test_appdirs.py:200: AssertionError
__________________ TestUserConfigDir.test_user_config_dir_osx __________________

self = <tests.unit.test_appdirs.TestUserConfigDir object at 0x7fb1042ed820>
monkeypatch = <_pytest.monkeypatch.MonkeyPatch object at 0x7fb1042edc70>

    def test_user_config_dir_osx(self, monkeypatch):
        monkeypatch.setattr(_appdirs, "system", "darwin")
        monkeypatch.setattr(os, "path", posixpath)
        monkeypatch.setenv("HOME", "/home/test")
        monkeypatch.setattr(sys, "platform", "darwin")

        if os.path.isdir('/home/test/Library/Application Support/'):
            assert (appdirs.user_data_dir("pip") ==
                    "/home/test/Library/Application Support/pip")
        else:
>           assert (appdirs.user_data_dir("pip") ==
                    "/home/test/.config/pip")
E           AssertionError: assert '/home/test/L...n Support/pip' == '/home/test/.config/pip'
E             - /home/test/Library/Application Support/pip
E             + /home/test/.config/pip
uranusjr commented 4 years ago

Duplicate to #7690. The fix will be included in the next pip release. Sorry for the inconvinience!

felixonmars commented 4 years ago

@uranusjr No, it's not the same problem. I already applied #7690 but the difference in behavior caused these test failures.

uranusjr commented 4 years ago

Ah, I see, sorry for messing up. I think whether you want to patch appdirs depends on what you want. Most of pip’s patches are for cross-platform compatibility (the changes are documented in the patch file). IIRC the only patch relevant to Arch are

eli-schwartz commented 4 years ago

I do not understand how you can violate your contract to not modify vendored projects willy-nilly like this. At the very least, if you think that XDG_CONFIG_DIRS="" should be treated identically to unset (my vague memory of the XDG spec is that it should be?) that would be "fixing a bug in appdirs", so why are you fixing bugs in appdirs.patch rather than pushing bugfixes upstream... EDIT: yes, double-checked, and this is an upstream bug which makes it even more important that it gets submitted upstream.

As for /etc vs. /etc/xdg, it seems you disagree with the intended behavior of appdirs, so you are modifying the user-facing behavior. If that's so important, you'd think the appdirs developers might be interested in discussing it as well. :/

Both of these changes explicitly violate the directive in https://github.com/pypa/pip/blob/master/src/pip/_vendor/README.rst, and furthermore (or perhaps because they violate the directive?) they are not described in the section "Modifications".

xavfernandez commented 4 years ago

To add some context, the appdirs vendoring is kinda different from the other ones as it was partially (i.e. 130 lines from the 480 of appdirs) added in pip 6.0 via 52ca0260 without declaring itself as vendored.

The "retrofit" vendoring happened in pip 20 to fix this special case but patches were needed to make it transparent for pip users and unfortunately in the process the vendoring directives were forgotten. Let's say it was semi-vendored to match the semi-supported debundling ;)

Ideally those patches should indeed be either upstreamed or included instead in pip wrapping code https://github.com/pypa/pip/blob/master/src/pip/_internal/utils/appdirs.py.

Concerning XDG_CONFIG_DIRS, https://github.com/ActiveState/appdirs/pull/131/files seems to be what you are looking for. Concerning the other patches, pull requests are always welcome :)

eli-schwartz commented 4 years ago

The XDG_CONFIG_DIRS change looks to be exactly what's needed, thanks. Would have been nice to see that happen years ago, but hmm, better late than never.

As for the rest... Perhaps someone familiar with why pip needs them, could open an issue to discuss each modification with upstream?

pradyunsg commented 3 years ago

Fixed in #7690.

pradyunsg commented 3 years ago

Ah, no, I misread.

pradyunsg commented 3 years ago

The exact patch that pip makes to the vendored appdirs today, can be found at https://github.com/pypa/pip/blob/main/tools/vendoring/patches/appdirs.patch.

We're going to be moving away from apprdirs, to platformdirs -- which is a better maintained fork of the project now. Consolidating this into https://github.com/pypa/pip/issues/10178, since I don't think this would be relevant once that is done.