pypa / setuptools

Official project repository for the Setuptools build system
https://pypi.org/project/setuptools/
MIT License
2.36k stars 1.15k forks source link

Add build_meta_legacy backend #1652

Closed pganssle closed 5 years ago

pganssle commented 5 years ago

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

jaraco commented 5 years ago

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.

njsmith commented 5 years ago

@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 setup.py executes without "" on sys.path. Is that right?

This... seems like a pretty weak rationale to me? setup.py has been running with "" on sys.path for like 20 years now, and if anyone wants to opt-out, then they can already write

import sys; sys.path.remove("")

and they're done.

Maybe it would be better to accept that "" on sys.path is simply part of setuptools's API, and skip the double-backend stuff? That would also be a much simpler change to review and release, at a time when packaging is on fire...

ncoghlan commented 5 years ago

sys.path.remove("") won't do anything in most setup.py invocations - you have to do sys.path.remove(os.path.dirname(os.path.abspath(__file__)).

I still think the better quick fix is to modify pip for now, and then decide on the future direction in setuptools without the pressure of installs from PyPI being broken for a non-trivial number of packages.

ncoghlan commented 5 years ago

Developers shouldn't have to switch their backends entirely away from setuptools to gain the defence against bootstrapping issues that the standard PEP 517 behaviours offers.

Assuming https://github.com/pypa/pip/pull/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.

ncoghlan commented 5 years ago

Just noting that I tested this branch out locally as described in https://github.com/pypa/pip/pull/6210#issuecomment-458125598, and with my suggested fix to add os.path.dirname(os.path.abspath(setup_script)) rather than adding '', it passed the new pip tests I added for this behaviour.

pganssle commented 5 years ago

@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?

I've said elsewhere I think we should take our time with this one since it's a public API change, whereas pip can revert the "opt in to PEP517" behavior until opting in to PEP 517 is no longer a backwards-incompatible change.

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.

ncoghlan commented 5 years ago

@pganssle I'm testing my two pip branches now, and haven't yet successfully installed PyInstaller 3.4 with the version based on the new setuptools backend: https://github.com/pypa/pip/pull/6212#issuecomment-458148519

Comparing that to the successful install in https://github.com/pypa/pip/pull/6210#issuecomment-458144828 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.

pganssle commented 5 years ago

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.

ncoghlan commented 5 years ago

Success! https://github.com/pypa/pip/pull/6212#issuecomment-458166386

It looks like there's currently an unrelated issue with --no-cache, and I just managed to trip over it while trying to test this change.

pganssle commented 5 years ago

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.

pradyunsg commented 5 years ago

Pinging @jaraco and @benoit-pierre on this. :)

pganssle commented 5 years ago
  1. 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 than pep517 which also isn't a great name. Why not simply setuptools.build and setuptools.build_legacy (obviously with some deprecation period for build_meta)? If there's support for that, better to adopt build_legacy here rather than the less desirable name.

I think no real objections here, though now that I'm thinking about it, why don't we move build_meta_legacy into build_meta.py, and change the default backend to setuptools.build_meta:legacy? Then we can transition to setuptools.build later, and build_meta:legacy will be switched over to build:legacy.

  1. 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?

pip needs another release to make the legacy backend the default, though obviously anyone could explicitly specify it if they want (though the point of this is to have a clearer way to distinguish between people who have explicitly specified a backend and those who haven't specified anything).

  1. 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 on sys.path, but the decision was to retain that behavior indefinitely. Is the intention here to make an explicit departure from that latter assumption?

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 setup.py invocations for the people using it because it's the default, whereas people opting in to the new build backend explicitly can be expected to have tested it for their use case and if something about the new backend has slightly different semantics, they can adjust their build files.

  1. 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?

Yes, I think it will have a sunset period, but it will be long - after pip deprecates non-PEP 517 builds and we can be sure that everyone's a least using the legacy backend. If / when we document this, we should explicitly state that you should not specify that your build-backend is setuptools.build_meta:legacy and that instead you should make adjustments to your setup.py as necessary to use setuptools.build_meta.

pfmoore commented 5 years ago

I've never liked the name build_meta.

FWIW, flit calls it buildapi.

Does pip need another release to honor this change?

Yes, The name is hard-coded into pip as "the backend to use if the user doesn't specify one".

pganssle commented 5 years ago

I've pushed a new change moving this to setuptools.build_meta:legacy.

pganssle commented 5 years ago

I went to add this to the documentation and it turns out that build_meta itself is also undocumented, so I think we can handle documentation for both of these a bit later.

njsmith commented 5 years ago

@jaraco

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'.

Realistically, I think you should assume that whatever name you choose here will be enshrined in PEP 517, documented, and will have to be supported for at least a decade.

Old packages (ones without an explicit pyproject.toml backend) will continue to exist, this is how you build old packages, and anyone consuming packages from PyPI is going to keeping relying on this backend to do it, indefinitely.

@pganssle

If / when we document this, we should explicitly state that you should not specify that your build-backend is setuptools.build_meta:legacy and that instead you should make adjustments to your setup.py as necessary to use setuptools.build_meta

This strikes me as pure fantasy. You're never going to get rid of the legacy backend, so there's no harm in people using it. And any actual problems caused by running setup.py with . on sys.path were worked around long ago. At this point, all you're going to accomplish by having two almost-but-not-quite-identical backends is to make diligent folks who trust you do pointless makework, confuse users, and produce ongoing fights between the people who trust you and think that everyone needs to use the new backend, versus people who look at the reality and decide that the old backend is fine.

There's a long tradition of this kind of thing in Python packaging, which is part of why so many people hate us. That's no reason to continue it, though...

takluyver commented 5 years ago

Going one step further, does it make sense to call the 'legacy' backend 'build_meta', and the new one something like 'build_hygienic'? This would mean pip doesn't need a new release to change the default.

I don't think it's realistic to get rid of the 'legacy' backend on any timescale that would suggest calling it 'legacy'.

On Mon, 4 Feb 2019, 01:05 Nathaniel J. Smith <notifications@github.com wrote:

@jaraco https://github.com/jaraco

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'.

Realistically, I think you should assume that whatever name you choose here will be enshrined in PEP 517, documented, and will have to be supported for at least a decade.

Old packages (ones without an explicit pyproject.toml backend) will continue to exist, this is how you build old packages, and anyone consuming packages from PyPI is going to keeping relying on this backend to do it, indefinitely.

@pganssle https://github.com/pganssle

If / when we document this, we should explicitly state that you should not specify that your build-backend is setuptools.build_meta:legacy and that instead you should make adjustments to your setup.py as necessary to use setuptools.build_meta

This strikes me as pure fantasy. You're never going to get rid of the legacy backend, so there's no harm in people using it. And any actual problems caused by running setup.py with . on sys.path were worked around long ago. At this point, all you're going to accomplish by having two almost-but-not-quite-identical backends is to make diligent folks who trust you do pointless makework, confuse users, and produce ongoing fights between the people who trust you and think that everyone needs to use the new backend, versus people who look at the reality and decide that the old backend is fine.

There's a long tradition of this kind of thing in Python packaging, which is part of why so many people hate us. That's no reason to continue it, though...

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/pypa/setuptools/pull/1652#issuecomment-460106296, or mute the thread https://github.com/notifications/unsubscribe-auth/AAUA9VPDidv1SF_YhjsMiy7MdiqNmRK2ks5vJ4d2gaJpZM4aU1-L .

pradyunsg commented 5 years ago

I'm definitely all for build_meta being the "legacy" behavior providing backend. A different setuptools.build backend being the one people are explicitly told to use (with no sys.path[0] == '').

It also has the benefit of working for people who (like pip) thought the setuptools.build_meta backend will be 100% backwards compatible and adopted early - I'd expect documentation to push future adopters to usesetuptools.build.

pfmoore commented 5 years ago

My personal preference would also be for build_meta to be the backwards-compatible one, because that avoids any changes to pip, and allows the current release of pip to "just work" once a new setuptools is released.

pganssle commented 5 years ago

We can just forget the whole thing. Someone else can "fix" setuptools.build_meta and we can lose our opportunity to urge people into a more sane build configuration if desired.

pfmoore commented 5 years ago

@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.

takluyver commented 5 years ago

@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.

pganssle commented 5 years ago

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.

pganssle commented 5 years ago

This strikes me as pure fantasy. You're never going to get rid of the legacy backend, so there's no harm in people using it.

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 setuptools to distinguish between users who have explicitly chosen to use PEP 517 and users who are using the PEP 517 backend by default and know nothing about build backends. It makes no sense to maintain the legacy backend as an explicitly-specified build backend because it defeats the entire purpose. Obviously people will do it for the sake of expedience and that's fine, but they should be on notice that it is not a supported configuration, whatever that might mean.

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 build_meta, they may come to rely on some feature of build_meta that was actually an accidental backwards-incompatible change. It's unlikely of course, but it may already be true that some projects are relying on the fact that '' is not in sys.path. Having one backend explicitly scoped as "this is an opt-in backend that attempts to provide build isolation semantics" and a second backend explicitly scoped as "this is an attempt to emulate python setup.py install for people who haven't moved over yet" should help with the decisionmaking of what to do in the future.

Regarding the name "legacy", it's true that it's non-specific and we could go with something like default or compat, but I think the name of the backend should actually convey that it's not intended to be something you specify. If we go with default people may think, "Oh, I don't need anything special, I'll do the default one". If we choose "compat" it's less likely they'll think that it's the default, but they may get the false impression that specifying it in your pyproject.toml is a supported configuration, which it is not.

Going one step further, does it make sense to call the 'legacy' backend 'build_meta', and the new one something like 'build_hygienic'? This would mean pip doesn't need a new release to change the default.

The reason I think that the default option needs to be something new that has never existed before is that build_meta has been available for some time now and most people using tox's PEP 517 support or who pre-emptively opted-in to build_meta will have already started using it. If we want there to be a very obviously clear dividing line between people who have opted in to using setuptools's build support and those who haven't, we need to introduce a brand new backend that from the beginning is explicitly specified as being only for use by build frontends as a default compatibility option.

Yes it would be nice if we didn't have to worry about changing how pip works, but unfortunately that's the nature of the beast.

pganssle commented 5 years ago

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 pip does not need to make a "workaround" release and instead we roll out the setuptools change along with a pip patch release switching the default backend.

@pradyunsg You seemed to indicate that if this change goes in before Feb 7/8, we can avoid the workaround release to pip. How about we give this another 24 hours or so for discussion and bikeshedding and whatever else, and after that I'll either close this PR or merge whatever we've converged on and cut a new setuptools release, depending on the conclusion. Hopefully that is a reasonable timeframe for getting something approximating the "right" solution out?

jaraco commented 5 years ago

I'm sorry for dragging in the naming concerns. I meant them to be small concerns but to avoid proliferating bad names. The build_meta:legacy naming is a very welcome approach... and the minimal way in which it affects the behavior is also welcome. I've no objections to proceed as Paul has outlined.

pradyunsg commented 5 years ago

@pradyunsg [snip] Hopefully that is a reasonable timeframe for getting something approximating the "right" solution out?

Yep, that sounds great to me. :)


FWIW, I'm 100% in favour of this change as it stands.

+1; I should have made it clearer in my previous comment.

Apologies for piling on in the bike shedding.

ncoghlan commented 5 years ago

@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".

pganssle commented 5 years ago

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 legacy definitely has a connotation that implies that you would not use it for new code.

Another option might be __legacy__, to the extent that people generally know not to explicitly invoke things with the "magic method" naming convention.

ncoghlan commented 5 years ago

Yeah, setuptools.build_meta:__legacy__ would be a pretty strong "This probably isn't what you want" hint, especially if combined with docs that explained it was for installer frontends wanting an implicit PEP 517 backend for projects that didn't specify one explicitly.

ncoghlan commented 5 years ago

I've updated https://github.com/pypa/pip/pull/6212 with a couple of placeholder assumptions:

  1. That the next setuptools version will be 40.8.0
  2. That the compatibility-mode backend name will be setuptools.build_meta:__legacy__

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.

pganssle commented 5 years ago

OK, I've updated it to build_meta:__legacy__. If anyone has any objections to the "magic method" naming convention, let me know soon, as I plan to merge this and cut a release in the next few hours.

webknjaz commented 5 years ago

@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__'
gaborbernat commented 5 years ago

This is released with 40.8, not ?

webknjaz commented 5 years ago

Oh... Then I probalby just hit a bug with Pip

pganssle commented 5 years ago

@webknjaz Yes, it was released before pip started using it. Possibly you are getting bitten by the --system-site-packages bug?

webknjaz commented 5 years ago

@pganssle oh.. might be, but those have sufficient version I think. Do you have a bug number?

webknjaz commented 5 years ago

I assume you referred to pypa/pip#6264

gaborbernat commented 5 years ago

👌yes it's that