Skip to content

Conversation

@bibajz
Copy link
Contributor

@bibajzbibajz commented Aug 4, 2024

Description

define - https://www.gnu.org/software/make/manual/html_node/Multi_002dLine.html - is a GNU make extension not generally supported by other makes.

Changes

Introduce UV variable to accomodate for the case when uv is not in PATH.

Remove the

$ python -m $(1) --version >/dev/null

commands that can prevent the installation of a package when it's already present.

The rationale is this approach is really fragile, for multiple reasons.

Firstly, it works on packages defining __main__.py module, there- fore as an example, anyio==4.4.0 package, even if installed, would output:

$ /tmp/venv/bin/python -m anyio >/dev/null /tmp/venv/bin/python: No module named anyio.__main__;'anyio' is a package and cannot be directly executed $ echo$? 1

Secondly, unlike package installation, which is insensitive to capitalization as well as -/_ separators - https://packaging.python.org/en/latest/specifications/name-normalization/#names-and-normalization - module names are sensitive to naming, meaning this may happen (and indeed happened in bc37ac7)

$ /tmp/venv/bin/python -m sphinx-autobuild --help >/dev/null /tmp/venv/bin/python: No module named sphinx-autobuild $ echo$? 1 $ /tmp/venv/bin/python -m sphinx_autobuild --help >/dev/null $ echo$? 0

And lastly, name of the package may not be the same as its contents that are subsequently imported. Consider PyYAML==6.0.1, whose contents are importable from the yaml namespace - https://pyyaml.org/wiki/PyYAMLDocumentation

$ /tmp/venv/bin/python -m pyyaml >/dev/null /tmp/venv/bin/python: No module named pyyaml $ /tmp/venv/bin/python -m yaml >/dev/null /tmp/venv/bin/python: No module named yaml.__main__;'yaml' is a package and cannot be directly executed

Further work

The whole Makefile does not work properly when not using GNU make. Consider the following target:

.PHONY: html html: BUILDER = html html: build @echo "Build finished. The HTML pages are in build/html." 

the way BUILDER variable is set, in a prerequisite, does not work in other makes - variable is empty when defined this way. This will probably need another issue and pull request.


📚 Documentation preview 📚: https://cpython-previews--122662.org.readthedocs.build/

`define` - https://www.gnu.org/software/make/manual/html_node/Multi_002dLine.html - is a GNU make extension not generally supported by other makes. Introduce `UV` variable to accomodate for the case when `uv` is not in `PATH`. Remove the ```bash $ python -m $(1) --version >/dev/null ``` commands that can prevent the installation of a package when it's already present. The rationale is this approach is really fragile, for multiple reasons. Firstly, it works on packages defining `__main__.py` module, there- fore as an example, `anyio==4.4.0` package, even if installed, would output: ```bash $ /tmp/venv/bin/python -m anyio >/dev/null /tmp/venv/bin/python: No module named anyio.__main__; 'anyio' is a package and cannot be directly executed $ echo $? 1 ``` Secondly, unlike package installation, which is insensitive to capitalization as well as `-`/`_` separators - https://packaging.python.org/en/latest/specifications/name-normalization/#names-and-normalization - module names are sensitive to naming, meaning this may happen (and indeed happened in bc37ac7) ```bash $ /tmp/venv/bin/python -m sphinx-autobuild --help >/dev/null /tmp/venv/bin/python: No module named sphinx-autobuild $ echo $? 1 $ /tmp/venv/bin/python -m sphinx_autobuild --help >/dev/null $ echo $? 0 ``` And lastly, name of the package may not be the same as its contents that are subsequently imported. Consider `PyYAML==6.0.1`, whose contents are importable from the `yaml` namespace - https://pyyaml.org/wiki/PyYAMLDocumentation ```bash $ /tmp/venv/bin/python -m pyyaml >/dev/null /tmp/venv/bin/python: No module named pyyaml $ /tmp/venv/bin/python -m yaml >/dev/null /tmp/venv/bin/python: No module named yaml.__main__; 'yaml' is a package and cannot be directly executed ```
@ghost
Copy link

ghost commented Aug 4, 2024

All commit authors signed the Contributor License Agreement.
CLA signed

@bedevere-app
Copy link

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.


.PHONY: ensure-sphinx-autobuild
ensure-sphinx-autobuild: venv
$(call ensure_package,sphinx-autobuild)
Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

As alluded in the PR description, this code contains a bug, because the python -m sphinx-autobuild always fails:

$ /tmp/venv/bin/python -m sphinx-autobuild --help >/dev/null /tmp/venv/bin/python: No module named sphinx-autobuild $ echo $? 1 $ /tmp/venv/bin/python -m sphinx_autobuild --help >/dev/null $ echo $? 0 

@hugovk

@picnixzpicnixz added skip news build The build process and cross-build labels Aug 4, 2024
@hugovkhugovk added needs backport to 3.12 only security fixes needs backport to 3.13 bugs and security fixes labels Aug 4, 2024
Copy link
Member

@hugovkhugovk left a comment

Choose a reason for hiding this comment

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

Just to let you know, there have been discussions about requiring GNU make, but no decisions have been made yet:

https://discuss.python.org/t/make-compatibility-requirements/26869

Anyway, these changes are fine and thanks for the fixes!

@hugovkhugovk merged commit f5c39b3 into python:mainAug 4, 2024
@miss-islington-app
Copy link

Thanks @bibajz for the PR, and @hugovk for merging it 🌮🎉.. I'm working now to backport this PR to: 3.12, 3.13.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Aug 4, 2024
…pythonGH-122662) (cherry picked from commit f5c39b3) Co-authored-by: Libor Martínek <libas@acco.cz>
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Aug 4, 2024
…pythonGH-122662) (cherry picked from commit f5c39b3) Co-authored-by: Libor Martínek <libas@acco.cz>
@bedevere-app
Copy link

GH-122668 is a backport of this pull request to the 3.13 branch.

@bedevere-appbedevere-appbot removed the needs backport to 3.13 bugs and security fixes label Aug 4, 2024
@bedevere-app
Copy link

GH-122669 is a backport of this pull request to the 3.12 branch.

@bibajz
Copy link
ContributorAuthor

Just to let you know, there have been discussions about requiring GNU make, but no decisions have been made yet:

https://discuss.python.org/t/make-compatibility-requirements/26869

Anyway, these changes are fine and thanks for the fixes!

Thank you for the pointer to the forum thread.

As I pointed out in the Further work section, the whole Doc/Makefile implicitly assumes GNU make - the syntax of variable setting as a prerequisite is GNU make-specific. Otherwise, my changes could be

.PHONY: _ensure-sphinx-autobuild _ensure-sphinx-autobuild: PACKAGE=sphinx-autobuild _ensure-sphinx-autobuild: _ensure-package 

instead of

.PHONY: _ensure-sphinx-autobuild _ensure-sphinx-autobuild: make _ensure-package PACKAGE=sphinx-autobuild 

which would be IMO nicer, since you can see the prerequisite of _ensure-sphinx-autobuild right away.


Personally, I do not have a horse in this race, be it GNU make or BSD make, but it would be nice to reach an explicit resolution which one is actually necessary to build stuff correctly.

Running the targets on FreeBSD with gmake worked even before my patch.


One last thing - I wanted to ask about commit messages.

In my original commit, I wrote a really detailed message what changes (and why) are introduced - 06b20b5 .

However, on main, there is only the merge commit - f5c39b3 - with links to GitHub issue and pull request.

Why can't the commit messages be included as well? This forces CPython to rely on out-of-band information, namely a git provider, which may or may not be present in years to come...


Thank you for your time and answers! (If anyone is more knowledgeable about the commit messages being off-loaded, please, tag them here, I find commit messages a treasure trove of information and oftentimes joy to read 😉 )

@hugovk

@hugovk
Copy link
Member

I think the commit message body was cleared by the Refined GitHub browser extension I have installed. If there are many commits, the default would be to list all the commit titles only, which is less useful. I can see it can be less useful when there's a single commit with a detailed body.

@faulhaberben
Copy link
Contributor

faulhaberben commented Aug 4, 2024

Sorry, I have a side question here: Will you close the issue manually @bibajz or is that taken care of by a bot? @hugovk

#122661 (comment)

@hugovk
Copy link
Member

It needs to be done manually, I'll do it. Thanks for the reminder!

hugovk pushed a commit that referenced this pull request Aug 9, 2024
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

buildThe build process and cross-buildskip news

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@bibajz@hugovk@faulhaberben@picnixz