Skip to content

PEP 582: Tweek sysconfig and path details, and add some minor clarifications#3036

Closed
FFY00 wants to merge 6 commits into
python:mainfrom
FFY00:582-sysconfig-minor-tweaks
Closed

PEP 582: Tweek sysconfig and path details, and add some minor clarifications#3036
FFY00 wants to merge 6 commits into
python:mainfrom
FFY00:582-sysconfig-minor-tweaks

Conversation

@FFY00
Copy link
Copy Markdown
Member

@FFY00 FFY00 commented Mar 3, 2023

@kushaldas I have a couple changes here, mostly related to sysconfig. Could you have a look?

Changes:

  • Rename the sysconfig scheme from localpackages to local_packages
    • It now follows the naming scheme, where words are separated by an underscore
  • Clarify that the scheme local_packages is only required to provide the purelib and platlib paths
    • I still plan to provide the other paths, to keep backwards compatibility, if we don't have a new sysconfig by the time PEP 582 is released
  • Remove __pypackages__ directory from the base/platbase definitions
    • IMO it should be part of the scheme, eg.
      ...
      'local_packages_posix': {
          'stdlib': '{installed_base}/{platlibdir}/python{py_version_short}',
          'platstdlib': '{installed_platbase}/{platlibdir}/python{py_version_short}',
          'include': '{installed_base}/include/python{py_version_short}{abiflags}',
          'platinclude': '{installed_platbase}/include/python{py_version_short}{abiflags}',
          'purelib': '{base}/__pypackages__/{py_version_short}/site-packages',
          'platlib': '{platbase}/__pypackages__/{py_version_short}/site-packages',
          'scripts': '{base}/__pypackages__/bin',
          'data': '{base}/__pypackages__/data',
      },
      ...
      
  • Change the layout to __pypackages__/3.10/site-packages
    • There's no need for all that clutter in the path here, we are not installing on the system, we are installing language-specific directory
  • Clarify that the subdirectory paths on Windows do not need to be different ("will be different" > "can be different")
  • Specify that we will expand the local packages paths to an absolute path before adding them to sys.path
    • If we let them be relative, os.chdir will change the location of the __pypackages__, bypassing the mechanisms described in the security considerations section
  • Clarify why we are offering an alternative option for the sysconfig API
    • This gives a bit more context to the reader, which IMO makes it a bit easier to follow
  • Update link of the sysconfig API proposal to Kushal's fork, where it was moved to
  • Clarify that when we say that the __pypackages__ in the current directory will be ignored when running a script, that we mean when running a script from a different path
    • Running a script in the current path will still pick up __pypackages__ from the current directory

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

FFY00 added 6 commits March 2, 2023 23:54
Signed-off-by: Filipe Laíns <lains@riseup.net>
Signed-off-by: Filipe Laíns <lains@riseup.net>
Signed-off-by: Filipe Laíns <lains@riseup.net>
Signed-off-by: Filipe Laíns <lains@riseup.net>
Signed-off-by: Filipe Laíns <lains@riseup.net>
Signed-off-by: Filipe Laíns <lains@riseup.net>
Copy link
Copy Markdown
Member

@CAM-Gerlach CAM-Gerlach left a comment

Choose a reason for hiding this comment

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

A handful of editorial comments

Comment thread pep-0582.rst
Specifically, both of the ``purelib`` and ``platlib`` directories must be
present, using the following code to determine the locations of those
directories::
according to a new ``local_packages`` scheme in the sysconfig module. The scheme
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

While you're at it, would be good to either link sysconfig to the canonical current docs:

Suggested change
according to a new ``local_packages`` scheme in the sysconfig module. The scheme
according to a new ``local_packages`` scheme in the :mod:`sysconfig` module. The scheme

or at least mark it as a literal, like you do elsewhere:

Suggested change
according to a new ``local_packages`` scheme in the sysconfig module. The scheme
according to a new ``local_packages`` scheme in the ``sysconfig`` module. The scheme

Comment thread pep-0582.rst

.. note:: There is a possible option of having a separate new API, it is documented at `issue #3013 <https://github.com/python/peps/issues/3013>`_.

An absolute version of these two locations will be added to ``sys.path``, other
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
An absolute version of these two locations will be added to ``sys.path``, other
An absolute version of these two locations will be added to ``sys.path``; other

Fix grammar error (comma splice)

Comment thread pep-0582.rst

An absolute version of these two locations will be added to ``sys.path``, other
directories or files in the ``__pypackages__`` directory will be silently
ignored. The paths will be based on Python versions.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Maybe I'm just dumb, but what does "based on Python versions" mean here, specifically? This seems rather vague for a specification.

Comment thread pep-0582.rst
.. note::

There is a possible option of having a separate new API, to avoid
`sysconfig` having to provide other scheme paths. It is documented in
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
`sysconfig` having to provide other scheme paths. It is documented in
``sysconfig`` having to provide other scheme paths. It is documented in

Fix reST syntax mistake (that sphinx-lint would catch, but that check wasn't enabled for some reason).

Comment thread pep-0582.rst
The following shows an example project directory structure, and different ways
the Python executable and any script will behave. The example is for Unix-like
systems - on Windows the subdirectories will be different.
systems - on Windows the subdirectories can be different.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
systems - on Windows the subdirectories can be different.
systems --- on Windows the subdirectories can be different.

Minor typographical nit, but should be an em dash rather than a hyphen here

Comment thread pep-0582.rst
systems - on Windows the subdirectories will be different.
systems - on Windows the subdirectories can be different.

::
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
::
.. code-block:: text

Add the appropriate lexer name for non-Python code (or should this be treated as pseudo-shell?

@FFY00
Copy link
Copy Markdown
Member Author

FFY00 commented Mar 7, 2023

Thanks for the review CAM! I just kept the current formatting, but am happy to go over the whole document and fix this everywhere if Kushal doesn't have any issue with that.

@CAM-Gerlach
Copy link
Copy Markdown
Member

CAM-Gerlach commented Mar 8, 2023

Just to make sure we're on the same page, the first review comment (the formatting suggestion) was really just a tip/hint to let you know what was possible if you or @kushaldas want rather than requesting a specific change—that's totally up to you, of course, and you shouldn't feel pressured to go back and change it everywhere unless @kushaldas wants. The rest all fix syntax problems, grammar errors, and other minor issues as well as pointing out one added part I found unclear.

As a standard reminder, N.B., the other suggestions can be batch-applied with Files changed -> Add to batch -> Commit. Thanks!

@hugovk
Copy link
Copy Markdown
Member

hugovk commented Sep 20, 2023

@FFY00 I don't have permission to push to this branch, but you can try this to resolve the conflict:

git checkout 582-sysconfig-minor-tweaks
git fetch upstream
git merge upstream/main
git push

@ambv
Copy link
Copy Markdown
Contributor

ambv commented Oct 11, 2023

Since the PEP has been rejected since, and Filipe is busy, I'm closing this.

@ambv ambv closed this Oct 11, 2023
@FFY00
Copy link
Copy Markdown
Member Author

FFY00 commented Oct 11, 2023

Thanks. I should have done it earlier when the PEP was rejected.

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