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

Documentation to mention pyproject.toml and build requirements #9796

Closed
wants to merge 2 commits into from

Conversation

benjaoming
Copy link

@benjaoming benjaoming commented Apr 9, 2021

Fixes #6345 (or at least gets into fixing some of the issue)

Has there been any documentation written about pip and pyproject.toml elsewhere? Poetry has a nice overview of how it uses it: https://python-poetry.org/docs/pyproject/

@github-actions github-actions bot added the needs rebase or merge PR has conflicts with current master label Sep 10, 2021
@pradyunsg
Copy link
Member

I will be documenting how pip builds projects (which is where pyproject.toml comes into play) as part of #9475.

@benjaoming Would it be fine if I close this out? I intend to rewrite this whole section of pip's documentation, as part of a WIP documentation change I have locally -- that's part of the broader #9475 effort.

@benjaoming benjaoming closed this Sep 18, 2021
@benjaoming benjaoming deleted the setup_requires branch September 18, 2021 21:19
@benjaoming
Copy link
Author

That's fine, good luck with the rewrite 👍

@benjaoming
Copy link
Author

It does worry me why a simple PR like this isn't getting reviewed and merged. But I have no idea who to address that to.

@@ -1225,3 +1215,6 @@ Examples

.. [1] This is true with the exception that pip v7.0 and v7.0.1 required quotes
around specifiers containing environment markers in requirement files.


.. _PEP_518: https://github.com/django-money/django-money/issues/595
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some kind of copy-paste error here - should have been https://www.python.org/dev/peps/pep-0518/

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Duly noted! The original reference can be updated to be:

:pep:`518`

Which is much easier to maintain, and avoid errors like this. :)

@pradyunsg
Copy link
Member

pradyunsg commented Sep 19, 2021

It does worry me why a simple PR like this isn't getting reviewed and merged.

Understandably so! I can elaborate on my line of reasoning here, in case that helps ease the worry. :)

I did review the changes in this PR when I wrote that comment (albeit a long time after it was filed, and only because it has merge conflicts and I was cleaning up PRs-with-merge-conflicts). In this case, the conflicts have been caused by the documentation rewrite moving stuff around and rewriting some of these things.

As it turns out, right now I have a decently big WIP change locally, that touches the same sections as this, with fairly similar changes that I've made independently -- it's consolidating https://pip.pypa.io/en/stable/cli/pip/#build-system-interface, https://pip.pypa.io/en/stable/cli/pip_wheel/#build-system-interface, https://pip.pypa.io/en/stable/cli/pip_install/#build-system-interface and https://pip.pypa.io/en/stable/cli/pip_install/#controlling-setup-requires into a single document/section about pip's build system. :)

Now, I figured there are two options:

  • I work with you here to update this PR, rebase my WIP changes on top of this and accomodate for these changes. This has the caveat of not knowing your availability, so I don't know how long the first step will take.
  • I can pick this up as part of the rewrite, and handle updating installation order section. Then, all I need to do is figure out how to make these changes in the context of the rewrite.

The latter is clearly less work for everyone involved IMO -- it skips two steps, including the one with uncertainty! -- to end up in a very similar situation. And given that I know that I'll have availability to pick this up, I figured it would be the right thing to do here. :)

@pfmoore
Copy link
Member

pfmoore commented Sep 19, 2021

It does worry me why a simple PR like this isn't getting reviewed and merged. But I have no idea who to address that to.

To further comment on this (as @pradyunsg noted, it had been mostly neglected since April) the reason is basically that there are very few pip maintainers (three who are relatively active at the moment, and I'm being generous by including myself in that as I do little more than reviews and design discussions at the moment), and we're all pretty busy with what I'd view as higher priority issues (like it or not, documentation is often neglected when there's work to do fixing bugs or dealing with changes in the wider ecosystem).

I could offer some comments on the PR (for example, I'm not sure I think we should be linking to the "Awesome pyproject" site, I'm not at all sure I agree with the stance it takes and I don't think it's the place of pip's documentation to promote pyproject.toml for tool configuration anyway) but that's not really the point here - the point is that looking at the PR to formulate such comments takes time, and while I won't speak for the other committers, I know that I have very little of that.

My personal workflow is to get notifications from the issue tracker, skim them and if they are actionable, do something. That does prioritise high-activity issues and trivial ones (that can just be dealt with immediately), and de-prioritises issues that need more effort but don't catch my interest on the initial notification. And while that's not necessarily an ideal result, it doesn't actually do a bad job of allowing me to manage the time I have.

I don't think that necessarily helps address your concern that "simple PRs" aren't getting dealt with more quickly. But maybe it gives you some insight into why it happens.

And please understand that even though this PR won't get merged as-is, we're still very grateful to you for the contribution. It focused attention on this particular point in the docs, gave us some good suggestions on how to re-word it, and fed into the larger documentation update work. So there's no doubt in my mind that pip's documentation will end up better as a result of your input, and that's the main point IMO.

@benjaoming
Copy link
Author

Thanks for elaborating - I hope that maintainers are not feeling overwhelmed and we can help each other out for benefit of project and community combined.

I fully agree that changes to documentation now need to go into the extensive rewrite that @pradyunsg is working on.

It seems from both your comments that the resource issues isn't necessarily about maintainership (since you keep up and look for actionable stuff) but more about reviewers that do work to ensure quality of PRs?

There's one thing that still remains from this PR to be figured out though - @pfmoore wrote:

I don't think it's the place of pip's documentation to promote pyproject.toml for tool configuration anyway

From the PR:

The encouraged way of specifying build dependencies is `PEP_518`_, which gives
us one unique choice of configuration file for a Python project: ``pyproject.toml``.

Zooming out again to a general level: This would of course be a great thing to discuss with core maintainers that have a heavy-weighing concern. But perhaps reviewers could help feed the discussions so they turn into actionable conclusions before core maintainers have spent time on reviewing and discussing them.

@pfmoore
Copy link
Member

pfmoore commented Sep 20, 2021

There's one thing that still remains from this PR to be figured out though - @pfmoore wrote:

I don't think it's the place of pip's documentation to promote pyproject.toml for tool configuration anyway

I wasn't talking about the use of pyproject.toml in pip's case. Yes, we should definitely recommend pyproject.toml for build requirements and build backend settings. I was talking about the general tendency of tools other than build systems to put config in there (which is what the linked "Awesome pyproject" site seems to be promoting). To be 100% specific, I don't think we should link to that site (or refer to it) in the pip documentation. On a personal note, I largely disagree with the whole idea that the site is suggesting (tools in general should put their config in pyproject.toml) but I may be in a minority on that...

@pradyunsg
Copy link
Member

I'd like us to set aside the discussion on pyproject.toml and how folks feel about the use of it for tool configuration. A closed documentation PR related to build requirements isn't the best place to discuss something like that. :)

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 6, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
needs rebase or merge PR has conflicts with current master
Projects
None yet
3 participants