Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tomli violates PEP517 Build Requirements #154

Closed
jameshilliard opened this issue Dec 17, 2021 · 19 comments
Closed

tomli violates PEP517 Build Requirements #154

jameshilliard opened this issue Dec 17, 2021 · 19 comments
Labels
question Further information is requested

Comments

@jameshilliard
Copy link

The PEP517 Build Requirements specifically disallow dependency cycles:

  • Project build requirements will define a directed graph of requirements (project A needs B to build, B needs C and D, etc.) This graph MUST NOT contain cycles. If (due to lack of co-ordination between projects, for example) a cycle is present, front ends MAY refuse to build the project.

  • Where build requirements are available as wheels, front ends SHOULD use these where practical, to avoid deeply nested builds. However front ends MAY have modes where they do not consider wheels when locating build requirements, and so projects MUST NOT assume that publishing wheels is sufficient to break a requirement cycle.

@hukkin
Copy link
Owner

hukkin commented Dec 17, 2021

Thanks for the issue!

I'm sure you're well aware of the problem by now. That is, packaging requires reading TOML but there is no TOML parser in the stdlib so build backends either vendor a parser or resort to this dependency cycle. Tomli's build backend (flit_core) has so far chosen to not vendor in the belief that it is the lesser evil. This is a bit of an (unfortunate) exception that we have until stdlib can parse TOML.

The cycle could be solved on Tomli side by vendoring flit_core and using it as build backend but... my intention with Tomli was to write the best pure Python TOML parser, not to end world hunger, solve Python packaging, or anything else. 😃

My understanding is that flit author (takluyver) also thinks that this special casing is best solved on flit's side, so I suggest we have a little patience and see what conclusion they arrive at.

@hukkin hukkin added the question Further information is requested label Dec 17, 2021
@jameshilliard
Copy link
Author

so build backends either vendor a parser or resort to this dependency cycle

Well from my understanding this means tomli at least at the moment is not a PEP517 compliant package, wouldn't one option to be to just switch to the setuptools PEP517 backend until flit figures out what they want to do?

@hukkin
Copy link
Owner

hukkin commented Dec 18, 2021

Hmm, maybe. I'm not sure if that'd be a great twist for the PEP517 Python bootstrap story though, as setuptools has its own dependency cycle with wheel, has dependencies (although bundled) and may depend on Tomli in the near future. Oh and for PEP517 you'd still need a build frontend, likely pypa/build which again, depends on Tomli.

One way to currently avoid the cycle (in case it helps you) is to restrict flit_core to versions >=3.2,<3.4. They are supported by Tomli as build backend and do not depend on Tomli (which arguably also means the title of this issue may not be fully accurate 😉).

@jameshilliard
Copy link
Author

I'm not sure if that'd be a great twist for the PEP517 Python bootstrap story though, as setuptools has its own dependency cycle with wheel, has dependencies (although bundled) and may depend on Tomli in the near future.

From what I can tell setuptools does not actually depend on wheel, as long as setuptools vendors tomli I don't think there would be an issue, it seems to be a significant improvement for bootstrapping compared with flit.

One way to currently avoid the cycle (in case it helps you) is to restrict flit_core to versions >=3.2,<3.4.

Wouldn't that then mean any package depending on flit_core >=3.4 would not be buildable?

They are supported by Tomli as build backend and do not depend on Tomli (which arguably also means the title of this issue may not be fully accurate 😉).

Well tomli currently has requires = ["flit_core>=3.2.0,<4"] so it's at least the case right now that tomli is not PEP517 compliant.

@hukkin
Copy link
Owner

hukkin commented Dec 18, 2021

as long as setuptools vendors tomli I don't think there would be an issue

Here it boils down to:

Tomli's build backend (flit_core) has so far chosen to not vendor in the belief that it is the lesser evil.

I.e. takluyver has made the decision to not vendor in the belief (at least at the time) that it is the best trade-off overall. I honestly trust their judgment in this way more than my own. So if you want things to change, please go pester them not me 😄

Also, with setuptools as backend, you still need that build frontend, which as I mentioned very likely depends on Tomli, so I'm not sure this makes bootstrapping any easier.

@jameshilliard
Copy link
Author

jameshilliard commented Dec 18, 2021

I.e. takluyver has made the decision to not vendor in the belief (at least at the time) that it is the best trade-off overall. I honestly trust their judgment in this way more than my own.

So it's unclear if flit_core itself violates PEP517 build requirements since tomli is not actually a build dependency for flit_core but rather a runtime dependency, however tomli definitely violates PEP517 by depending on flit_core since tomli is a runtime dependency of flit_core and thus transitively a build dependency of tomli. I wrote a validator for build to help detect these invalid pep517 requirements.

Also, with setuptools as backend, you still need that build frontend, which as I mentioned very likely depends on Tomli, so I'm not sure this makes bootstrapping any easier.

Bootstrapping works much better with setuptools since it has a non-generator setup.py shim for bootstrapping, see #155.

@hukkin
Copy link
Owner

hukkin commented Dec 18, 2021

Yeah I didn't mean to imply anything about whether flit_core violates PEP517 or not. My point is that due to conditions I laid out in my first post this cycle may be a necessary exception. An alternative being that many packages vendor a TOML parser, but apparently that was deemed to be the worse alternative by packaging experts.

Also, with setuptools as backend, you still need that build frontend, which as I mentioned very likely depends on Tomli, so I'm not sure this makes bootstrapping any easier.

Bootstrapping works much better with setuptools since it has a non-generator setup.py shim for bootstrapping, see #155.

But that is not PEP517 bootstrapping. Then you resort to the legacy build system. If you are happy to bootstrap like that, why not just run the following to build Tomli

python3 -c 'from setuptools import setup; setup(name="tomli", packages=["tomli"], package_data={"": ["*"]}, version="2.0.0")' sdist bdist_wheel

It should work perfectly already with no changes made to the repository.

@jameshilliard
Copy link
Author

jameshilliard commented Dec 18, 2021

Yeah I didn't mean to imply anything about whether flit_core violates PEP517 or not. My point is that due to conditions I laid out in my first post this cycle may be a necessary exception. An alternative being that many packages vendor a TOML parser, but apparently that was deemed to be the worse alternative by packaging experts.

I really don't see why there should be an exception here when simply using setuptools instead of flit_core seems to fix the issue, the PEP is also pretty clear that this is a hard requirement(ie MUST NOT) as it says This graph MUST NOT contain cycles. which is different from say a recommended behavior(ie SHOULD) like Front ends SHOULD check explicitly for requirement cycles.

But that is not PEP517 bootstrapping.

It also has PEP517 bootstrapping support which uses this and doesn't have a circular dependency issue:

requires = ["setuptools", "wheel"]
build-backend = "setuptools.build_meta"

This is basically the same way packages like build do their installs.

Then you resort to the legacy build system. If you are happy to bootstrap like that, why not just run the following to build Tomli

By using the empty setuptools shim you basically get guaranteed identical behavior with PEP517 bootstrapping above as both effectively are configured with setup.cfg, so this prevents any behavior divergence better between the two.

@hukkin
Copy link
Owner

hukkin commented Dec 18, 2021

It also has PEP517 bootstrapping support which uses this and doesn't have a circular dependency issue:

Yeh, but then you need the PEP517 frontend that depends on Tomli: we have the same cycle again. There really is no way around the fact that PEP517 needs TOML parsing and that we dont have until Tomli is built. I may not have the energy to write in this issue after this post, but as Ive said before, Id like to wait for what takluyver does with flit. The command I provided can be used to do legacy build of Tomli and is trivial to modify to fetch version number dynamically from pyproject.toml, .bumpversion.cfg or init.py file.

@jameshilliard
Copy link
Author

Yeh, but then you need the PEP517 frontend that depends on Tomli: we have the same cycle again.

There are PEP517 frontends without a circular tomli dependency issue(due to vendoring)...like pip.

There really is no way around the fact that PEP517 needs TOML parsing and that we dont have until Tomli is built.

Not all PEP517 frontends have a direct dependency on the tomli pypi package so this is useful for those frontends.

I may not have the energy to write in this issue after this post, but as Ive said before, Id like to wait for what takluyver does with flit.

That would mostly fix the boostrapping issue with tomli(at least for flit_core), however there are other advantages to using setuptools such as making it easier for other build tools to install tomli without running into the circular dependency issue, also setuptools is effectively bundled with a python install while flit_core is not which makes it more suitable for bootstrapping.

The command I provided can be used to do legacy build of Tomli and is trivial to modify to fetch version number dynamically from pyproject.toml, .bumpversion.cfg or init.py file.

Using flit_core still leaves PEP517 bootstrapping broken though due to the dependency cycle, this fixes both issues.

@hukkin
Copy link
Owner

hukkin commented Dec 18, 2021

There are PEP517 frontends without a circular tomli dependency issue(due to vendoring)...like pip

Yeah as Ive stated, I do understand that vendoring TOML parsers is an alternative and how this is a trade-off and how an active decision to not vendor has been made.

Using flit_core still leaves PEP517 bootstrapping broken though due to the dependency cycle, this fixes both issues.

The command has nothing to do with flit_core, it interacts with it in no way at all. It should fix both your issues.

@jameshilliard
Copy link
Author

Yeah as Ive stated, I do understand that vendoring TOML parsers is an alternative and how this is a trade-off and how an active decision to not vendor has been made.

So based on my understanding of the situation if flit_core does not decide to vendor tomli then tomli would need to move off of flit_core to be PEP517 compliant. I guess @takluyver should maybe comment on how this should be resolved.

The command has nothing to do with flit_core, it interacts with it in no way at all.

I realize that.

It should fix both your issues.

It still however leaves tomli with a broken PEP517 dependency cycle at the moment.

@astrojuanlu
Copy link

@layday
Copy link
Contributor

layday commented Dec 18, 2021

It also has PEP517 bootstrapping support which uses this and doesn't have a circular dependency issue:

To be clear, setuptools absolutely does have a circular dependency on wheel when bootstrapping with PEP 517. setuptools needs wheel to build a wheel of itself and wheel needs setuptools to build itself.

@jameshilliard
Copy link
Author

To be clear, setuptools absolutely does have a circular dependency on wheel when bootstrapping with PEP 517. setuptools needs wheel to build a wheel of itself and wheel needs setuptools to build itself.

Are you sure? I don't see wheel listed here.

@jameshilliard
Copy link
Author

Oh, it's wheel specific and comes from here I guess.

@layday
Copy link
Contributor

layday commented Dec 19, 2021

If the circular dependency in setuptools would be resolved then yes, it would perhaps be better to use a backend which allows declaring project metadata in a format other than TOML. But then you'd need twice the number of backends to bootstrap Python's packaging stack, because Flit has been or will be adopted by other foundational dependencies. We should look at this whole thing... holistically instead of trying to fix issues as they crop up in each one of these dependencies. If we want to bootstrap our foundational dependencies with Flit then we all need to switch to Flit and we need to come up with a solution for building and installing wheels that works for all of them.

@takluyver
Copy link
Contributor

We'll go with the vendoring option in flit_core.

Please don't pester @hukkin about this. I'm the one who introduced the dependency cycle, and I'm the one working on a packaging tool. We're already having a discussion about it at pypa/flit#483 (and other Flit issues and PRs), which @hukkin is already involved in. Opening more issues feels more like shouting at us to pay attention to your problem than like it actually moves a solution forwards. I'm sorry I haven't responded quicker, but Flit isn't my job, and it's not always what I want to do in the evening after work.

@jameshilliard
Copy link
Author

setuptools needs wheel to build a wheel of itself and wheel needs setuptools to build itself.

I've updated my dependency cycle checker so that it is able to catch the wheel cycle violation as well now.

Please don't pester @hukkin about this. I'm the one who introduced the dependency cycle, and I'm the one working on a packaging tool. We're already having a discussion about it at pypa/flit#483 (and other Flit issues and PRs), which @hukkin is already involved in. Opening more issues feels more like shouting at us to pay attention to your problem than like it actually moves a solution forwards.

Great, sorry about the noise, was throwing around different potential solutions to see what would be acceptable as there's a lot of potential ways to fix this issue that I've come across, closing this as a viable solution seems to have been decided on at least for resolving the flit_core+tomli dependency cycle.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants