-
Notifications
You must be signed in to change notification settings - Fork 933
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
base: main
Are you sure you want to change the base?
Conversation
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). |
Great! Thank you. I have started to read this. |
Distribution | ||
Package |
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.
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.
@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. :) |
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.
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). |
A tool directly responsible for transforming a | ||
:term:`Source Tree` or :term:`Source Distribution` |
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 think we should remove sdist since backends just get a simple directory, which frontends may populate with sdist contents rather than using the source.
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`. |
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 don't think applications are necessarily packaged, often just installed in a virtual environment with locked deps.
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'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.
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.
For sure! My point was just not always
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.
@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):
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 thepyproject.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).[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 therequires
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 developersI've made some relatively modest adjustments to the outline originally proposed in #955:
Final outline w/ mapping from existing PEP sections (click to expand)
pyproject.toml
configuration filebuild-system
table (high-level statement of purpose from the PEP 518 section; links to Specifying a project's build system spec for the actual details)project
table (very brief statement of purpose, which links to the Declaring project [source] metadata spec)tool
table (PEP 518 section)requires
key (PEP 518 build-system section and PEP 517 Build Requirements sectionbuild-backend
key (PEP 517 Source Trees section [which only contains ≈1 sentence about source trees])backend-path
key (PEP 517 In-tree Build Backends section)build_wheel
(mandatory) (PEP 517 section)get_requires_for_build_wheel
(PEP 517 section)prepare_metadata_for_build_wheel
(PEP 517 section)build_sdist
(mandatory) (PEP 517 section)get_requires_for_build_sdist
(PEP 517 section)build_editable
(PEP 660 section)get_requires_for_build_editable
(PEP 660 section)prepare_metadata_for_build_wheel
(PEP 660 section)After a fair amount of thought and experimentation, I've tweaked the current
Package Distribution Formats
heading to readBuilding 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