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

Make build configuration compatible with PEP621 #3220

Merged
merged 6 commits into from
Oct 29, 2023

Conversation

lioman
Copy link
Contributor

@lioman lioman commented Oct 28, 2023

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
Copy link
Member

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 Outdated Show resolved Hide resolved
Copy link
Member

@justinmayer justinmayer left a comment

Choose a reason for hiding this comment

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

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

  1. 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.
  2. 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 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
@justinmayer
Copy link
Member

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 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
@justinmayer
Copy link
Member

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

pyproject.toml Outdated Show resolved Hide resolved
@lioman lioman force-pushed the centralize-python-information branch from a0dde32 to f971730 Compare October 29, 2023 07:48
Copy link
Member

@justinmayer justinmayer left a comment

Choose a reason for hiding this comment

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

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

.github/workflows/main.yml Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
- adapt documentation
- add wheel tests to check wheel contents.
- adapt pipeline to use pdm
- adapt autopub config
- add scripts as shortcuts to invoke tasks
Copy link
Member

@justinmayer justinmayer left a comment

Choose a reason for hiding this comment

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

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

docs/contribute.rst Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
"black<20.0,>=19.10b0",
"ruff>=0.1.3,<1.0.0",
"tomli;python_version<'3.11'",
]
Copy link
Member

Choose a reason for hiding this comment

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

  1. Should we replace the long dev group with a more-specific set of docs, lint, and test groups? (example)
  2. 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.)
  3. 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member

@justinmayer justinmayer left a comment

Choose a reason for hiding this comment

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

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

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
Copy link
Member

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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
Copy link
Member

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 lioman force-pushed the centralize-python-information branch from bbde24f to eb052ca Compare October 29, 2023 13:11
@lioman
Copy link
Contributor Author

lioman commented Oct 29, 2023

@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 Outdated Show resolved Hide resolved
@justinmayer justinmayer changed the title migrate configuration to PEP621 compatible one Make build configuration compatible with PEP621 Oct 29, 2023
Copy link
Member

@justinmayer justinmayer left a comment

Choose a reason for hiding this comment

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

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

@justinmayer justinmayer merged commit 73599f4 into getpelican:master Oct 29, 2023
14 checks passed
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.

Deprecate EOL version and centralize information about it
4 participants