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 778: Supporting Symlinks in Wheels #3786

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

ethanhs
Copy link
Contributor

@ethanhs ethanhs commented May 20, 2024

Posting here to reserve the PEP numbers and posting the text of PEP 778 to share on discuss.

Basic requirements (all PEP Types)

  • Read and followed PEP 1 & PEP 12
  • File created from the latest PEP template
  • PEP has next available number, & set in filename (pep-NNNN.rst), PR title (PEP 123: <Title of PEP>) and PEP header
  • Title clearly, accurately and concisely describes the content in 79 characters or less
  • Core dev/PEP editor listed as Author or Sponsor, and formally confirmed their approval
  • Author, Status (Draft), Type and Created headers filled out correctly
  • PEP-Delegate, Topic, Requires and Replaces headers completed if appropriate
  • Required sections included
    • Abstract (first section)
    • Copyright (last section; exact wording from template required)
  • Code is well-formatted (PEP 7/PEP 8) and is in code blocks, with the right lexer names if non-Python
  • PEP builds with no warnings, pre-commit checks pass and content displays as intended in the rendered HTML
  • Authors/sponsor added to .github/CODEOWNERS for the PEP

Standards Track requirements

  • PEP topic discussed in a suitable venue with general agreement that a PEP is appropriate
  • Suggested sections included (unless not applicable)
    • Motivation
    • Rationale
    • Specification
    • Backwards Compatibility
    • How to Teach This
    • Reference Implementation
    • Rejected Ideas
    • Open Issues
  • Right before or after initial merging, PEP discussion thread created and linked to in Discussions-To and Post-History

📚 Documentation preview 📚: https://pep-previews--3786.org.readthedocs.build/pep-0778/

@ethanhs ethanhs requested a review from a team as a code owner May 20, 2024 20:51
@hugovk
Copy link
Member

hugovk commented May 21, 2024

Hi Ethan! We normally assign the next available PEP numbers (for example, 747), what's the justification for jumping up to 777?

@warsaw Please can you confirm you're sponsoring the second PEP here ("Supporting Symlinks in Wheels")?

And we need a sponsor for the first one ("Reinventing the Wheel (Wheel 2.0)" - excellent title btw!). @warsaw Will you sponsor this one as well?

And for next time, we normally don't open a PEP discussion thread (https://discuss.python.org/t/pep-778-supporting-symlinks-in-wheels/53824) until after the draft PEP has been merged into the repo - it means a first pass review and fixes can be made, but more importantly, means we have a stable PR for people to review. We've had situations where people were still looking at the old PR preview even after merge and subsequent PR updates.

@ethanhs
Copy link
Contributor Author

ethanhs commented May 21, 2024

Hey Hugo! Sorry! I selected 777 because I expect a series of related PEPs (wheel 2.0) and I wanted to have their numbers grouped together (like 482, 483, 484), if someone puts a new PEP up between now and when I get to writing the other PEPs, it would break the grouping. I posted to discuss to get feedback on 778 while I work on 777.

Sorry for not following the process I'll try to follow it more closely in future.

@warsaw
Copy link
Member

warsaw commented May 21, 2024

Please can you confirm you're sponsoring the second PEP here ("Supporting Symlinks in Wheels")?

Confirmed!

@warsaw
Copy link
Member

warsaw commented May 21, 2024

@warsaw Will you sponsor this one as well?

Just to be explicit, yes I'll sponsor this one as well.

@hugovk hugovk added the new-pep A new draft PEP submitted for initial review label May 21, 2024
Copy link
Member

@hugovk hugovk left a comment

Choose a reason for hiding this comment

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

Please could you copy the "New PEP" template into the PR message and check them off?

https://github.com/python/peps/blob/main/.github/PULL_REQUEST_TEMPLATE/Add%20a%20new%20PEP.md

For example, please also add Barry to the CODEOWNERS file for these.

peps/pep-0777.rst Outdated Show resolved Hide resolved
peps/pep-0778.rst Show resolved Hide resolved
peps/pep-0778.rst Outdated Show resolved Hide resolved
peps/pep-0778.rst Outdated Show resolved Hide resolved
symlinks are unsupported on the platform, and explain why installation has failed.

For people building libraries, documentation on ``packaging.python.org`` should describe the use
cases and caveats (especially platform suppport) of symlinks in wheels. Otherwise it should be
Copy link
Member

@hugovk hugovk May 21, 2024

Choose a reason for hiding this comment

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

Suggested change
cases and caveats (especially platform suppport) of symlinks in wheels. Otherwise it should be
cases and caveats (especially platform support) of symlinks in wheels. Otherwise it should be

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Was there a change you wanted to suggest here? I don't see any diff.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure what happened there! Edited to remove the third P in "suppport".

peps/pep-0778.rst Outdated Show resolved Hide resolved
peps/pep-0777.rst Outdated Show resolved Hide resolved
ethanhs and others added 2 commits May 22, 2024 09:00
Co-authored-by: Hugo van Kemenade <[email protected]>
Co-authored-by: Hugo van Kemenade <[email protected]>
@geofft
Copy link
Contributor

geofft commented May 22, 2024

EDIT: this was indeed raised in the Discourse thread already.

For symlinks - have you looked at the approach @njsmith took for pybi and the rationale for it? https://github.com/njsmith/posy/blob/main/pybi/README.md#symlinks

They are stored both as actual symlinks in the zip file with one of the common extensions for representing them, and as metadata in the RECORD file. This means that if you're just using a naive zip tool (e.g. unzip xyz.whl) on a system that supports symlinks, you'll get the right content, but if you're on a platform that doesn't directly support symlinks, it can ignore the symlink entries in the zip file and look at the RECORD file alone.

In general I think it would be good to unify wheel 2.0 with pybi as far as possible / make it so that the pybi format doesn't need to exist and it's just wheel 2.0.

(BTW - are you all still at sprints?)

(Also should I be commenting here or on the Discourse thread?)

@hugovk
Copy link
Member

hugovk commented May 22, 2024

(Also should I be commenting here or on the Discourse thread?)

Discourse for more general discussion like this, thanks.

@ethanhs-nv ethanhs-nv mentioned this pull request Oct 9, 2024
21 tasks
ethanhs and others added 4 commits October 9, 2024 12:20
Co-authored-by: Hugo van Kemenade <[email protected]>
Co-authored-by: Hugo van Kemenade <[email protected]>
Co-authored-by: Hugo van Kemenade <[email protected]>
The PEP has been split out into python#4036
@ethanhs ethanhs changed the title PEP 778: Add initial text and reserve 777 PEP 778: Supporting Symlinks in Wheels Oct 9, 2024
@ethanhs
Copy link
Contributor Author

ethanhs commented Oct 9, 2024

Ah, this won't build cleanly until 777 is merged in. I'll make sure to update the MR after that so it builds correctly. Also, I've added the checklist template to the original message.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new-pep A new draft PEP submitted for initial review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants