dephell icon indicating copy to clipboard operation
dephell copied to clipboard

convert from pyproject.toml is broken for dependencies that are an array

Open eli-schwartz opened this issue 6 years ago • 11 comments

$ dephell deps convert --traceback --from pyproject.toml --to setup.py
WARNING cannot find tool.dephell section in the config (path=pyproject.toml)
ERROR AttributeError: 'Array' object has no attribute 'get' 
Traceback (most recent call last):
  File "/usr/lib/python3.8/site-packages/dephell/cli.py", line 115, in main
    result = task()
  File "/usr/lib/python3.8/site-packages/dephell/commands/deps_convert.py", line 46, in __call__
    resolver = loader.load_resolver(path=self.config['from']['path'])
  File "/usr/lib/python3.8/site-packages/dephell/converters/base.py", line 94, in load_resolver
    root = self.load(path=path)
  File "/usr/lib/python3.8/site-packages/dephell/converters/base.py", line 54, in load
    root = self.loads(content=stream.read())
  File "/usr/lib/python3.8/site-packages/dephell/converters/poetry.py", line 110, in loads
    deps.extend(self._make_deps(
  File "/usr/lib/python3.8/site-packages/dephell/converters/poetry.py", line 334, in _make_deps
    url = content.get('file') or content.get('path')
AttributeError: 'Array' object has no attribute 'get'

The pyproject.toml is the current poetry 1.0.0 one, and contains

keyring = [
    { version = "^18.0", python = "~2.7 || ~3.4" },
    { version = "^19.0", python = "^3.5" }
]

After shoving some print statements into dephell (to print the type and repr of content), I notice the crash occurs here:

[{'version': '^18.0', 'python': '~2.7 || ~3.4'}, {'version': '^19.0', 'python': '^3.5'}]
<class 'tomlkit.items.Array'>
ERROR AttributeError: 'Array' object has no attribute 'get' 

eli-schwartz avatar Dec 15 '19 06:12 eli-schwartz

I think the dephell CI should fetch and process the poetry toml, as it is regularly causing problems. c.f. https://github.com/dephell/dephell/issues/256

I've worked around it with a small patch https://build.opensuse.org/package/view_file/home:jayvdb:py-submit/python-poetry/simplify-toml.patch?expand=1

jayvdb avatar Dec 18 '19 05:12 jayvdb

To be honest, we can't it full support. For now, DepHell has one node for every package on the graph. Nodes are merged if they have the same name. One exception is extras. They are represented as a modification of the name and the dependency itself depends on that extra when it's added to the graph. That's already quite complicated but it's the simplest solution that you can make to support extras without any hacks in the graph and resolving, this is just another type of dependencies.

However, array-based dependencies are really tricky and make things a lot more complicated. For example below, what if pkg1 depends on keyring 17 or 18, and pkg2 depends on keyring 18 or 19? Well, that's easy, the result is one keyring 18 node. But what about markers? What if they are different? We have to adjust nodes lookup, add a lot of tricks, think about merging. Another interesting thing is all changes on nodes should be revertable because after resolving I don't want to see markers and constraints of subdependencies in my requirements file. In that example, that means I still want to see keyring 17 or 18 in converted requirements file but keyring 18 in the lockfile.

Also, there is something unobvious. If you duplicate dependency in requirements.txt they are merged by AND rule, not OR. For example, django>=1.9 and django<1.12 in the same file are merged into django>=1.9,<1.12.

So, resolving in Python already incredibly complicated, and this doubles the complexity. And after all, let's be honest, any project needs only one version of one package. Maybe, both at the same time, but it's a different story. And it's never about ordependency for one project for different platforms. One package does one thing, the same on different platforms. For example, I'd like to install docker package only for unix because win doesn't support it, but it's hard to imagine that docker of some version works for win, and other versions aren't.

Hence such cases exist because developers try to do the job of the resolver, but they shouldn't. The resolver can check the supported platforms itself and install keyring 18.0 for old pythons (that will be out of life 2 weeks later btw) and 19.0 for new pythons.

I understand that we already created that hell and dephell have to help us to handle it. However, taking into account everything above, let's consider some simple solutions that can be applied in the poetry converter itself. The outcome if this decision is output will produce one dependency, not an array. For the example in the issue it will be keyring 18 || 19. What do you think?

orsinium avatar Dec 18 '19 12:12 orsinium

Reading poetry's documentation on how to write a pyproject.toml it's not even clear this is supposed to be valid in the first place. :(

You're entirely correct, the resolver should be able to see that keyring 19 doesn't support python2, and try installing an older version. In fact, pip2 install keyring does exactly that, sooo...

<rant> I don't even understand why it's somehow supposed to be necessary to enforce, in poetry, that you're running the latest version of keyring 19 while on python3. venv management tools will already try to install the latest version that matches requirements, keyring 18 is manifestly supported by poetry on python2 at a minimum and therefore I find it very very hard to believe it will crash on python3, and it isn't the job of a python package to enumerate which old versions work fine but the developer wants you to avoid anyway. Just specify that the project codebase depends on at least version 18, because some target platforms only have that available and therefore poetry is committed to making sure version 18 works.

Lastly, why on earth does poetry still support python 3.4 which was EOL'ed 9 months ago. o_O </rant>

eli-schwartz avatar Dec 18 '19 14:12 eli-schwartz

Lastly, why on earth does poetry still support python 3.4 which was EOL'ed 9 months ago.

Hah, the next dephell release will drop 3.5 support because, as I see in dephell package downloads dephell nobody (!) needs it. Meh, I did a lot of work to support it, even forked bowler and fissix. Anyway, it's time to move forward.

What about the issue, let's do it in the dirty hacks way: try to merge it and do not care about keeping both nodes separately. It helps in most of the use cases.

I think the dephell CI should fetch and process the poetry toml

Yeah, I thought about adding more smoke tests on actual data. Now we are parsing dephell's own metadata.

I've worked around it with a small patch

The page returns 404.

orsinium avatar Dec 18 '19 14:12 orsinium

sorry openSUSE system is a bit odd.

Package was accepted, so now moved to https://build.opensuse.org/package/view_file/devel:languages:python/python-poetry/simplify-toml.patch?expand=1

jayvdb avatar Dec 18 '19 16:12 jayvdb

ping @orsinium ; being able to parse the pyproject.toml of the poetry project is IMO really important as dephell can be a bootstrap for poetry.

openSUSE now has macros to use dephell as a bootstrap for pyproject.toml sdists, but it doesnt work for poetry https://build.opensuse.org/package/view_file/devel:languages:python/python-dephell/macros.py-dephell?expand=1

p.s. poetry 1.1.0 is now in alpha release, so it would be helpful to have this solved before their next release.

jayvdb avatar May 07 '20 09:05 jayvdb

A simpler way to fix poetry' pyproject.toml:

sed -i '/^keyring = /,+3d' pyproject.toml
sed -i 's/\(\[tool.poetry.dependencies\]\)/\1\nkeyring = { version = "^18.0" }/' pyproject.toml

jayvdb avatar May 07 '20 10:05 jayvdb

poetry now also has this in its pyproject.toml:

https://github.com/python-poetry/poetry/blob/1d64e1c75cfd03d8f98533645a7815f0dbcaf421/pyproject.toml#L61-L64

So it's now necessary to patch this too. Full patch:

--- poetry-1.0.9/pyproject.toml
+++ poetry-1.0.9/pyproject.toml
@@ -49,19 +49,13 @@
 virtualenv = { version = "^16.7.9", python = "~2.7" }
 # functools32 is needed for Python 2.7
 functools32 = { version = "^3.2.3", python = "~2.7" }
-keyring = [
-    { version = "^18.0.1", python = "~2.7 || ~3.4" },
-    { version = "^20.0.1", python = "^3.5" }
-]
+keyring = { version = "^18.0" }
 # Use subprocess32 for Python 2.7 and 3.4
 subprocess32 = { version = "^3.5", python = "~2.7 || ~3.4" }
 importlib-metadata = {version = "~1.1.3", python = "<3.8"}
 
 [tool.poetry.dev-dependencies]
-pytest = [
-    {version = "^4.1", python = "<3.5"},
-    {version = "^5.4.3", python = ">=3.5"}
-]
+pytest = {version = "^5.4.3" }
 pytest-cov = "^2.5"
 mkdocs = { version = "^1.0", python = "~2.7.9 || ^3.4" }
 pymdown-extensions = "^6.0"

sebix avatar Jul 05 '20 11:07 sebix

I have poetry 1.1.0a3 building at https://build.opensuse.org/package/show/home:jayvdb:py-new/python-poetry . Needed a few more changes, as a new array was added for pytest .

sed -i '/packaging/d;/poetry-core/d;/clikit/d' pyproject.toml  # the dep version pins from dephell are unusable
sed -i '/^keyring = /,+4d' pyproject.toml
sed -i 's/\(\[tool.poetry.dependencies\]\)/\1\nkeyring = { version = "^18.0" }/' pyproject.toml
sed -i '/^pytest = /,+3d' pyproject.toml
dephell deps convert --from pyproject.toml --to-format setuppy --to-path setup.py

jayvdb avatar Jul 12 '20 14:07 jayvdb

+keyring = { version = "^18.0" }

... which only allows keyring between 18.0 and <19 ... :(

While trying to fix the new isort plugins tests I found out, that poetry refuses to work, because it reads it's own egg.info/requires.txt and sees that the used keyring is way too new.

Edit: Hey but now that I have found dephell, maybe I should use this?

bnavigator avatar Jul 28 '20 20:07 bnavigator

The correct dependency is of course keyring = "*" to permit all versions, unless it's actually known that some versions may be bad.

eli-schwartz avatar Jul 28 '20 20:07 eli-schwartz