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

RFC: Clarify that the direct_url.json url field must be a spec-compliant url #1506

Merged
merged 7 commits into from
Mar 4, 2024

Conversation

konstin
Copy link
Contributor

@konstin konstin commented Feb 21, 2024

Clarify that the url field in direct_url.json must be a valid url. This was prompted by a case where the url field contained a value that was not a url (astral-sh/uv#1744). I'm not sure what the best venue to discuss this change is, so i'm putting it up as a PR here.

Relatedly, would it make sense to write a json schem for direct_url.json? A machine readable spec would allow tooling to generate types and would simplify verification.

Post on discuss.python.org

Closes #1510 (link check fix)


Thanks to whoever set up nox + sphinx-autobuild, nox -s preview is great!


📚 Documentation preview 📚: https://python-packaging-user-guide--1506.org.readthedocs.build/en/1506/

@sbidoul
Copy link
Member

sbidoul commented Feb 21, 2024

@konstin thanks for this!

Could you also announce this PR on discuss.python.org/packaging. Since this could be considered a spec change, it is worth announcing broadly (I personally don't think this change requires a PEP, but better check).

@konstin
Copy link
Contributor Author

konstin commented Feb 21, 2024

Could you also announce this PR on discuss.python.org/packaging. Since this could be considered a spec change, it is worth announcing broadly (I personally don't think this change requires a PEP, but better check).

Sure: https://discuss.python.org/t/clarify-that-the-direct-url-json-url-field-must-be-a-spec-compliant-url/46518

sbidoul

This comment was marked as resolved.

@radoering
Copy link

I wonder if this is too restricted regarding to git urls. If I understand correctly, you can't just convert a scp-like url to a valid url, cf servo/rust-url#220 and linked issues for example. If you can't convert it what should an installer that supports scp-like urls write into direct_url.json? Or does this change mean that installers should not support scp-like urls?

@konstin
Copy link
Contributor Author

konstin commented Feb 23, 2024

Wouldn't prefixing them with ssh:// make them valid urls?

@radoering
Copy link

radoering commented Feb 24, 2024

I assume for github they are equivalent, but if this statement is correct they are not always equivalent:

However, the user@host and ssh:// url forms are not equivalent--in the user@host form, the path is usually relative to the user's homedir, while in the ssh:// form, the path is relative to root. (This is the behaviour when pulling over a vanilla ssh+shell setup--obviously hosting tools can make their own choices.)

(from https://secure.phabricator.com/T11004)

I have no idea how common that is. Maybe, that's just an edge case that can be neglected?

@radoering
Copy link

FYI: We decided to do the normalization in Poetry for now even if it might not always be equivalent in theory.

@pradyunsg

This comment was marked as resolved.

@konstin

This comment was marked as outdated.

@chrysle
Copy link
Contributor

chrysle commented Mar 2, 2024

@konstin Could you rebase?

Copy link
Contributor

@chrysle chrysle left a comment

Choose a reason for hiding this comment

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

By the way, this document seems to fix the order of fields in the data structure, but I think that is misleading? At least I have seen valid direct_url.json files where it has been swapped.

source/specifications/direct-url-data-structure.rst Outdated Show resolved Hide resolved
source/specifications/direct-url-data-structure.rst Outdated Show resolved Hide resolved
konstin and others added 2 commits March 2, 2024 16:18
Co-authored-by: chrysle <fritzihab@posteo.de>
Co-authored-by: chrysle <fritzihab@posteo.de>
@chrysle chrysle added this pull request to the merge queue Mar 4, 2024
Merged via the queue into pypa:main with commit f6c0291 Mar 4, 2024
5 checks passed
@chrysle
Copy link
Contributor

chrysle commented Mar 4, 2024

Thank you!

@konstin konstin deleted the url-must-be-a-url branch March 4, 2024 14:38
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.

5 participants