Make build configuration compatible with PEP621 #3220

lioman · 2023-10-28T08:47:15Z

Pull Request Checklist

Resolves: #3203

This solves the issue by using pdm as a build tool to resolve dependencies and adding pyproject.toml with PEP621 compliant configuration.

Todo

adapt documentation to read pyproject.toml
check if files to build are added (transfer Manifest.in config)
double check entry points. etc from setup.py
remove setup.py

Ensured tests pass and (if applicable) updated functional test output
Conformed to code style guidelines by running appropriate linting tools
Added tests for changed code
Updated documentation for changed code

justinmayer · 2023-10-28T08:56:02Z

In the interests of keeping PR scope as small as possible, I may cherry-pick c48f5f1 and apply it before reviewing the rest of this PR.

.pdm-python

justinmayer

Many thanks for working on this, @lioman! In addition to the in-line comments, I have two additional thoughts:

I see you have chosen to use PDM for the build backend. I have only using Hatchling in combination with PDM, and so I have no experience with PDM's build backend. But I imagine it works just fine and has the ostensible advantage of not needing an additional dependency (i.e., Hatchling). So I am down to try it and see how it goes.
I have a few mild preferences regarding the order of items in pyproject.toml, so I will add a commit with some proposed changes. Those changes may also include a couple of the suggested changes I made in my in-line comments.

.pdm-python

pdm.lock

pyproject.toml

justinmayer · 2023-10-28T10:22:19Z

Also, the c48f5f1 commit should be dropped from this PR, as I already added it to master. Then, this PR should be rebased on current master, which should fix the failing test currently shown here.

pyproject.toml

pdm.lock

justinmayer · 2023-10-28T21:35:42Z

Looks like c48f5f1 is still present. That commit should be dropped from this PR.
There is some commented-out configuration that should probably be removed?
Some other PRs were merged, so there are conflicts that need to be resolved.

pyproject.toml

justinmayer

Hi @lioman. I made some more recommended fixes for some problems I found.

.github/workflows/main.yml

pyproject.toml

- adapt documentation - add wheel tests to check wheel contents. - adapt pipeline to use pdm - adapt autopub config - add scripts as shortcuts to invoke tasks

justinmayer

Great work, Lioman. We're getting close! I commented on a few areas where I think we can still make improvements.

docs/contribute.rst

pyproject.toml

justinmayer · 2023-10-29T11:33:59Z

pyproject.toml

+    "black<20.0,>=19.10b0",
+    "ruff>=0.1.3,<1.0.0",
+    "tomli;python_version<'3.11'",
+]


Should we replace the long dev group with a more-specific set of docs, lint, and test groups? (example)

Except for Jinja2, Markdown, Pygments, and Furo, I think we can remove the upper bounds. More often than not, upper bounds cause more problems than they solve. (See example of the Black project dropping upper bounds on dependencies.)

Given that we are going to use Ruff, I don't think we need flake8, flake8-import-order, or isort any longer — Ruff contains all the functionality of those tools.

yes I think so too and thought about it. To keep this PR a bit smaller I propose to do it in an additional PR

I agree that deferring the separation into development dependency groups makes sense, but I think my comments in (2) and (3) are probably best resolved in the context of this PR, don't you think?

justinmayer

A few more comments. So sorry! I'm sure this is annoying. Thank you for your patience!

justinmayer · 2023-10-29T12:06:35Z

pelican/tests/build_test/test_wheel.py

+def test_wheel_contents(pytestconfig):
+    """
+    This test, should test the contents of the wheel to make sure,
+    that everything that is needed is included in the final build


I suggest removing the two commas in this sentence, as well as adding a period at the end.

justinmayer · 2023-10-29T12:07:42Z

pelican/tests/build_test/test_wheel.py

+    wheel_file = pytestconfig.getoption("--check-wheel")
+    assert wheel_file.endswith(".whl")
+    files_list = ZipFile(wheel_file).namelist()
+    ## Check is theme files are copiedto wheel


Maybe a single # instead of two? Also, there are some errors. I would change the text to:

# Check if theme files are copied to wheel

justinmayer · 2023-10-29T12:08:42Z

pelican/tests/build_test/test_wheel.py

+    ## Check is tool templatesare copiedto wheel
+    tools = Path("./pelican/tools/templates")
+    for x in tools.iterdir():
+        assert str(x) in files_list


I think this comment was copy+pasted and not relevant to what appears below it?

justinmayer · 2023-10-29T12:10:48Z

pyproject.toml

+    "black<20.0,>=19.10b0",
+    "ruff>=0.1.3,<1.0.0",
+    "tomli;python_version<'3.11'",
+]


I agree that deferring the separation into development dependency groups makes sense, but I think my comments in (2) and (3) are probably best resolved in the context of this PR, don't you think?

justinmayer · 2023-10-29T13:04:24Z

Hey @lioman. As I just mentioned in the chat room, I didn't realize my request to remove the upper bound on dependencies would result in that huge mass-formatting of other files. I think it's best to revert that and go back to what you had before that change (eb052ca, presumably). I am really sorry.

lioman · 2023-10-29T13:41:55Z

@justinmayer removed those commits. I will create an additional PR with those commits and suggest to apply it, just before the release, to minify the friction.

.github/workflows/main.yml

justinmayer

Many thanks to @lioman for the hard work on (and patience with) this endeavor. Much appreciated! 👏

avaris reviewed Oct 28, 2023

View reviewed changes

.pdm-python Outdated Show resolved Hide resolved

justinmayer requested changes Oct 28, 2023

View reviewed changes

.pdm-python Outdated Show resolved Hide resolved

pdm.lock Outdated Show resolved Hide resolved

pyproject.toml Outdated Show resolved Hide resolved

pyproject.toml Outdated Show resolved Hide resolved

pyproject.toml Outdated Show resolved Hide resolved

offbyone reviewed Oct 28, 2023

View reviewed changes

pyproject.toml Show resolved Hide resolved

pyproject.toml Outdated Show resolved Hide resolved

pyproject.toml Outdated Show resolved Hide resolved

pdm.lock Outdated Show resolved Hide resolved

lioman commented Oct 28, 2023

View reviewed changes

pyproject.toml Outdated Show resolved Hide resolved

lioman requested a review from justinmayer October 28, 2023 21:52

lioman and others added 2 commits October 29, 2023 08:48

migrate configuration to PEP621 compatible one

a76a419

Re-order pyproject items, with other small fixes

c18f1a7

lioman force-pushed the centralize-python-information branch from a0dde32 to f971730 Compare October 29, 2023 07:48

justinmayer requested changes Oct 29, 2023

View reviewed changes

.github/workflows/main.yml Show resolved Hide resolved

pyproject.toml Outdated Show resolved Hide resolved

pyproject.toml Outdated Show resolved Hide resolved

lioman added 2 commits October 29, 2023 11:59

migrate configuration to PEP621 compatible config

8b6d215

- adapt documentation - add wheel tests to check wheel contents. - adapt pipeline to use pdm - adapt autopub config - add scripts as shortcuts to invoke tasks

remove old setup files

00d26fc

lioman force-pushed the centralize-python-information branch from c7b5797 to 00d26fc Compare October 29, 2023 11:01

lioman requested review from justinmayer, avaris and offbyone October 29, 2023 11:03

justinmayer requested changes Oct 29, 2023

View reviewed changes

Capitalize PDM in docs

eb052ca

lioman requested a review from justinmayer October 29, 2023 12:01

justinmayer requested changes Oct 29, 2023

View reviewed changes

lioman force-pushed the centralize-python-information branch from bbde24f to eb052ca Compare October 29, 2023 13:11

lioman requested a review from justinmayer October 29, 2023 14:10

avaris reviewed Oct 29, 2023

View reviewed changes

.github/workflows/main.yml Outdated Show resolved Hide resolved

only install dev dependencies during lint step

8a0f335

avaris approved these changes Oct 29, 2023

View reviewed changes

justinmayer changed the title ~~migrate configuration to PEP621 compatible one~~ Make build configuration compatible with PEP621 Oct 29, 2023

justinmayer approved these changes Oct 29, 2023

View reviewed changes

justinmayer merged commit 73599f4 into getpelican:master Oct 29, 2023
14 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make build configuration compatible with PEP621 #3220

Make build configuration compatible with PEP621 #3220

lioman commented Oct 28, 2023 •

edited

justinmayer commented Oct 28, 2023

justinmayer left a comment

justinmayer commented Oct 28, 2023

justinmayer commented Oct 28, 2023

justinmayer left a comment

justinmayer left a comment

justinmayer Oct 29, 2023

lioman Oct 29, 2023

justinmayer Oct 29, 2023

justinmayer left a comment

justinmayer Oct 29, 2023

justinmayer Oct 29, 2023

justinmayer Oct 29, 2023

justinmayer Oct 29, 2023

justinmayer commented Oct 29, 2023

lioman commented Oct 29, 2023

justinmayer left a comment

Make build configuration compatible with PEP621 #3220

Make build configuration compatible with PEP621 #3220

Conversation

lioman commented Oct 28, 2023 • edited

Pull Request Checklist

Todo

justinmayer commented Oct 28, 2023

justinmayer left a comment

Choose a reason for hiding this comment

justinmayer commented Oct 28, 2023

justinmayer commented Oct 28, 2023

justinmayer left a comment

Choose a reason for hiding this comment

justinmayer left a comment

Choose a reason for hiding this comment

justinmayer Oct 29, 2023

Choose a reason for hiding this comment

lioman Oct 29, 2023

Choose a reason for hiding this comment

justinmayer Oct 29, 2023

Choose a reason for hiding this comment

justinmayer left a comment

Choose a reason for hiding this comment

justinmayer Oct 29, 2023

Choose a reason for hiding this comment

justinmayer Oct 29, 2023

Choose a reason for hiding this comment

justinmayer Oct 29, 2023

Choose a reason for hiding this comment

justinmayer Oct 29, 2023

Choose a reason for hiding this comment

justinmayer commented Oct 29, 2023

lioman commented Oct 29, 2023

justinmayer left a comment

Choose a reason for hiding this comment

lioman commented Oct 28, 2023 •

edited