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

vendoring is broken, due to a cyclic dependency during license fetching #10570

Closed
pradyunsg opened this issue Oct 11, 2021 · 10 comments · Fixed by #10583
Closed

vendoring is broken, due to a cyclic dependency during license fetching #10570

pradyunsg opened this issue Oct 11, 2021 · 10 comments · Fixed by #10583
Assignees
Labels
project: vendored dependency Related to a vendored dependency type: maintenance Related to Development and Maintenance Processes

Comments

@pradyunsg
Copy link
Member

Well, the lack of maintainance of the license fetching logic in vendoring has come to bite us. :)

flit recently established a cyclic dependency, by depending on tomli: see pypa/flit#451 and https://flit.readthedocs.io/en/latest/bootstrap.html.

We get licenses from sdists in vendoring (which means building metadata / wheels -- leading to pradyunsg/vendoring#1). Since flit is no longer bootstrappable through regular mechanisms, it'd be best to switch to using wheels for the license fetch phase.

This is the tracking issue for actually fixing this, and adopting the fix in our workflow.

@takluyver
Copy link
Member

Sorry about that breakage, and if you need me to do anything temporarily in Flit to keep things ticking over, let me know.

@pradyunsg
Copy link
Member Author

Nothing that comes to mind right now.

This is the sort of thing where doing a major version bump is controversial since SemVer doesn't really cover that -- it's a distribution-related change, not a usage change. 🙃

FWIW, why was this change made? I can't seem to find any discussion about it, and IIRC, being able to bootstrap backends was explicitly why backend-path was added in.

@takluyver
Copy link
Member

The change was switching the TOML parser in Flit to tomli - because that's the one everyone else is now using, and because it supports the TOML 1.0 standard (which the toml package didn't). flit_core can still bootstrap itself (it can build a wheel of itself without reading any TOML). But it needs the toml parser to package anything else - including tomli itself. It works if you can use tomli (which is pure Python) installed from a pre-built wheel, so I didn't think it would be too disruptive. 🤷

Previously we avoided this simply by making bootstrapping someone else's problem - toml was built with setuptools. But if you want to use PEP 517 all the way, that's actually no better: there's a cycle between setuptools & wheel. And I obviously like the idea of being able to bootstrap my packaging tool without setuptools. 🙂

IMO, the long term fix is putting a toml parser in the stdlib. But even if that's agreed now, it's going to be quite some time before we can actually rely on that.

@hukkin
Copy link
Contributor

hukkin commented Oct 12, 2021

Using a vendored Tomli in flit_core would be a way to break the cycle temporarily, until a TOML parser is available in the stdlib. But I don't how much @takluyver and some repackagers would hate doing that? I assume a lot. 😄

@takluyver
Copy link
Member

I don't particularly mind doing that - but I'd guess that there's a large overlap between the people who can't use a pre-built wheel and the people who reject bundled packages, so I don't know if bundling would help many people.

@pfmoore
Copy link
Member

pfmoore commented Oct 12, 2021

I'd be interested in understanding whether backend-path is actually useful here, and if not, why not (if "we still have to vendor stuff and that sucks" is the answer, then that's valid). Because as far as I recall, it was added explicitly to handle this type of situation, so if it's not doing that job, we maybe need to review it.

@takluyver
Copy link
Member

backend-path is working correctly for flit_core, as far as I'm concerned. It's set here, and flit_core can be built from source with no problem:

pip wheel --no-binary :all: --no-deps flit_core

The issue arises because flit_core has a runtime dependency (not a build dependency) on tomli. So tomli's build dependency on flit_core means it has a transitive build dependency on itself. Building flit_core is fine, using it for tomli is what falls down.

Another possible workaround would be to have a special build backend in tomli sdists - e.g. bundling flit_core in the sdists - and using backend-path in tomli's pyproject.toml to use that. That would work, as far as I know, but I don't think it should be on tomli to do special workarounds for bootstrapping - if we need special cases, they should be in flit_core.

Yet another possibility is that pip could detect such a cycle - we're trying to build tomli, and we've found that we need tomli - and use a wheel in an isolated build env to break it, unless you pass some extra option --no-wheels-not-even-to-break-build-cycles. But that's probably a lot of work for a hopefully very specific situation.

@pradyunsg
Copy link
Member Author

Ah, right. backend-path solves the build time dependency problem, not the runtime one. :)

@pradyunsg
Copy link
Member Author

Filed #10583, which should resolve this.

@pradyunsg
Copy link
Member Author

Alrighty, I've been overly eager and merged this; just to make sure that this isn't the blocker for the the bugfix release. :)

@pradyunsg pradyunsg self-assigned this Oct 20, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 19, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
project: vendored dependency Related to a vendored dependency type: maintenance Related to Development and Maintenance Processes
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants