Use Python Version and OS to Select the Appropriate Wheel (#944) {Against "master" Branch}

[KyleKing]

Resolves #944 & #807

Issue: If a package only has platform_specific_wheels, Poetry just downloads the first one, which is usually cp27 and MacOSX (i.e.: numpy)

Changes: This PR adds logic to determine the best wheel file to download for Poetry supported platforms. If the best wheel can't be identified, the method falls back on the original behavior to return the first platform wheel. This fixes the [OS Error] on Windows and improves matching for Linux and Mac.

Also, tested on a couple of different computers as a sanity check (Win7-64, Win10-64, and MacOSX Mojave)

Pull Request Check List

• Added tests for changed code.
• Updated documentation for changed code. {N/A - no changes necessary}

[Pluckerpluck]

I'll try to test this out tomorrow (UK). This is a much needed fix as, with numpy being broken, Poetry is unusable in my field.

This should also prioritize platform specific wheels over sdist if I'm looking at this correctly? I remember having a problem where pip could install a package just fine but poetry (at least when run via tox) had to build it, which failed as I didn't have a C compiler. So this could potentially fix that as well.

Edit: I totally forgot... I'll try to test it out this weekend instead

[Pluckerpluck]

Appears to work perfectly for me. I can now install Numpy and haven't yet noticed any side effects.

[Pluckerpluck]

New problem:

Python 32bit can be run on a 64bit PC. In this situation this downloads the 64bit version, which again fails to run.

At least, that's what I assume the problem is. I ran this with 32bit python on a separate PC and it failed (and downloaded the amd64 version of PyWin32). Tomorrow I can run it against a 64 bit version of python to confirm this is the problem, but I don't have time today.

---

This doesn't change the fact that this pull request is still valid and is much more effective than the "randomly pick the first one" option that previously existed, but it is still a problem.

[KyleKing]

@Pluckerpluck, I think that case is fixable and I’ll look tonight. I remember planning to use the bit-version of Python instead of the OS, but I’ll have to check

Update: will probably use sys.maxsize > 2**32 based on https://stackoverflow.com/a/1405971/3219667

[KyleKing]

@Pluckerpluck do you have an example 64-bit package that failed to install on 32-bit Python? I can make it one of the test cases

[KyleKing]

@sdispater this branch should be ready for review. Let me know if you have any questions or would like to see any changes

---

@Pluckerpluck, the latest changes should address the case where Python is 32-bit and OS is 64. I tested on a Win7-64 with Python 32-bit:

(py372) C:\Users\king.kyle\Desktop\TEST>poetry add pywin32 -vvv
Using virtualenv: C:\Users\king.kyle\AppData\Local\pypoetry\Cache\virtualenvs\TEST-py3.7
PyPI: No release information found for pywin32-210, skipping
PyPI: No release information found for pywin32-214, skipping
PyPI: 3 packages found for pywin32 *
Using version ^224.0 for pywin32

Updating dependencies
Resolving dependencies...
   1: fact: test is 0.1.0
   1: derived: test
   1: fact: test depends on pywin32 (^224.0)
   1: selecting test (0.1.0)
   1: derived: pywin32 (^224.0)
PyPI: No release information found for pywin32-210, skipping
PyPI: No release information found for pywin32-214, skipping
PyPI: 1 packages found for pywin32 >=224.0,<225.0
PyPI: Getting info for pywin32 (224) from PyPI
PyPI: No dependencies found, downloading archives
PyPI: Attempting to determine best match for: {'plat': 'windows', 'not32bit': False, 'pyver': ('3', '7', '2')}
PyPI: Found best wheel match: https://files.pythonhosted.org/packages/8a/37/917c4020e93e0e854d4cbff1cbdf14ec45a6d1cedf52f8cafdea5b22451a/pywin32-224-cp37-cp37m-win32.whl
PyPI: Downloading wheel: pywin32-224-cp37-cp37m-win32.whl
   1: selecting pywin32 (224)
   1: Version solving took 4.029 seconds.
   1: Tried 1 solutions.


Package operations: 1 install, 0 updates, 0 removals

Writing lock file

  - Installing pywin32 (224)

(py372) C:\Users\king.kyle\Desktop\TEST>python
Python 3.7.2 (default, Jan  2 2019, 17:27:13) [MSC v.1912 32 bit (Intel)] :: Anaconda, Inc. on win32
Type "help", "copyright", "credits" or "license" for more information.
>>> import sys
>>> import platform
>>> platform.machine()
'AMD64'
>>> sys.maxsize > 2**32
False
>>>

[Pluckerpluck]

Turns out my error was unrelated. While I can now confirm that 32 bit python downloads 32 bit versions, is it actually a requirement to work? I honestly have no idea and had just assumed it from the identical error.

---

I might actually be suffering from a bug within shutil.rmtree on windows which can fail sometimes. There are multiple instances of this occurring, with issues in both pip and ansible.

I'll report this as its own issue once I've investigated further (if it's definitely a bug). I think it masks the real problems though.

[KyleKing]

@sdispater is there anything I can do to help merge this feature?

Would it be better to submit this change to the development branch? Are there any additional tests or other changes you would like to see before merging?

This PR fixes a showstopper issue for us and has been in use for the better part of two months!

[drunkwcodes]

The comparison with PackageFinder in setuptools?

[KyleKing]

@drunkwcodes, I think you're asking what makes this PR different

I checked the source for setuptools.PackageFinder: https://github.com/pypa/setuptools/setuptools/init.py#L47

The PackageFinder class appears to identify all Python packages within a local filesystem directory

This PR parses the list of wheel filenames for the optimal OS and Python-specific one

[drunkwcodes]

@KyleKing I see.

PEP 425 compatibility?
And I think having a room for fuzzy search as a fallback method is good.

[KyleKing]

The logic steps use the PEP425-specified tags to find the best match. I commented a few of the relevant lines

(Sorry for the extra emails, I meant to put those comments in one review)

[kung-foo]

I had a similar issue with a llvmlite dependency. It would fetch a 2.7 macosx wheel and then add a requires for enum34 which breaks when running on python > 3.4. This PR fixed it!

[kbakk]

Looks good, just a minor input on the PR regarding using property.

[KyleKing]

Pushed new PR to the develop (poetry 1.x.x) branch #1144. I wanted the 1.X.X features locally and maybe that PR will have a better chance of being merged

[sdispater]

I am on the fence regarding this.

While that prevents installation errors it defeats the "platform-agnostic lock file" purpose. I wonder if it would be preferable to build markers based on the wheel files if no sdist is present, that way we are making sure that the package won't be installed on systems that are not compatible.

@KyleKing What do you think?

[KyleKing]

I've researched/rewritten this response a number of times, so hopefully this final iteration makes sense! Let me know what you think

This PR shouldn't impact the lock file. The changes fix the resolver for selecting the wheel file in the case that an sdist or generic distribution isn't available.

I ran a test initializing a project on a Mac, then cloning and installing the project on Windows (used the code from this PR in both cases). With the downloaded Mac poetry.lock file, Poetry picked the correct Windows wheel file for numpy and the lock file was unchanged. I also tried deleting the lock file and confirming that Poetry created the exact same file. This makes sense as the poetry.lock file stores every wheel file hash and doesn't explicitly indicate the wheel file used.

See my demo at: KyleKing/Poetry-EdgeCaseTesting/PR955-20190623 / commits

---

Separately, I think it would be great to have environment markers in Poetry. I would be happy to submit or help review a PR

The markers would fix the case where a developer adds a Windows-only dependency (i.e. pywin32) to the pyproject.toml file. Then, when attempting to run poetry install on a Mac/Linux, Poetry crashes installing pywin32 (which would have been safely skipped if a marker was available)

Environment markers looks to be specified in PEP-508 and this is an example from pipenv: "markers": "sys_platform == 'linux'")

Edit (24Jun2019): Looks like Poetry already supports environment markers! #1142 / release 1.0.0a4 🎊

poetry add pywin32 --platform Windows

[tool.poetry.dependencies]
pywin32 = {version = "^224.0",platform = "Windows"}

---

(More for my reference) Here are some external links:

• Discussion on pipenv for OS-specific locking
  • Follow up - looks like it was removed
• PipEnv Discussion on hash verification breaking when new wheel files uploaded
• PipEnv issue with markers for ref
• StackOverflow question on yarn.lock
• NPM package-lock documentation
  • "All optional dependencies should be included even if they’re uninstallable on the current platform.""
• NPM ignores the lock file and just uses the package.json - not sure if this is still the case

[KyleKing]

Oops, I misunderstood your question and didn't realize Poetry supports environment markers (I've edited my last comment)

I think the desired behavior is that poetry will crash when attempting to install a dependency not available for the specific system. This error is clear and indicates that the project hasn't been tested/designed for the platform the user is attempting to install on and will require code changes. If Poetry were to auto-assign markers (python version/environment), the error would be buried at install and instead would later appear as a no module named ___ ImportError.

Sometimes authors will add additional wheel files after a release, although rare, could cause Poetry to add markers that are no longer relevant a few days later. Poetry would also likely need to be able to disable auto-marker assignment by dependency and the setting would need to be preserved when the project is cloned.

I think the existing support for user-specified markers is the right solution. I'm always open to editing this PR, so let me know what you think

[KyleKing]

@sdispater, do you think you're still on the fence?

• This change implements PEP425 support for packages that do not produce sdist files nor generic wheels
• This PR does not change the lockfile
• I think it should be up to the user's to specify proper environment markers. Having Poetry auto-assign markers would only be burying errors (PEP20 - "Errors should never pass silently")

[sdispater]

@KyleKing The thing is if a package only has Windows wheels and no sdist, we cannot declare it as compatible with any system, we have to declare it as being a Windows only package by setting the right markers. If not, we will have an error later on when pip tries to install the package on MacOs, for instance, and can't find an appropriate file.

[KyleKing]

Yeah, but I think that’s the right behavior to know that your platform isn’t supported when you run ‘poetry install’ rather than later when getting a possibly unintuitive import error

If a package author added a Windows-only package, then they are likely using it without a fallback. If they intend to support multiple platforms, they need to both make code changes to handle that importerror and indicate that the file is Windows only with a marker. Some examples are mypy and pytype that are Linux only. I tried installing them with Poetry and saw right away they didn’t support Windows when the install failed.

If you think the auto-marker-adding feature is the right forward, I’ll start a PR; however, I don’t think that new feature should hold up this PR (I have some follow up questions on the edge cases). This PR is a critical fix for non-Mac/Linux users, so it would be really beneficial to merge. There are identical changes submitted against the development branch, if you think that would be better than merging to master while the auto-marker feature is under development

(Typed on mobile, so hopefully reads well)

[sdispater]

@KyleKing It all comes down to the fact that the lock file is system agnostic and, as such, can't have packages locked that are not compatible with any system. That defeats the purpose of the lock file.

What this means is we cannot rely on the current system in any way to generate the lock file. The only source of truth to generate a lock file is the pyproject.toml file.

[Pluckerpluck]

It all comes down to the fact that the lock file is system agnostic and, as such, can't have packages locked that are not compatible with any system.

I disagree with this statement. Specifically, I argue that it is still system agnostic even if components within it are not. The lockfile can implicitly state that a build is actually impossible on a given platform when libraries do not exist. In fact, I would go so far as to say that it's a requirement.

Using a marker implies that the system will just not use that package if you're on the wrong platform. But that will result in the program crashing, as that was a required package. So all you've done is move the error elsewhere.

I believe that in the case of wheels not existing (and no sdist) poetry should error cleanly with a descriptive error message. That would make the most sense to me.

[sdispater]

@Pluckerpluck

The lockfile can implicitly state that a build is actually impossible on a given platform when libraries do not exist

Then, by definition, it is no longer system agnostic.

Using a marker implies that the system will just not use that package if you're on the wrong platform. But that will result in the program crashing, as that was a required package. So all you've done is move the error elsewhere.

But if you lock a package that was not meant to be locked for a specific system, it will crash at installation time because the dependency will not be found due to the fact that no suitable sdist or wheels exist.

[Pluckerpluck]

Then, by definition, it is no longer system agnostic.

The structure and contents are still system agnostic. In the same way that Java is platform agnostic yet you can use the win32 library for direct access. It will crash when you try to run it on Mac, but that's OK.

Basically, it only has to be system agnostic when the program it's building can be system agnostic.

But if you lock a package that was not meant to be locked for a specific system, it will crash at installation time because the dependency will not be found due to the fact that no suitable sdist or wheels exist.

Which, in my opinion, is the correct time to crash. At this point the entire application is platform specific. Well, it should "exit" cleanly with a sensible error.

Alternatively you have to embed somewhere that the entire application in Windows only.

[KyleKing]

@sdispater what would like me to do to continue?

[KyleKing]

This PR fixes the bug reported in #944

Since the scope for this PR has been expanded to be a new feature and not just a bug fix, I'll start implementing the auto-marker assignment against the Develop branch in #1144

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