fix: Broken cache on Windows

[serverwentdown]

(Backport to 1.1: #4549)

The previous implementation would fail to install packages on Windows because it creates a Path starting with a slash. Such a Path is invalid on Windows. Instead, use the utility function url_to_path.

Additionally (not a bug in 1.1, migrated from separate PR at #4532), Links and Paths were passed in to pip_install as a string. This makes it hard for pip_install to parse out the extension of the file to install. By accepting only Links or Paths, pip_install can parse out the extension using the Link. This also resolves a bug when Executor._download_link returns a Link, which can't be parsed by Path(str(link)) on Windows.

This was overlooked when running on Linux because file:/path/to/cache is somehow a valid pip install path in Python on Linux, but not a valid path on Windows. With this PR, pip install is always called with a proper Path (/path/to/cache) and never a Path-parsed Link (file:/path/to/cache)

Pull Request Check List

Resolves: #4479 #4535

• Added tests for changed code
• Updated documentation for changed code. (N/A)

[Ivoz]

I wonder if we can create a testcase that will fail that this fixes?

[serverwentdown]

@Ivoz do you know where to put full end-to-end tests (running poetry commands)?

[serverwentdown]

I have yet to test the tests without the fix on Windows, but this PR is ready.

[mrogaski]

I confirmed that this fixes #4541.

[serverwentdown]

@finswimmer @sdispater I'd like a review of this PR because this is a major issue for all Windows users

[Ivoz]

Win 10 x64, Python 3.9.6

@ master 82459bd

Test session starts (platform: win32, Python 3.9.6, pytest 6.2.5, pytest-sugar 0.9.4)
rootdir: C:\Users\Matthew\code\poetry-test
plugins: cov-2.12.1, mock-3.6.1, sugar-0.9.4
collecting ...
 tests/utils/test_pip.py ✓✓                                                                               13% █▍
 tests/installation/test_executor.py ✓✓✓✓✓✓✓✓✓✓✓✓✓                                                       100% ██████████

Results (18.20s):
      15 passed

Paste contents of the two tests/ files from this PR @ 25adda8

Test session starts (platform: win32, Python 3.9.6, pytest 6.2.5, pytest-sugar 0.9.4)
rootdir: C:\Users\Matthew\code\poetry-test
plugins: cov-2.12.1, mock-3.6.1, sugar-0.9.4
collecting ...
 tests/utils/test_pip.py ✓                                                                                 6% ▋

―――――――――――――――――――――――――――――――――――――――――――――――― test_pip_install_link ――――――――――――――――――――――――――――――――――――――――――――――――

tmp_dir = 'C:\\Users\\Matthew\\AppData\\Local\\Temp\\poetry_kosx30x2'
tmp_venv = VirtualEnv("C:\Users\Matthew\AppData\Local\Temp\poetry_kosx30x2\venv")
fixture_dir = <function fixture_dir.<locals>._fixture_dir at 0x00000216DF58E9D0>

    def test_pip_install_link(tmp_dir, tmp_venv, fixture_dir):
        file_path = Link(
            path_to_url(fixture_dir("distributions/demo-0.1.0-py2.py3-none-any.whl"))
        )
>       result = pip_install(file_path, tmp_venv)

tests\utils\test_pip.py:21:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

path = <Link file:///C:/Users/Matthew/code/poetry-test/tests/fixtures/distributions/demo-0.1.0-py2.py3-none-any.whl>
environment = VirtualEnv("C:\Users\Matthew\AppData\Local\Temp\poetry_kosx30x2\venv"), editable = False, deps = False
upgrade = False

    def pip_install(
        path: Union[Path, str],
        environment: Env,
        editable: bool = False,
        deps: bool = False,
        upgrade: bool = False,
    ) -> Union[int, str]:
        path = Path(path) if isinstance(path, str) else path
>       is_wheel = path.suffix == ".whl"
E       AttributeError: 'Link' object has no attribute 'suffix'

poetry\utils\pip.py:21: AttributeError

 tests/utils/test_pip.py ⨯✓                                                                               17% █▋
 tests/installation/test_executor.py ✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓                                                     100% ██████████=============================================== short test summary info ===============================================
FAILED tests/utils/test_pip.py::test_pip_install_link - AttributeError: 'Link' object has no attribute 'suffix'

Results (17.72s):
      17 passed
       1 failed
         - tests\utils/test_pip.py:17 test_pip_install_link

Only test_pip.py brings up an additional error

Paste contents of installation/executor.py and utils/pip.py from same commit as above:

Test session starts (platform: win32, Python 3.9.6, pytest 6.2.5, pytest-sugar 0.9.4)
rootdir: C:\Users\Matthew\code\poetry-test
plugins: cov-2.12.1, mock-3.6.1, sugar-0.9.4
collecting ...
 tests/utils/test_pip.py ✓✓✓                                                                              17% █▋
 tests/installation/test_executor.py ✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓                                                     100% ██████████

Results (19.38s):
      18 passed

So @serverwentdown it appears to me naively that the executor tests at least do not fail prior to the functional patch, not sure if this was intended or not

[serverwentdown]

@Ivoz Oh you're right! Thanks for catching this lack of test coverage. I'll fix it after I sleep, but my main suspect is that fixture_dir returns a relative path hence the bug does not get triggered.

[serverwentdown]

Oh I figured it out: In the latest master, _download_archive always returns a Path, never a Link anymore.

The backport to 1.1 (#4549) should still be merged ASAP, but I'm gonna turn this PR into a cleanup PR and to add tests to prevent the same bug from happening again.

No, I'm misreading code. Will attempt to figure it out again tomorrow.

[abn]

Overall looks good. Some minor suggestions.

[serverwentdown]

I have made changes a while back, need another review.

@abn? @sdispater? Anyone who sees this PR can leave a review on this PR too.

(Also, check out the backport to #4549)

[neersighted]

This looks a lot better than the current code -- we're making a lot better use of duck typing while eliminating a rather silly corner case. There's still some isinstance(), but I think you've got it down to the bare minimum

I'm ready to merge this once it passes lint and is rebased onto the current master (the FreeBSD tests were fixed there) -- and please make sure to make the same post-review changes to your backport PR.

Addressed

[neersighted]

Merged! Thank you for your work on this -- and sorry that poetry has been broken so long!

[serverwentdown]

Awesome, thanks for getting this finally merged! 🥳

[mario-bermonti]

Great! Thanks for fixing this.

[github-actions]

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.