-
Notifications
You must be signed in to change notification settings - Fork 124
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
main: build binary distributions via sdists by default #304
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm +0.5 on this, but this would like a way to force the old behavior from the cli (build a wheel from the source directly). We've basically doubled the build time for a wheel, which sometimes can be a big regression.
This patch only affects invoking
Essentially, if no distribution is specified, we will build a wheel from sdist, if any are, we will just build those distributions directly. I forgot to touch up the help messages, I should fix that. |
I'm +0.5 on this as well. Initially it seemed like it could be problematic, but every argument I could come up with against it already has an answer or obvious workaround ( |
I sympathise with not wanting to add more options to the CLI, but I don't like that |
@FFY00 what about |
That is a different change, this PR is only for the default behavior. I know you mentioned this in #257, but can you open a separate issue for that? So that we can track it as an actionable task. |
e794906
to
57d9c81
Compare
The last commit will also allow us to move the colorama stuff and |
The people that would be affected by that are advanced users, which should not really have much of an issue reading the documentation. I am optimizing for the normal user, which would be the one that would fall into that documentation issue. Almost all normal users should actually want to build binary distributions via the source distribution. |
Fixes pypa#257 Signed-off-by: Filipe Laíns <[email protected]>
Signed-off-by: Filipe Laíns <[email protected]>
This is probably obvious but caused me some gray hairs, so let spell it out here: After a bit of digging, this change in particular appears to be the cause of my builds breaking, by imposing new assumptions on folder structure: I have a project in which a Cython pxd includes C++ code through cdef extern from "../my/relative/path/file.cpp" This works fine with |
That is exactly the kind of issue this change is supposed to be uncovering! It means your package cannot be built from the sdist. If there is no compatible wheel on PyPI, pip will try to build the package from the sdist. You have two options:
|
I think that exposes a valid bug though. Surely a sdist created like that would also fail, because that relative path is not exist at the users machine either 🤔 |
I'm curious, if you have compiled code, are you only building one wheel (one OS/arch/Python version), or are you building identical sdists every time? Normally, I'd expect one SDist job and multiple wheel jobs (such as in https://scikit-hep.org/developer/gha_wheels). Build #311 (and cibuildwheel pypa/cibuildwheel#597) do not support running directly from the SDist, though I think it would be a good idea to be able to do that, to encourage this structure even in that case. :) |
FWIW, this was exactly my use case; an sdist-less internal package designed for use in controlled environments for which the source code could not meaningfully be bundled, and for which only wheels were necessary. |
So you want to only build wheel then via |
There's absolutely nothing wrong with building only wheels; there is something deeply and inherently wrong about building SDists that are lies, being known to be invalid and broken. ;) So, yes, just ask for wheel-only and everything will be fine. The default SDist + wheel is the only thing that has changed (to SDist -> wheel). |
Can I change my vote after-the-fact to a strong +1? This is finding bugs even in build backends! :) |
Fixes #257
Signed-off-by: Filipe Laíns [email protected]