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

Fix PEP-723 script metadata parsing. #2722

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

Conversation

jsirois
Copy link
Member

@jsirois jsirois commented Mar 19, 2025

This pre-emptively fixes the issue reported here:
astral-sh/uv#12303

This pre-emptively fixes the issue reported here:
  astral-sh/uv#12303
In these cases degrade to a naive parse and assume all matching lines
are comment lines.
@jsirois jsirois requested a review from benjyw March 19, 2025 13:30
@jsirois
Copy link
Member Author

jsirois commented Mar 19, 2025

Apparently @ofek thinks this is bad form, but as a tool maintainer that generally has to work around PEPs that are too loose leading to crazy situations in the wild, I think this is just par for the course. I'd rather Pex not choke and ruin a user's day than tell them they're doing it wrong. Of course I think the underlying issue is, yet again, the PyPA. It should not be a forked entity from Python. If PEP-723 were implemented in Python by Python and shipped with Python, it could unambiguously parse this data as part of its normal bytecode parsing phase.

@ofek
Copy link
Contributor

ofek commented Mar 19, 2025

Of course I think the underlying issue is, yet again, the PyPA.

I'm not sure why you always mention that the issue at the end of the day is the PyPA; wouldn't the issue literally be the spec? I don't see how any other conclusion can be made 😅

If PEP-723 were implemented in Python by Python and shipped with Python, it could unambiguously parse this data as part of its normal bytecode parsing phase.

No, that's a bad idea and was rejected: https://peps.python.org/pep-0723/#why-not-use-possibly-restricted-python-syntax

Python isn't a closed ecosystem and many kinds of tools from all kinds of languages need to be able to parse the block.

@jsirois
Copy link
Member Author

jsirois commented Mar 19, 2025

I'm not sure why you always mention that the issue at the end of the day is the PyPA; wouldn't the issue literally be the spec? I don't see how any other conclusion can be made

My point is there is no spec involving Python source code that can avoid ambiguity without parsing the Python source code. As such, if there were instead no spec and Python shipped with its own single set of tooling, then this would all be avoided. Non-python ecosystem tooling could either shell out to python tooling - easy to do if there is 1 and only 1 tooling set - or else bite the bullet and parse the source code too. The PyPA and CPython though seem to consistently reject the idea the very split in the two groups is a deep issue.

The Python ecosystem keeps having to pay for the division between Python and its tooling. The JS ecosystem has tread this road as well. Perhaps some folks are satisfied with that? I only have had exposure to the opposite.

@ofek
Copy link
Contributor

ofek commented Mar 19, 2025

Non-python ecosystem tooling could either shell out to python tooling - easy to do if there is 1 and only 1 tooling set - or else bite the bullet and parse the source code too.

I view that as completely unreasonable for all of those other non-Python projects, so I guess we just have to agree to disagree here about the burden of work we are comfortable with imposing on others.

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.

2 participants