-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Implement PEP 660 hooks in setuptools/build_meta.py #2872
Conversation
I chose the root .pth editable strategy as it "just works" for implicit namespace packages and also looked easier haha. While most of the code is (hopefully) rather straightforward, it is quite verbose as unforunately setuptools doesn't have much in terms of wheel utilties. There's also a somewhat sketchy hack used to acquire the distribution location so I can put the right path to ${dist-name}.pth in the wheel. Since the PEP 517 is isolated from the command framework, it's pretty hard querying information from the various setup.py commands ran. The shim added captures the egg_info instance when setup.py dist_info runs so the build_editable hook can query the location (i.e. egg_base). One limitation of this commit is that the build_editable hook completely ignores the metadata_directory parameter and violates the guarantee the hook shall produce a (editable) wheel with identical metadata. Given that it seems no major backends is currently following through with this promise[^1] this is hopefully fine for now? dholth's work which I referenced extensively: https://github.com/dholth/setuptools_pep660 [^1]: https://discuss.python.org/t/nobody-is-following-the-metadata-directory-promise-in-pep-517/6964
https://github.com/pypa/setuptools/runs/4180806139?check_suite_focus=true#step:5:204
Well apparently dist.version is not a valid version? >>> import setuptools.dist
>>> setuptools.dist.Distribution._normalize_version("58.5.3.post20211111.post-20211111")
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
File "/home/ichard26/programming/oss/setuptools/setuptools/dist.py", line 484, in _normalize_version
normalized = str(packaging.version.Version(version))
File "/home/ichard26/programming/oss/setuptools/setuptools/_vendor/packaging/version.py", line 277, in __init__
raise InvalidVersion("Invalid version: '{0}'".format(version))
setuptools.extern.packaging.version.InvalidVersion: Invalid version: '58.5.3.post20211111.post-20211111' |
Alright, implementing this patch: diff --git a/setuptools/build_meta.py b/setuptools/build_meta.py
index a24eb0f7..4e377916 100644
--- a/setuptools/build_meta.py
+++ b/setuptools/build_meta.py
@@ -49,7 +49,7 @@ import setuptools.command.egg_info
import distutils
import pkg_resources
-from pkg_resources import parse_requirements
+from pkg_resources import parse_requirements, safe_name, safe_version
__all__ = ['get_requires_for_build_sdist',
'get_requires_for_build_wheel',
@@ -313,7 +313,11 @@ class _BuildMetaBackend(object):
dist = pkg_resources.DistInfoDistribution.from_location(
location, dist_info, metadata=metadata
)
- wheel_name = f'{dist.project_name}-{dist.version}-ed.py3-none-any.whl'
+ # Seems like the distribution name and version attributes aren't always
+ # 100% normalized ...
+ dist_name = safe_name(dist.project_name).replace("-", "_")
+ version = safe_version(dist.version).replace("-", "_")
+ wheel_name = f'{dist_name}-{version}-ed.py3-none-any.whl'
wheel_path = os.path.join(wheel_directory, wheel_name)
with zipfile.ZipFile(wheel_path, 'w') as archive:
archive.writestr(f'{dist.project_name}.pth', location) Avoids the weird error message from pip (due to misreading the not normalized distribution name), but then predictably uncovers this issue: Building editable for setuptools (pyproject.toml) ... done
Created wheel for setuptools: filename=setuptools-58.5.3.post20211111.post_20211111-ed.py3-none-any.whl size=9822 sha256=050f33119d1f68404c8eed8007f9870d5cc49e40fcd4dd3edb5c4d7e1adfc50f
Stored in directory: /tmp/pip-ephem-wheel-cache-tb3eq2fm/wheels/29/a6/99/6cd5ddea9b773be0e848527e54dd03b1092fe671cb9b561ab5
WARNING: Built editable for setuptools is invalid: Metadata 1.2 mandates PEP 440 version, but '58.5.3.post20211111.post-20211111' is not
Failed to build setuptools
ERROR: Could not build wheels for setuptools, which is required to install pyproject.toml-based projects Is this is a short-coming in my implementation or can I safety assume non-PEP-440 versions are not my problem 🙂 .. although in that case I need to figure out to have dist_info return a PEP 440 version for setuptools itself ... |
As it seems to be doing better than what setuptools.dist.Distribution provides
TIL that egg_info and dist_info can return different versions.
bdist_wheel calls egg_info and uses that as its metadata source (which is how it avoids this error). I guess I might have to use egg_info instead then, but AFAICT PEP 660 only specifies .dist-info? I also kinda don't want to rewrite the code to use .egg-info instead :p |
Considering that PEP 660 requires backends to choose how to implement editable installs and the fact that I don't see any rush to implement this, so it'd be better to do it right than to do it quickly. |
That's definitely unintentional. |
One thing I'd like to suggest is that |
Yeah yeah I know this implementation needs a bunch of work, my apologies for misguiding y'all expectations. I'm quite busy right now but I can look into revisiting this implementation soon. |
Can you provide some direction on how this would be accomplished? |
I feel that the initial pass of PEP 660 support should replicate the current behaviour, as that's what users expect and are used to. And I agree with @pradyunsg that the Further iterations could then investigate other implementation mechanisms that make different trade-offs than the current |
I think it's pretty obvious how this could be accomplished. I seem to remember there was a proof of concept implementation kicking around when PEP 662 was being discussed. I seem to remember thinking it was easier than I thought it was going to be, but I can't find a copy of it. Maybe @gaborbernat was the one who did it?
I disagree on both counts here. The legacy behavior is fundamentally broken, and since PEP 660 forces us to make choices for our users rather than letting them make their own trade-offs, we should choose correctness. I think people expect It doesn't seem likely that we need to worry about backwards compatibility here because this will primarily affect interactive workflows rather than automated ones — some people may be relying on the accidental feature that more stuff is included in editable installs than in normal installs for their production systems, but this is simply a bug in their |
You can use the |
Yeah, I definitely agree that PEP 660 provides a clunky backend-specific non-standard way to allow users to choose their editable install mode. I don't think we can really rely on that or expect people to discover or use it, particularly when, as I mentioned, the problems with the current editable install mode are subtle and quiet, whereas the users will discover the problems with their assumptions quickly and loudly with the strict mode. I seem to recall during the PEP 660 discussions that everyone in favor of PEP 660 seemed fine with the idea that backends should choose what their install mode would be, and since "strict" is really the only mode that makes sense for Depending on the implementation, it might make sense for there to be a dedicated command class that can be used to customize the behavior of editable installs. |
If that's the route you want to take, I'll flag that it could be disruptive to users in situations where they can't easily control the setuptools version (like pip's isolated build environments). To that end, I encourage doing things to reduce user pain due to it: documenting the failure modes with some amount of clarity in the documentation, clear communication about the change with known regressions clearly stated, and so on. If you can do some sort of opt-in/opt-out via build_config or an environment variable, that would help users mitigate their immediate breakage as well, reducing the amount of toxicity you'd need to deal with when the release goes out. |
To some extent I agree that PEP 660 deliberately chooses to move the choices about trade-offs into the hands of backend implementers, something I was very concerned with, and one of the main reasons I thought PEP 660 was a bad design. On the other hand, the PEP 660 proponents had at least some point when they said that this is much less of a problem for editable installs than for normal installs, because this sort of thing would only ever be a problem if you are installing from source code, which you can easily change. I can't think of any examples where someone would be doing an editable install using
I agree that we should document the failure modes. I think opt-in wouldn't achieve our goals because, as I said, the current behavior is broken in such a way that most users do not know it is broken, and so they will never opt-in. Opt-out might make sense, but I don't think it's critical, and I don't think we need to bend over backwards to accommodate the fact that PEP 660 is poorly designed. |
I believe you're misunderstanding -- the moment you release PEP 660 support in setuptools, pip's isolated build environments will start picking up the latest setuptools version and use that for performing editable installs for users. If you change behaviours there vs
No, the opt-in -> switch default with opt-out -> drop opt-out is not to deal with "PEP 660 is bad actually". It's to deal with the fact that you want to make a backwards incompatible change as part of implementing the setuptools' PEP 660 support. Even if setuptools implemented PEP 660 via a .pth file first and then moved to adopt the strict mode, the users would still only make noise when the adoption of the strict mode happens -- because that is the problematic bit (i.e. the breaking change). Even in that situation, I would suggest adopting the gradual transitional approach to give users time to provide early feedback/adapt after the change. |
I understand what you mean but I'm saying that I'm struggling to imagine a situation where someone would be using an editable install in an isolated build environment. Editable installs are for interactive workflows where you are making changes on the fly. AFAICT, the only reason someone would want to use an editable install during their build is if they messed up their package configuration such that normal installs install something totally different from editable installs, in which case their package is broken and it should raise an error! It's a bug, not a feature, that using an editable install can gloss over problems with your package configuration.
The reason this has to do with PEP 660 being bad is that PEP 660 opted to standardize a method where there is no standard way for end-users to configure their installation mode. If there were, Now that we know that there's no option for us to leave it up to end-users, we have to make a choice as to whether or not we fix the bug, and I think we should fix it. People already hate setuptools and packaging in general (with good reason!), so we may as well lean into the pain and at least give them as good a product as we can. |
I don't know which PoC you are remembering, but I'd created frontend-editables at the time which relied on symlinking to enforce 'strictness'. You can't have symlinks in a wheel, so how would you do this within the framework of PEP 660? |
pip creates an isolated build environment to call PEP 660 hooks inside of. It will install the latest available version of setuptools in the isolated build environment.
Surely passing |
I don't think PEP 660 or PEP 662 differed in how things could be implemented, because you can use a PEP 662 approach and then implement the code the front-end was expecting to implement on the backend. The problem I had always had in providing Once you have the list of files to install, you can use one of the strategies outlined in the PEP for installation. Probably the most cross-platform of which is a proxy platform using import hooks in a |
I'm confused by this statement. PEP 660 has the standard Footnotes
|
The proxy module trick doesn't work with namespace packages. It might not be an option for setuptools. Creating a tree of symlinks out of band and adding them to the Python path would work, but we'll have gone from drop this one file in site packages to multi-step installation and uninstallation which is unspecified and outside the control of pip. In my opinion, PEP 660 isn't equipped to handle anything this intricate; trying to force an (oversized ;) wheel into a square hole is only gonna end in frustration. |
Surely any users who are installing a package can edit their own
I haven't had much time to look into it, to be honest. How do other backends deal with this? If it's true that PEP 660 backends can't even implement this correctly, then it's a much worse spec than I thought.
There is a standard way to pass options to backends, but the options themselves are specific to the backend, and I'm not even sure that all front-ends have a way to pass options via that mechanism. The point is that we have to go outside of the standard to allow end-users to configure this if it's possible at all. I'm not saying we can't do it, just that it's a messy hack and we shouldn't expect users to discover it. The default should be strict installations. |
I don't think upper-capping setuptools in a project's build metadata solely for editable installs in development is something we'd want to encourage or even suggest.
I don't know who thought that was an acceptable UI but even if they did, editing editable-specific metadata is fundamentally different from editing project-wide metadata to satisfy a specific purpose.
They use |
It's not clear to me that any strategy is perfect here. Symlinks don't work on all platforms,
Define correct. This is not a rhetorical question, neither PEP 660 nor PEP 662 precisely defined an editable install, both using wording something along the lines of
That's from PEP 662, PEP 660 was similar, more restrictive in some ways, less in others. Again, if we had a precise definition of what it meant to do an editable install, we'd be having a much different conversation. As it stands, a But at this point we're just arguing all of the points that were covered in the PEP discussion. And we're not going to end up at any different conclusion. Even if we declared PEP 660 a failure, and abandoned it, we'd still only have symlinks, import hooks and Footnotes
|
I don't think that any of the strategies mentioned so far is perfect either. When I said that symlinking would work, I meant that it would have the intended outcome. That it can't be relied on on Windows makes it a lot less than "perfect". When I said PEP 660 isn't equipped to handle anything this intricate, I meant that the delivery medium for PEP 660 is the wheel and pre-installing packages at some unspecified location on disk, and presumably not recording them in the distribution's metadata, is very much not something that the PEP invites. |
Somehow I haven't stated this already: As much as I agree that the strict mode will be better than the status quo in terms of a UX, I don't think that coupling the changeover with PEP 660 is a good idea.
Sure. Then frontends would need to make the choice, or expose the users to a mechanism to pick on their end. And they'd need to be mindful of backwards compatibility concerns if they made the change that you want to force setuptools to make, in the same manner that setuptools needs to right now. I can tell you that pip would not do this work, and just have the pth based approach as the default for quite a while; if we'd gone with a design like that. It isn't an "issue" with the PEP, it's a consequence of the fact that you absolutely want to change behaviours in the initial implementation. Making drastic, backwards incompatible changes without putting effort to reduce the transition costs/churn is how you burn goodwill with users. None of the proposals magically avoid the backwards compatibility concerns in any way or remove the change management/communication work needed in anyway; they shift who has to do that. |
Yes, if you also manage the rest of the build-time dependencies of the project. It would be a viable workaround, but certainly not something that I'd consider to be "smooth".
Use an older version if the newer version breaks things for you, is certainly an option. I'm not sure I'm quite on board with the idea of having users pin their setuptools versions in pyproject.toml, but... sure. It's strictly worse than providing a knob to only change the editable behaviour.
+1 |
Right now, setuptools is the only backend that wants to change how editable installs work.
Only one popular option does not: pip. As @pfmoore said:
No? Unless you want to standardise the name of the config setting that only setuptools is going to use, I don't see what else can be done TBH. It's not outside of the standard, but it is backend specific configuration. Or maybe you want to have a separate dedicated option to consider it "in standard", I still don't think that has much value. As far as I can tell, exactly 1 backend is considering implementing something other than the existing pth file mechanism. Heck, exactly one backend maintainer wants to implement something other than the existing pth file mechanism: @pganssle. I don't see how standardisation would have had much value here. |
PEP 660 did not standardise the nature of "editableness". If backends are only expected to use |
Nowhere in any of the discussions on editable installs, did we achieve any sort of consensus on what an "editable install" should be. People seemed comfortable with leaving it implementation defined. Was that wrong? Maybe as a community we should be more strict over how much implementation defined behaviour we're willing to accept in PEPs?
Maybe. But hindsight is easy. Why did no-one suggest that at the time? Both PEP 660 and PEP 662 left the definition implementation-defined, and a huge amount of the discussion was around ways of implementing "better" editable installs. Would anyone have got consensus if they'd said "let's just standardise a way of injecting a Given that PEP 660 is currently still provisional1, we could propose a change to define editable installs precisely, requiring a Footnotes
|
I remember this coming up in between the discussions but got quickly discarded as too restrictive. I for one would be against such a narrow scoped definition. Note I'm not a backend maintainer though. pth files are cheap to implement IMHO but wrong on many levels, so if we're going to standardize something we should aim (or at least allow) for something better. I also don't entirely agree that no backend wants to do anything else, I think they'll do it eventually, but the cheapest solution is the pth, so naturally, that will get the quickest implementation.
I believe this was the only way we could reach a consensus because everyone had a different implementation in mind at the end of the day or a different definition of what editable installation nature qualifies. |
I don't know if that was wrong. But maybe we should show some lenience if that's a door we intentionally left open instead of treating setuptools like an anomaly because they are thinking about trying something different.
Please re-read my response in context. |
Ah, apologies, I did indeed misread your comment. |
I no longer have the motivation to work on this given the .pth approach isn't acceptable. Figuring out how to basically rewrite the entire editable install mechanism as my first contribution to setuptools is just not feasible or a good idea. |
Summary of changes
Implements the hooks required to support PEP 660 -- Editable installs for pyproject.toml based builds (wheel based). Resolves #2816.
I chose the root .pth editable strategy as it "just works" for implicit namespace packages and also looked easier haha. While most of the code is (hopefully) rather straightforward, it is quite verbose as unforunately setuptools doesn't have much in terms of wheel utilties.
There's also a somewhat sketchy hack used to acquire the distribution location so I can put the right path to ${dist-name}.pth in the wheel. Since the PEP 517 is isolated from the command framework, it's pretty hard querying information from the various setup.py commands ran. The shim added captures the egg_info instance when setup.py dist_info runs so the build_editable hook can query the location (i.e. egg_base).
One limitation of this commit is that the build_editable hook completely ignores the metadata_directory parameter and violates the guarantee the hook shall produce a (editable) wheel with identical metadata. Given that it seems no major backends is currently following through with this promise this is hopefully fine for now?
Pull Request Checklist
changelog.d/
.FWIW this is my first contribution to setuptools so I'm a little nervous I've messed everything up. Hopefully this isn't too bad :)