PEP 796: Relative Virtual Environments #4476
Conversation
|
Hello, before we go any further with this PR, has the idea been discussed on Discourse? And after that we'll need a sponsor before assigning the PEP number, do you have one yet? Let's unassign 796 for now. Please see: |
hugovk
changed the title
|
Thanks @hugovk and @StanFromIreland for the early review. I used a draft PR to see the CI results to further clean it up -- my apologies for wasting some of your time, but thank you regardless.
Yes: https://discuss.python.org/t/making-venvs-relocatable-friendly/96177 The criteria of "discussed enough with enough support" is vague, but what gave me enough confidence to begin a PEP and start a (draft) PR at this point was:
Thanks for clarifying that part! Yes, finding a sponsor is my next big step. |
Tip: you can enable GitHub Actions at https://github.com/rickeylev/peps/actions and run the CI on your fork.
Good luck! |
This comment has been minimized.
This comment has been minimized.
|
@paveldikov thank you for the comment, please could you re-post it on Discourse? The peps repo / PR discussion is mainly for editorial discussion, rather than substantive comment on the proposal itself. A |
Co-authored-by: Hugo van Kemenade <[email protected]> Co-authored-by: Zanie Blue <[email protected]>
…to relative.venvs
|
Belatedly removed the DO-NOT-MERGE label (that was added pending the Discourse discussion and sponsorship of the PEP) |
There was a problem hiding this comment.
The PEP is currently missing a Security Implications section, which I think should be added given that arbitrary directory traversal is permitted -- at the very least explaining why this is fine.
Several editorial notes, I think the Motivation & Rationale sections should be strengthened to focus on the benefits from relative environments, there is currently (I believe) a lot of assumed context.
The PEP also discusses at some length a broader proposal for reloacatable venvs. Is it worth considering making that the proposal here? I don't know the specifics, so it might be that the changes needed for 'relocatable' are too large to tackle in one go.
A
AA-Turner
changed the title
Co-authored-by: Alyssa Coghlan <[email protected]> Co-authored-by: Adam Turner <[email protected]> Co-authored-by: Stan Ulbrych <[email protected]>
There was a problem hiding this comment.
Ok, all pending comments addressed (I think. The Github UI frequently tricks me 😂 )
PTAL
re: security section: added. At least, insofar as I understand your concern correctly.
re: motivation and rationale strengthening: Thanks! Yeah, honestly, a big issue I've had writing this that "Motivation" and "Rationale" are, to me at least, practically synonyms. I did my best to better separate "Why do this change at all?" (motivation) and "Implementation decision and why that decision" (rationale).
re: proposing relocatable venv in this pep: I added a section about why this pep limits its scope to just the core interpreter.
| During interpreter startup (i.e. :file:`getpath.py`), the relative path is joined to the | ||
| directory containing ``pyvenv.cfg`` to form an absolute path. | ||
| Parent-directory references (``../``) and current | ||
| directory references (``./``) are resolved syntactically (i.e. not resolving |
There was a problem hiding this comment.
| directory references (``./``) are resolved syntactically (i.e. not resolving | |
| directory references (``./``) are resolved lexically (i.e. not resolving |
(I don't mind if this isn't accepted, I've just always referred to this style of resolution as lexical rather than as syntactic: python/cpython#124825
|
|
||
| This PEP does not specify how to create a ``pyvenv.cfg`` with a relative path, | ||
| nor how downstream tools (e.g. installers) should identify them or process | ||
| them. These questions are best addressed separately by tool owners. |
There was a problem hiding this comment.
This isn't an open issue, but rather than intentional omission. Perhaps just include a variant of this paragraph in the rationale subsection about not fully specifying portable virtual environments?
hugovk
changed the title
peps/pep-0796.rst
Outdated
| machines do so either by relying on undocumented interpreter behaviour | ||
| (Bazel, omitting the ``home`` key entirely to trigger an implementation | ||
| dependent fallback to resolving via a symlinked interpreter binary on | ||
| non-Windows systems, see `gh-135773`) or by requiring a post-installation script to be executed |
There was a problem hiding this comment.
Lint fix:
Alyssa's suggestion below is better.
| How to Teach This | ||
| ================= | ||
|
|
||
| Teaching this should be straightforward: if you use a relative path in | ||
| ``pyvenv.cfg``, then it's relative to the directory containing the | ||
| ``pyvenv.cfg`` file. This is simple to explain and easy to understand for | ||
| anyone that is already familiar with handling relative filesystem paths. |
There was a problem hiding this comment.
It might be useful to say where you will teach this. What docs will you update?
peps/pep-0796.rst
Outdated
| paths. | ||
|
|
||
| A proof-of-concept of this is implemented in the author's branch, | ||
| `rickeylev/feat.relative.pyvenv.home <https://github.com/python/cpython/compare/main...rickeylev:cpython:feat.relative.pyvenv.home>`__. |
There was a problem hiding this comment.
| `rickeylev/feat.relative.pyvenv.home <https://github.com/python/cpython/compare/main...rickeylev:cpython:feat.relative.pyvenv.home>`__. | |
| `rickeylev/feat.relative.pyvenv.home | |
| <https://github.com/python/cpython/compare/main...rickeylev:cpython:feat.relative.pyvenv.home>`__. |
Please could you also wrap some of the other long lines to 79 chars?
Co-authored-by: Hugo van Kemenade <[email protected]>
|
@rickeylev I went ahead and applied the straightforward markup & grammar fixes in ae5a67f, so the remaining comments are the ones that aren't as simple to address. @AA-Turner This will need a re-review from you at some point for the "Changes requested" flag. |
Basic requirements (all PEP Types)
pep-NNNN.rst), PR title (PEP 123: <Title of PEP>) andPEPheaderAuthororSponsor, and formally confirmed their approvalAuthor,Status(Draft),TypeandCreatedheaders filled out correctlyPEP-Delegate,Topic,RequiresandReplacesheaders completed if appropriate.github/CODEOWNERSfor the PEPStandards Track requirements
Python-Versionset to valid (pre-beta) future Python version, if relevantDiscussions-ToandPost-HistoryWork towards python/cpython#136051
📚 Documentation preview 📚: https://pep-previews--4476.org.readthedocs.build/