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

PEP 518 modifications required for PEP 517 #4997

Closed
ghost opened this issue Jan 26, 2018 · 26 comments
Closed

PEP 518 modifications required for PEP 517 #4997

ghost opened this issue Jan 26, 2018 · 26 comments
Labels
state: needs discussion This needs some more discussion

Comments

@ghost
Copy link

ghost commented Jan 26, 2018

I've removed some modifications from gh-4799 that aren't specifically required for PEP 518 compliance, but are an implementation detail that will greatly ease and simplify PEP 517. I plan to submit a separate PR with these modifications which should be merged after 10.0 is released, but I thought I'd start the discussion about these changes here. Please try to keep the discussion focused on this specific issue, because broader discussions have been shown to be less-than-productive.

Premise: To implement PEP 517 support, pip must run all setup.py scripts with a newer version of setuptools. This is a design decision that will simplify implementation, but I prefer not to discuss it in this specific issue (if this really needs to be discussed, and I think arguing about this point is a waste of time, then let's open another issue).

Question: How can we run all setup.py scripts with the latest setuptools while preserving backwards compatibility?

Well, we can't isolate projects without a pyproject.toml due to backwards compatibility concerns. However, what we can do is create a subclass of BuildEnvironment that prepends the newer setuptools to the PYTHONPATH so that it's loaded before the older setuptools.

In addition to these concerns, implementing this will imply the following points:

  • pip will no longer check for setuptools to be installed system-wide, because it will already install it before calling setup.py.
  • pip will always require a setuptools and wheel package to be available in the index. This is not normally a problem, but it does mean that we need to modify the tests so that pip can always access a setuptools and wheel package.
@pradyunsg
Copy link
Member

I plan to submit a separate PR with these modifications [snip] after 10.0 is released

Let's defer this discussion until after 10.0 then. :)

@pradyunsg pradyunsg added the state: needs discussion This needs some more discussion label Feb 15, 2018
@ghost ghost mentioned this issue Mar 4, 2018
@ghost
Copy link
Author

ghost commented Mar 4, 2018

@pradyunsg If you have time, are you opposed to this? Does it seem reasonable?

@ghost
Copy link
Author

ghost commented Mar 4, 2018

Actually, the simpler option may be to bundle a wheel of setuptools with pip.

@pradyunsg
Copy link
Member

I'm happy to have the use of PEP 517 be dependant on having a version of setuptools greater than some version number. I think that's obvious.

I think that just failing by saying, hey, you need a newer setuptools to install this package, should be acceptable. I don't see why we can't just do that.

@ghost
Copy link
Author

ghost commented Mar 4, 2018

Are you absolutely sure about that approach? I'm very afraid of causing a firestorm from RHEL folks.

cc @ncoghlan Is requiring a newer setuptools a problem for you?

@pradyunsg
Copy link
Member

If a project does not have a pyproject.toml, I don't think we want pip to do anything different from what it's doing today.

@ghost
Copy link
Author

ghost commented Mar 4, 2018

From a maintenance perspective, I would like the old source distributions to be internally hidden from the rest of pip. One approach to accomplish that would be to vendor wheel, and then run egg2dist to generate the dist-info directories.

@pradyunsg
Copy link
Member

pradyunsg commented Mar 4, 2018

Just to be clear, I'm saying when installing a pyproject.toml sdist, pip can require a newer version of setuptools being needed.

For an existing, non 517 sdist, pip should not change what it's doing. (i.e. I am opposed to playing with PYTHONPATH)

@pradyunsg
Copy link
Member

PS - it's late here and I'm a little sleepy. I'd rather not have this discussion at this time. In about 12 hours, I'll come back to this. =)

@pradyunsg
Copy link
Member

@xoviat did you see #5051? That should help with compartmentalising the legacy sdist vs modern sdist implementation differences.

@ghost
Copy link
Author

ghost commented Mar 4, 2018

The biggest difference is that legacy source distributions generate egg_info, whereas new distributions generate dist_info. My plan is to vendor wheel (egg2dist) so that pip doesn't have to parse egg_info anymore.

@pradyunsg
Copy link
Member

Uhm, I understand how this would simplify implementation but I really don't think it's a good idea to change the code that legacy sdist are being run through.

I'm happy to have some extra complexity to maintain as much compatibility as possible.

@ghost
Copy link
Author

ghost commented Mar 4, 2018

I will not implement PEP 517 unless I can change the code path for legacy sdists. I expect that this will expose bugs (that I will fix) in my implementation, but I have reasons for doing it this way. If you want, I guess we can wait another eight months (if that's what it takes) until you come around to my point of view, as happened with the gh-4799, or you can do it yourself.

@ghost ghost closed this as completed Mar 4, 2018
@ghost
Copy link
Author

ghost commented Mar 4, 2018

Back when I started on this, I recall that @pfmoore made some comment that legacy sdists would eventually be run through the setuptools build backend (I'm not going to spend time hunting down that particular comment). I started out with that approach, and I'm not going to change it after all of the technical investment that I've put into it.

@pradyunsg
Copy link
Member

I respect that. It's your time that you're volunteering and if you feel so, I'm not gonna try to convince you otherwise.

For all I know, you might be right and I might change my position over rather coming months.

@ghost
Copy link
Author

ghost commented Mar 4, 2018

For the record, I won't feel personally insulted if someone else comes along (possibly you) with another, better implementation. So if you have the time, go for it!

@pradyunsg
Copy link
Member

pradyunsg commented Mar 4, 2018 via email

@pfmoore
Copy link
Member

pfmoore commented Mar 4, 2018

To confirm - this change is for after pip 10 has been released. So there's no rush here. But I do think that if we change user-visible behaviour for legacy sdists as part of implementing PEP 517 in pip 11 or whenever, those changes should be documented in the changelog - particularly if they are backward incompatible. (I'm not interested in documenting internal details, those are not relevant to users).

@xoviat - my previous comment (to the extent that I recall the context) was about how we internally manage the builds. I'm fine with you rearranging internal code paths. But I do want to ensure we publicise any change to user-visible behaviour. Are you OK with documenting the user-visible changes resulting from your proposed approach? And @pradyunsg are you OK with judging whether the incompatibilities are acceptable on that basis?

@ghost
Copy link
Author

ghost commented Mar 4, 2018

To confirm - this change is for after pip 10 has been released. So there's no rush here. But I do think that if we change user-visible behaviour for legacy sdists as part of implementing PEP 517 in pip 11 or whenever, those changes should be documented in the changelog - particularly if they are backward incompatible. (I'm not interested in documenting internal details, those are not relevant to users).

That's correct. These changes are not going into pip 10.

@xoviat - my previous comment (to the extent that I recall the context) was about how we internally manage the builds. I'm fine with you rearranging internal code paths. But I do want to ensure we publicise any change to user-visible behaviour. Are you OK with documenting the user-visible changes resulting from your proposed approach? And @pradyunsg are you OK with judging whether the incompatibilities are acceptable on that basis?

What I was previously proposing is that the only changes in user-facing behavior is that pip would fetch a newer setuptools if the locally-installed setuptools did not meet a minimum version. What I am now proposing is that pip implements an internal build backend for legacy source distributions that is used when the locally-installed setuptools is too old, which requires vendoring wheel (which I don't think is a major issue).

To confirm, there will be no user-visible changes made intentionally with this method. But there may be unintentional changes made, which would be considered bugs. If we have minimal differences in the build backend code, these bugs will be much more easily exposed than having code that is only run when new source distributions are present (can you imagine a bug report for code that isn't run for the vast majority of packages...ugh).

@ghost
Copy link
Author

ghost commented Mar 4, 2018

I think this comment is pretty clear though (and in contradiction with your previous position):

I really don't think it's a good idea to change the code that legacy sdist are being run through.

I shouldn't have to say this, but disclaimer:

  • Your previous position may not be correct
  • I may have misinterpreted such position
  • People are allowed to change their minds

At least for me though, your previous position is currently my position.

@pfmoore
Copy link
Member

pfmoore commented Mar 4, 2018

@xoviat You realise you were quoting @pradyunsg not me? I've never claimed that internal changes shouldn't be allowed. Although I understand what @pradyunsg is saying, if we make changes to what is effectively the backward compatibility code path, we're risking breakage to that compatibility. While pip doesn't have a formal backward compatibility policy, we do need to acknowledge that a lot of people are relying on "what pip does", and we don't have comprehensively documented behaviour (although we're getting better) so any behaviour change might impact someone.

Personally, I'd like to see the various internal code paths better documented (for maintenance purposes, not as something we guarantee to end users) but I understand your reluctance to spend time on doing that. Please forgive me if I misunderstand your proposals as a result - I am trying to keep what's happening now, what's proposed, and what's been discussed but discarded, clear in my mind. One day, if I have time to understand the code well enough, I'll scratch my own itch on this one and write some docs myself 😄

@ghost
Copy link
Author

ghost commented Mar 4, 2018

Yes, your previous position contradicts @pradyunsg's position (and this is where I become frustrated with human intuition, because such a statement logically says nothing about your current position, but a human might think I've claimed that your position has changed).

@ncoghlan
Copy link
Member

ncoghlan commented Mar 5, 2018

Even before I left Red Hat, my recommendation to RHEL users was:

  • always use virtual environments
  • always run "pip install --upgrade pip setuptools wheel" as the first command in that environment

Until Red Hat are actually paying someone to work on upstream maintenance of Python packaging tools, I think we should continue that policy, and tell anyone that doesn't like it to talk to their Red Hat account manager, not us.

So I think a hard requirement on a newer setuptools in pip for PEP 517 source builds would be fine. If Red Hat (or Red Hat's customers) don't like it, that's a problem for Red Hat to figure out - they're the ones currently making billions of dollars a year off RHEL subscriptions, after all :)

@pradyunsg
Copy link
Member

@ncoghlan That policy sounds good.

and this is where I become frustrated with human intuition, because such a statement logically says nothing about your current position, but a human might think I've claimed that your position has changed

But since you've stated it, there's no chance of this ambiguity existing anymore. :)

@pradyunsg are you OK with judging whether the incompatibilities are acceptable on that basis?

I'm still concerned about things breaking unintentionally or re-exposing a previously fixed issue but we can go ahead with that. That's a known risk with any change.

Looking at @xoviat's comments now, with a well rested mind, makes me think what he/she's saying makes sense and, right now, is the best way to getting PEP 517 implemented on a reasonable time scale.

@pradyunsg
Copy link
Member

I'm fine with you rearranging internal code paths. But I do want to ensure we publicise any change to user-visible behaviour.

Basically, I'm a +1 to this now.

@ncoghlan
Copy link
Member

ncoghlan commented Mar 5, 2018

Regarding user notifications in the release notes, it's reasonable to add a note like "The build process has been refactored as part of adding support for pyproject.toml based sdists without a setup.py file. While this refactoring shouldn't have affected pip's ability to build any existing setup.py based project, it's worth keeping in mind as a potential source of problems if an sdist builds correctly with pip 10.x, but fails to build with pip 11.x".

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 15, 2021
This issue was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
state: needs discussion This needs some more discussion
Projects
None yet
Development

No branches or pull requests

3 participants