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

Add PEP 517, PEP 518 & PEP 660 to specs and glossary #1111

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

CAM-Gerlach
Copy link
Contributor

@CAM-Gerlach CAM-Gerlach commented Jul 25, 2022

@pfmoore @brettcannon @pradyunsg

As discussed on #955 , this (finally!) migrates the content of PEP 517, PEP 518 and PEP 660 to the specifications section and glossary, and updates the various references throughout the site, specs and glossary to the PEPs, their contents and the terms they define to refer to, and accurately reflect, the newly canonical content spec and glossary content.

Top level preview

Overall, while I did put a lot of effort into diligently taking advantage of the appropriate reST syntax/directives/roles and Sphinx cross referencing for consistent source formatting and output rendering, implementing editorial corrections for organization, clarity and consistency, and making some limited and motivated additions and updates to the non-normative portions (mostly added introductory/connecting material and updating/fleshing out examples), I attempted to be conservative and avoid any substantive changes to the actual content of the normative specification, which can be discussed and applied subsequent to this initial PR.

On the high level, they end up most neatly fitting into three separate specs (links to preview):

  • A brief new meta spec covering the pyproject.toml file, containing the high-level content from PEP 518 (as well as a bit from PEP 621/the declaring project metadata spec) related to the pyproject.toml file as a whole, since it services multiple purposes beyond just the original build-system table, so it wouldn't make much sense nor be reader-friendly to bury it inside a build interface spec. It links to the individual specs for each table (which also helps address the confusion and concerns of multiple recent readers who had difficulty finding it and its subtables, since the previous names were highly non-obvious to them).
  • Broadening the existing "Declaring build system dependencies" specification to be a spec on Declaring a project's build system, covering the [build-system] table as a whole, containing the [build-system] content from PEP 518 and the build-system top-level section from PEP 517. This section includes a lot more than just the requires key so it should not be limited to it, but can fairly cleanly be separated from the actual build interface specification itself, and also helps partition the content of potential interest to regular package authors versus build frontend/backend developers
  • Finally, creating a new Build Frontend-Backend Interface specification that covers the hooks and the specified behavior of frontends and backends themselves from PEP 517 and PEP 660, as would be consulted by frontend and backend developers.
  • Also, updating the Glossary to include and reflect the new key terminology and concepts in these PEPs

I've made some relatively modest adjustments to the outline originally proposed in #955:

Final outline w/ mapping from existing PEP sections (click to expand)

After a fair amount of thought and experimentation, I've tweaked the current Package Distribution Formats heading to read Building and Installing heading and added/moved the new specs there in the Specifications Index, alongside the "Recording Installed Distributions" spec, given it is a much better fit for the new section than the crowded, core-metadata-focused "Package Distribution Metadata" section, particularly given it and the wheel spec that unpacks to it and the hooks that build it go hand in hand, and the new section now describes the build and install process from end to end.

On a procedural note, I made every effort to break this up into separate PRs or at the very least separate commits, as I normally would (and all the future specs should be). However, I ended up having to make this as a single atomic, squashed commit for practicality's sake, because otherwise there would be intractable circular dependencies between each PR, given the new specs, existing specs, glossary entries and other documents all heavily cross-reference and depend upon one another, and that would furthermore require a heavy cost in both time and quality/consistency, when this will be squashed down anyway, has been blocked on such for years and is much better spent migrating other specs and making further improvements to the packaging site in followup PRs.

I'll post a thread in the Packaging Discourse to announce this and solicit reviews of this, to both check for editorial correctness and ensure it accurately reflects the PEPs on which it is based.

Part of #1093

Resolves #955

@CAM-Gerlach
Copy link
Contributor Author

I didn't realize I had never actually contributed a PR of my own here, despite several reviews on others. If the review process is going to be intensive, perhaps worth going ahead with another PR doing another task I'd planned to, like #1101 , to avoid having to wait for workflow approval each time I change something (though I've of course tested and previewed locally).

@JDLH
Copy link

JDLH commented Jul 25, 2022

Great! Thank you. I have started to read this.

Comment on lines +53 to +54
Distribution
Package
Copy link
Member

@pradyunsg pradyunsg Jul 25, 2022

Choose a reason for hiding this comment

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

Please don't add these -- we explicitly don't use the short forms here, to avoid ambiguity across the board. This is also called out in the definition here.

If there's places where we're that using the short form to direct at this, I'd rather that we update those references to be more explicit.

@pradyunsg
Copy link
Member

pradyunsg commented Jul 25, 2022

@CAM-Gerlach Could you break this PR up into smaller atomic commits, that each changes a single thing at a time?

Right now, this PR is fairly difficult to review in a reasonable amount of time. There is a single commit that updates multiple pages for adding glossary references, adds new terms to the glossary, restructures the specifications section, adds the new documentation, and perhaps more that I didn't spot before realising that reviewing this will take way too long as-is.

Beyond that, I've only skimmed this PR so far, and I'm feeling a bit of "uhh... I don't think this is the right way to describe it" about the "integration frontend" term -- but I've not looked into the PR for long enough to check if that instinctive response is justified. :)

@pfmoore
Copy link
Member

pfmoore commented Jul 25, 2022

I agree with @pradyunsg - this is way too big to practically review. I read the summary of your approach above, as well as briefly skimming the structure of the PR, and my view is you're going about this the wrong way.

I should note that my key concern here is that we avoid any change in meaning between what ends up in the specs and what was agreed in the PEPs. That seems fundamental to me.

To that end, I think we should follow the same model as all other work to include PEPs in the spec pages.

  1. Copy the text of the PEP unchanged into the specs. PEP 518 can easily be migrated into "Declaring build system dependencies". PEP 517 and PEP 660 can go into a new section, with PEP 517 being the body of the section and maybe PEP 660 as a linked sub-page. See "Declaring project metadata" (PEP 621) for how this works. That's three PRs, one for each. This shouldn't need significant understanding of the specs to create or review, as the only real question here is "is the included copy identical to the original?"
  2. Once the PEPs are in the spec, we're in the normal situation of revising the packaging guide for clarity and readabilliy.
  3. Fixing links in the rest of the guide to point to the new spec sections should be straightforward. These can be one or more simple PRs, requiring no deep understanding of the specs to create or review.
  4. Cross-linking in the specs can also be done as follow-up PRs, with care taken to not change the meaning but otherwise these should be simple PRs.
  5. Adding summary sections and/or overviews can be done easily enough - but note that these should be in the guide, IMO, not in the specifications area, as they would be user-facing explanatory material, not specifications as such. To do this properly (and to review it) needs a good understanding of the specs, to make sure that the summaries don't misrepresent the detail. But as it's summaries, the text should be relatively short (if a summary is too long to review, it's too long to be a summary, IMO 🙂)
  6. Revising the wording of the spec should be done extremely cautiously, and following the PyPA spec change process (which I personally interpret pretty strictly these days, as I feel we've been burned rather too often - anything that changes the meaning should be considered as requiring a PEP). Again, this should be one change, one PR.

The key thing for me is that the wording as agreed in the PEPs is what the community agreed as the standard. Not all of the PEP content is normative, but as a community, we don't have a history of being particularly formal over specifications, and it's hard to always be precise about what was intended as significant (or what people view as significant after the fact). Maybe things would be easier if packaging specs were precise and unambiguous like (for example) RFCs. But that's not the reality, for better or worse. So preserving the PEP content as closely as possible is key here.

(Just to be clear, I'm not suggesting the content in this PR varies from the PEPs. I'm sure you transcribed it accurately. My suggestions above are intended simply to make it easier to handle the process of confirming that).

Comment on lines +31 to +32
A tool directly responsible for transforming a
:term:`Source Tree` or :term:`Source Distribution`
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should remove sdist since backends just get a simple directory, which frontends may populate with sdist contents rather than using the source.

Comment on lines +163 to +165
A library, framework, script, plugin, application,
collection of data or other resources, or some combination thereof
that is intended to be packaged into a :term:`Distribution`.
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think applications are necessarily packaged, often just installed in a virtual environment with locked deps.

Copy link
Member

@pradyunsg pradyunsg Jul 26, 2022

Choose a reason for hiding this comment

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

I've definitely seen deployment mechanisms where the application is being packaged like a regular Python package, and shipped with all its dependencies, as a bundle of files. This is how, for example, https://shiv.readthedocs.io/en/latest/ works and pyinstaller works.

Copy link
Contributor

Choose a reason for hiding this comment

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

For sure! My point was just not always

zahlman added a commit to zahlman/bbbb that referenced this pull request Jul 28, 2024
Determined that the order of parameters in the Appendix A example code
was wrong, and fixed it. This matters because `pyproject_hooks` passes
them positionally, despite that both default to `None` and it's not at
all obvious what order they "should" be in.

I commented on python/peps#2536, and cited
pypa/packaging.python.org#1111, to draw
attention to the issue in the PEP. The corresponding documentation
issue, pypa/packaging.python.org#955, still
appears not to be resolved.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bring over PEPs 517, 518, and 660 to the specs section
5 participants