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

Use "modern" Makefile template #1370

Merged
merged 8 commits into from
Aug 15, 2024
Merged

Use "modern" Makefile template #1370

merged 8 commits into from
Aug 15, 2024

Conversation

AA-Turner
Copy link
Member

@AA-Turner AA-Turner commented Aug 14, 2024

Sphinx added this format in 2014. Nevertheless, it is the "modern" format, and far less verbose in the Makefile.

A


📚 Documentation preview 📚: https://cpython-devguide--1370.org.readthedocs.build/

@hugovk
Copy link
Member

hugovk commented Aug 14, 2024

Before, in classic make style, this would only run the script when the include/release-cycle.json dependency had been changed more recently than the include/branches.csv, include/end-of-life.csv and include/release-cycle.svg targets.

Now, it always runs it.

Can we maintain the original behaviour?

@AA-Turner
Copy link
Member Author

In my local testing, it always re-generated, because versions was marked as PHONY. I might have missed something or not got the setup quite right through WSL, though.

I'm out at the moment but will have a look when I get back.

A

@AA-Turner
Copy link
Member Author

AA-Turner commented Aug 14, 2024

This is the current behaviour on my computer (adding a "date" command into the versions target for double-checking), showing that versions is re-run each time:

.../devguide$ make html
venv already exists.
To recreate it, remove it first with `make clean-venv'.
date "+%Y-%m-%d %H:%M:%S"
2024-08-14 17:54:08
Release cycle data generated.
./venv/bin/sphinx-build --builder html --doctree-dir _build/doctrees --jobs auto --fail-on-warning --keep-going . _build/html
[snip]
.../devguide$ make html
venv already exists.
To recreate it, remove it first with `make clean-venv'.
date "+%Y-%m-%d %H:%M:%S"
2024-08-14 17:54:39
Release cycle data generated.
./venv/bin/sphinx-build --builder html --doctree-dir _build/doctrees --jobs auto --fail-on-warning --keep-going . _build/html
[snip]

A

@AA-Turner
Copy link
Member Author

I haven't worked out how to construct a pattern rule that looks for an naked name (no suffix, no directory), so this hopefully serves as a work-around. Feel free to test on a proper UNIX system!

A

@hugovk
Copy link
Member

hugovk commented Aug 14, 2024

On main

Yeah, so make html is calling make versions each time. I'm not too bothered either way about that.

However, versions depends on the CSVs and SVG.

The targets for those depend on the JSON.

The flow is: JSON -> CSVs + SVG

If the JSON is older than the CSVs and SVG, then those ones won't run.

We can test this by putting a "hello" in the Python script:

example diff
diff --git a/Makefile b/Makefile
index 1f8e73e..6aef618 100644
--- a/Makefile
+++ b/Makefile
@@ -210,3 +210,4 @@ include/release-cycle.svg: include/release-cycle.json
 .PHONY: versions
 versions: venv include/branches.csv include/end-of-life.csv include/release-cycle.svg
 	@echo Release cycle data generated.
+	date "+%Y-%m-%d %H:%M:%S"
diff --git a/_tools/generate_release_cycle.py b/_tools/generate_release_cycle.py
index 27b5cc3..1027154 100644
--- a/_tools/generate_release_cycle.py
+++ b/_tools/generate_release_cycle.py
@@ -153,4 +153,5 @@ def main() -> None:
 
 
 if __name__ == "__main__":
+    print("hello")
     main()

Let's make sure the JSON is newer than the CSVs and SVG:

touch include/branches.csv include/end-of-life.csv include/release-cycle.svgtouch include/release-cycle.json

Then run it and we see "hello":

make versions
venv already exists.
To recreate it, remove it first with `make clean-venv'.
date "+%Y-%m-%d %H:%M:%S"
2024-08-14 21:24:40
./venv/bin/python3 _tools/generate_release_cycle.py
hello
Release cycle data generated.
date "+%Y-%m-%d %H:%M:%S"
2024-08-14 21:24:40

As expected, the script ran and updated the CSVs and SVG.

Let's run again. This time the JSON is older than the newly updated CSVs and SVG. There's no "hello"; the script isn't run:

❯ make versions
venv already exists.
To recreate it, remove it first with `make clean-venv'.
Release cycle data generated.
date "+%Y-%m-%d %H:%M:%S"
2024-08-14 21:27:03

@hugovk
Copy link
Member

hugovk commented Aug 14, 2024

On the PR

Adding the print("hello") and running similar commands gives us just a single "hello" so this looks like it's working now :)

touch include/branches.csv include/end-of-life.csv include/release-cycle.svgtouch include/release-cycle.jsonmake html
./venv/bin/python3 _tools/generate_release_cycle.py
hello
Release cycle data generated.
./venv/bin/sphinx-build -M html "." "_build" --jobs auto --fail-on-warning --keep-going -q
matplotlib is not installed, social cards will not be generatedmake html
./venv/bin/sphinx-build -M html "." "_build" --jobs auto --fail-on-warning --keep-going -q
matplotlib is not installed, social cards will not be generated

Will review a bit closer :)

Copy link
Member

@hugovk hugovk left a comment

Choose a reason for hiding this comment

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

On main, we get:

make html
venv already exists.
To recreate it, remove it first with `make clean-venv'.
Release cycle data generated.
./venv/bin/sphinx-build --builder html --doctree-dir _build/doctrees --jobs auto  --fail-on-warning --keep-going . _build/html
Running Sphinx v8.0.2
loading translations [en]... done
matplotlib is not installed, social cards will not be generated
loading pickled environment... done
building [mo]: targets for 0 po files that are out of date
writing output...
building [html]: targets for 0 source files that are out of date
updating environment: 0 added, 0 changed, 0 removed
reading sources...
looking for now-outdated files... none found
no targets are out of date.
Writing redirects...
build succeeded.

The HTML pages are in _build/html.

On the PR we get:

make html
./venv/bin/sphinx-build -M html "." "_build" --jobs auto --fail-on-warning --keep-going -q
matplotlib is not installed, social cards will not be generated

Comparing the called commands, rearranging and adding spaces to aid comparison:

-./venv/bin/sphinx-build --builder html --doctree-dir _build/doctrees --jobs auto --fail-on-warning --keep-going  .   _build/html
+./venv/bin/sphinx-build        -M html                               --jobs auto --fail-on-warning --keep-going "." "_build"    -q

So that removed --doctree-dir _build/doctrees is fine because it's the default.

The output dir change from _build/html to _build doesn't seem to matter, the output files are in _build/html anyway.

What's -M? It seems to be --builder but the help says the short option is -b.

It also adds -q aka --quiet: "no output on stdout, just warnings on stderr".

Do we want to suppress the output? Is there a way to permanently suppress that -q, or temporarily do so for a single run?

Makefile Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
@AA-Turner
Copy link
Member Author

So that removed --doctree-dir _build/doctrees is fine because it's the default.

The output dir change from _build/html to _build doesn't seem to matter, the output files are in _build/html anyway.

What's -M? It seems to be --builder but the help says the short option is -b.

Sphinx's make mode -- fully replicating the old-style (e.g. here) Makefile logic within Sphinx. In effect it just sets doctreedir to <build>/doctrees and builddir to <build>/<builder_name>.

It also adds -q aka --quiet: "no output on stdout, just warnings on stderr".

Do we want to suppress the output? Is there a way to permanently suppress that -q, or temporarily do so for a single run?

This was unintended, sorry (accidental commit of local tests).

Copy link
Member

@hugovk hugovk left a comment

Choose a reason for hiding this comment

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

Nice cleanup, thanks!

I've not reviewed make.bat or make.ps1.

@AA-Turner AA-Turner merged commit cedcb49 into python:main Aug 15, 2024
4 checks passed
@AA-Turner AA-Turner deleted the make-mode branch August 15, 2024 13:07
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