-
-
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
Add build_meta_legacy backend #1652
Conversation
9e95853
to
6fcc7bf
Compare
Not sure when I'll get to this; I have a busy week ahead. It's on my to-do list; I hope to get to it next weekend or week. |
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 not going to "approve", because I don't understand the setuptools backend well enough to feel comfortable doing so. I have added some comments, though.
As a general statement, I assume the changes to build_meta.py
are simply a refactoring to allow subclassing by the legacy backend. But I don't really understand some parts of the main backend, for example get_build_requires
which seems to be somehow introspecting setup.py
to locate requirements, something I don't follow. So all I can really say is "if the main backend is right, the legacy backend probably is" - but given that we're in this situiation precisely because the main and legacy backends need to behave differently, that's not very helpful :-(
In summary - see my comments in the code itself. Otherwise, I can't see anything wrong but I'm not qualified to give anything more than a superficial review.
@jaraco This is apparently breaking tons of packages... even Guido has noticed. I totally hear not having time though. Would you feel comfortable delegating someone else to handle reviewing/releasing a fix here? Regarding the overall plan: maintaining two backends like this seems like a lot of complexity. If I understand correctly, the payoff for this is that in the future, projects who explicitly set their build-backend to setuptools will be able to use a backend that's just like the default backend, except that This... seems like a pretty weak rationale to me? import sys; sys.path.remove("") and they're done. Maybe it would be better to accept that |
I still think the better quick fix is to modify pip for now, and then decide on the future direction in |
Developers shouldn't have to switch their backends entirely away from Assuming pypa/pip#6210 (or at leasts its tests) get accepted, then there'll be an ongoing integration test that ensures both the "standard PEP 517" and "legacy backwards compatibility" paths get exercised. |
Just noting that I tested this branch out locally as described in pypa/pip#6210 (comment), and with my suggested fix to add |
I've said elsewhere I think we should take our time with this one since it's a public API change, whereas That said, if @jaraco and/or @benoit-pierre agree on the general direction of this, I think I've gotten a lot of good comments for the review of the actual code, so I can always merge and cut a release myself. |
@pganssle I'm testing my two Comparing that to the successful install in pypa/pip#6210 (comment) makes me think we still have some more testing to do to be confident we have all the pieces lined up for this approach to handle things correctly. |
Per Nick Coghlan's suggestion on PR pypa#1652, a try/finally block ensures that the path is restored even in the event of an error.
I've updated this branch addressing the review comments. If it still doesn't work for PyInstaller I suppose we can start with some more tests to figure out why it's not helping. |
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.
Pending @ncoghlan comment I think this is good to ship.
Success! pypa/pip#6212 (comment) It looks like there's currently an unrelated issue with |
OK, I think we can probably merge this if @jaraco and @benoit-pierre agree with the general approach, then we can set about documenting it. |
7ce2484
to
c40ce70
Compare
Per Nick Coghlan's suggestion on PR pypa#1652, a try/finally block ensures that the path is restored even in the event of an error.
Pinging @jaraco and @benoit-pierre on this. :) |
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.
This change looks okay to me; a sound approach. I have a few comments/concerns (order of increasing importance):
- I've never liked the name
build_meta
. This change further cements (and expands) the name. I know it was me who suggested something other thanpep517
which also isn't a great name. Why not simplysetuptools.build
andsetuptools.build_legacy
(obviously with some deprecation period forbuild_meta
)? If there's support for that, better to adoptbuild_legacy
here rather than the less desirable name. - I think I understand the motivations behind this change, but it's not fully clear to me from the changelog entry. Does pip need another release to honor this change?
- I vaguely recall a consideration in the past to drop the implicit expectation that
setup.py
was run from the project directory and that directory was onsys.path
, but the decision was to retain that behavior indefinitely. Is the intention here to make an explicit departure from that latter assumption? - Do we expect this legacy backend to have a sunset period, or are there cases where we can imagine this legacy backend being used indefinitely? If the latter, it needs a better name than 'legacy'. What does this change imply for projects with an explicit setuptools build declaration? Which backend should a package specify? Can we provide some guidance somewhere (setuptools docs, packaging guide, ...) that describes the recommended best practice?
I don't want my questions or concerns to block this from moving forward, so I'm going to approve it. We can certainly iterate on all of these concerns (although naming is best addressed early).
I think no real objections here, though now that I'm thinking about it, why don't we move
That is the immediate motivating concern, yes, but more generally the idea is that in the future this will make it easier for us to distinguish between people who have opted-in to PEP 517 explicitly and those who are using it because it is the default. We need to be vigilant about backwards compatibility with
Yes, I think it will have a sunset period, but it will be long - after |
My personal preference would also be for |
We can just forget the whole thing. Someone else can "fix" |
@pganssle, I appreciate your frustration, and the bikeshedding over names was unhelpful. You have my apology for my comment, I should have stayed silent. I made the mistake of thinking that @jaraco's comment implied that naming suggestions would be welcome, while from my own experience I know better - bikeshedding about naming is never a good idea. I hope you'll reconsider closing this PR. FWIW, I'm 100% in favour of this change as it stands. |
@pganssle I'm sorry, I didn't mean to rubbish what you're trying to achieve here. I get that there aren't many opportunities to push people relying on setuptools to use it a better way. I should have known better than to drop another option into the naming question. pip appears to be willing to cut a bugfix release to resolve the issue, so it's not important that it gets fixed just by a release of setuptools. It's more important to reach some decision - even if that's a decision that it will take longer to work out, so pip should use a temporary workaround for now. |
Note: If you are not interested in the back-and-forth about naming, please skip ahead two posts. This post is about why I closed the PR, the next post is my response to the naming convention, and the third post is an important question about the timing of the rollout. @pfmoore @takluyver Thank you for my apologies, though I can assure you that they were unnecessary. I would not want to discourage anyone from speaking their mind, particularly on a significant cross-project API change. The reason I closed the PR is that it seemed to me that I had originally thought people were on board with this distinction, but I got the impression from the pile-on after Nathaniel's comment that it's actually just me who cares about preserving the distinction between the "default backend" and our explicitly provided public build hooks. I think I have done a fairly good job advocating for my position to this point, and if all I've managed to do is get people grudgingly on board so that any fix can be committed, then I see that as my failure to convince the community as a whole that my approach is worth adopting. I apologize for the manner in which I closed the PR, my frustration at having failed to achieve a consensus on this, combined with my frustration about the course of the "backend bootstrapping" discussion evidently soured my disposition, and looking back on my closure message it's a lot harsher than I intended. If others are interested in continuing the discussion, I'm willing to address the points made about the naming. |
This may be true, but "we might get rid of it" is not the reason I chose "legacy" as the name, and it's not the reason that we should tell people not to use it. The whole point of having the legacy backend is to have a clean way for Aside from the question of the path semantics, a major reason to maintain two separate backends is that over time and as more people start to opt-in to Regarding the name "legacy", it's true that it's non-specific and we could go with something like
The reason I think that the default option needs to be something new that has never existed before is that Yes it would be nice if we didn't have to worry about changing how |
I know I've dumped a lot of text here, but even for those who don't want to read my last two posts, I think we should set a timeline here. My ideal situation would be one where @pradyunsg You seemed to indicate that if this change goes in before Feb 7/8, we can avoid the workaround release to |
I'm sorry for dragging in the naming concerns. I meant them to be small concerns but to avoid proliferating bad names. The |
Yep, that sounds great to me. :)
+1; I should have made it clearer in my previous comment. Apologies for piling on in the bike shedding. |
@pganssle Thanks for your explanation of the intent behind the naming. Wearing my copy editor hat, I'd suggest that "setuptools.build_meta:implicit" may convey that intent better than "legacy" does, as the key semantic point we want to convey is "This backend is for frontends that are opting projects in to PEP 517 implicitly, so it should never be used explicitly in pyproject.toml", not "This backend is for projects that want or need the traditional legacy setup.py semantics". |
This is another fair name, but I don't think it's in common use and would obviously be understood. Also, to me Another option might be |
Yeah, |
I've updated pypa/pip#6212 with a couple of placeholder assumptions:
Those are both trivial to change if they're incorrect - I just wanted to see how that PR looked with the for-testing-only local post-release references removed. |
OK, I've updated it to |
The PEP-518 mandated that all packages having the pyproject.toml must specify the build-backend section [1]. This field was not defined in Snippy pyproject.toml file previously. There has been updates at least to setuptools [2] to allow compatibility mode without this parameter. The referred change [2] is very recent so it is not fully understood why the installation has been (?) working when this project has the pyproject.toml. The reason is likely that the similar case is wide spread among Python projects and it has been sorted out somehow. This is not fully understood how this goes. Good reading in [1] and [3] related to latest updates on Python packaging on top of PEP-517 and PEP-518. [1] https://www.bernat.tech/growing-pain/ [2] pypa/setuptools#1652 [3] https://www.bernat.tech/pep-517-518/ Signed-off-by: Heikki Laaksonen <laaksonen.heikki.j@gmail.com>
@pganssle @jaraco I think this should be released ASAP because the current version of Pip already uses this... Obtaining file:///home/travis/build/webknjaz/molecule
Installing build dependencies: started
Installing build dependencies: finished with status 'done'
Getting requirements to build wheel: started
Getting requirements to build wheel: finished with status 'error'
Complete output from command /home/travis/build/webknjaz/molecule/.tox/ansible27-unit/bin/python /home/travis/build/webknjaz/molecule/.tox/ansible27-unit/lib/python2.7/site-packages/pip/_vendor/pep517/_in_process.py get_requires_for_build_wheel /tmp/tmpSKpl9C:
Traceback (most recent call last):
File "/home/travis/build/webknjaz/molecule/.tox/ansible27-unit/lib/python2.7/site-packages/pip/_vendor/pep517/_in_process.py", line 211, in <module>
main()
File "/home/travis/build/webknjaz/molecule/.tox/ansible27-unit/lib/python2.7/site-packages/pip/_vendor/pep517/_in_process.py", line 201, in main
json_out['return_val'] = hook(**hook_input['kwargs'])
File "/home/travis/build/webknjaz/molecule/.tox/ansible27-unit/lib/python2.7/site-packages/pip/_vendor/pep517/_in_process.py", line 48, in get_requires_for_build_wheel
backend = _build_backend()
File "/home/travis/build/webknjaz/molecule/.tox/ansible27-unit/lib/python2.7/site-packages/pip/_vendor/pep517/_in_process.py", line 39, in _build_backend
obj = getattr(obj, path_part)
AttributeError: 'module' object has no attribute '__legacy__' |
This is released with 40.8, not ? |
Oh... Then I probalby just hit a bug with Pip |
@webknjaz Yes, it was released before pip started using it. Possibly you are getting bitten by the |
@pganssle oh.. might be, but those have sufficient version I think. Do you have a bug number? |
I assume you referred to pypa/pip#6264 |
👌yes it's that |
Summary of changes
Per discussion on #1642 and pypa/pip#6163, this implements a new PEP 517 backend explicitly designed as a "default value" when opting people in to using PEP 517.
build_meta
is the "opt-in" version, allowing us to design it with fewer backwards compatibility purposes.Supercedes #1643.
This may be all we need to do for #1642, with the rest of the changes on
pip
's side, though I'm not sure how this works with #1644.Also I have not explicitly documented this anywhere, because I'm not sure how public we actually want it to be. Presumably it should have some documentation.
Pull Request Checklist