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

move setup.cfg to pyproject.toml #352

Merged
merged 3 commits into from
Oct 19, 2023
Merged

move setup.cfg to pyproject.toml #352

merged 3 commits into from
Oct 19, 2023

Conversation

sjvrijn
Copy link
Contributor

@sjvrijn sjvrijn commented Sep 28, 2023

Description

  • Removed setup.py and setup.cfg
  • Added all relevant info to pyproject.toml

Some minor adjustments made during the translation:

  • Added the requirement >=64.0.0 to setuptools under [build-system]: this is the first version to add support for PEP660: Editable installs for pyproject.toml based builds
  • Remove obsolete zip_safe setting
  • Simplified package data to just include-package-data = true under [tool.setuptools] in combination with requiring setuptools-scm. This automatically adds all data files that are included under source control.
  • Replaced package find options with an explicit packages = {{ cookiecutter.package_name }} under [tools.setuptools]
  • Added an additional cookiecutter variable repository_url to define an https github url under [project.urls], since it wouldn't accept the git url in cookiecutter.repository:
ValueError: invalid pyproject.toml config: `project.urls.Repository`.
      configuration error: `project.urls.Repository` must be url

Related issues:

Instructions to review the pull request

Create a python-template-test repo on GitHub (will be overwritten if existing)

cd $(mktemp -d --tmpdir py-tmpl-XXXXXX)
cookiecutter -c pyproject-toml-351 https://github.com/NLeSC/python-template
# Fill with python-template-test info
cd python-template-test
git init
git add --all
git commit -m "First commit"
git remote add origin https://github.com/<you>/python-template-test
git push -u origin main -f
python -m venv env
source env/bin/activate
python -m pip install --upgrade pip setuptools
python -m pip install '.[dev,publishing]'

@sjvrijn sjvrijn requested review from apalha, egpbos and gcroci2 and removed request for apalha September 28, 2023 17:02
Copy link
Member

@gcroci2 gcroci2 left a comment

Choose a reason for hiding this comment

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

Much cleaner now, great :) I inserted few minor suggestions, and I would only reorder the sections a bit (e.g., [tool.setuptools.packages.find] should go under [tool.setuptools] section).

"tox",
"myst_parser",
]
publishing = [
Copy link
Member

Choose a reason for hiding this comment

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

In my project I have build package as well in the publishing extra. Should be needed, right?

Copy link
Member

Choose a reason for hiding this comment

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

It's not yet needed, since we still depend on the old build methods. See #318 and #328 for two ways to move forward on this. Needs a separate PR by someone brave enough to make a choice ;)

Comment on lines 83 to 85
[tool.setuptools]
include-package-data = true
packages = ["{{ cookiecutter.package_name }}"]
Copy link
Member

Choose a reason for hiding this comment

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

After reading up on setuptools package discovery and data file inclusion (check especially the comment in the pyproject.toml example) I think this whole section could be removed. For sure include-package-data = true can be removed, because it is now true by default. The package discovery could be removed if we move to a src-based layout.

This actually has two advantages:

  1. This above, i.e. simpler configuration.
  2. It also automatically supports single-file packages (you don't need a folder for a package if it only consists of one file, but to support that you normally need to reconfigure, see e.g. how I did that in kwandl; I think that with a src dir, though, that will also just be autodiscovered, yay!).
Suggested change
[tool.setuptools]
include-package-data = true
packages = ["{{ cookiecutter.package_name }}"]

To be clear, this suggestion would have to be accompanied by adding a src dir.

Copy link
Member

@gcroci2 gcroci2 Oct 17, 2023

Choose a reason for hiding this comment

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

I also agree that moving to a src-based layout would be the nicest way to go :)

@egpbos
Copy link
Member

egpbos commented Oct 13, 2023

This is a great PR, thanks for this effort!

One additional change to a src-based layout would make this even better, since it would simplify configuration (see my comments). Do you have time for this? If not, I think we can merge after taking @gcroci2's suggestions into account and then make a new issue/PR for moving to src-based.

@sjvrijn
Copy link
Contributor Author

sjvrijn commented Oct 17, 2023

Thanks for the feedback @egpbos and @gcroci2, the PR has been updated.
Given the difficulties already indicated in #173, I've chosen to keep the move to an src setup to a separate PR that I'll work on next.

Copy link
Member

@gcroci2 gcroci2 left a comment

Choose a reason for hiding this comment

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

There is the same typo (almost) everywhere: reporitory instead of repository. For the rest feel free to merge when it's fixed :)

{{cookiecutter.directory_name}}/README.md Outdated Show resolved Hide resolved
{{cookiecutter.directory_name}}/README.md Outdated Show resolved Hide resolved
{{cookiecutter.directory_name}}/sonar-project.properties Outdated Show resolved Hide resolved
{{cookiecutter.directory_name}}/sonar-project.properties Outdated Show resolved Hide resolved
{{cookiecutter.directory_name}}/sonar-project.properties Outdated Show resolved Hide resolved
@sjvrijn
Copy link
Contributor Author

sjvrijn commented Oct 17, 2023

@gcroci2 good catch! downside of making a typo during a 'find & replace all' 😅

@egpbos
Copy link
Member

egpbos commented Oct 17, 2023

@sjvrijn great stuff, keep it coming! 😄 Be aware that the failure with >=3.9 is already fixed in #347.

@sjvrijn
Copy link
Contributor Author

sjvrijn commented Oct 17, 2023

@egpbos All other tests are now fixed, I'll merge this after #347 (and #353? I guess)

@egpbos
Copy link
Member

egpbos commented Oct 17, 2023

Ok, both other PRs are merged. There are some merge conflicts left to fix. Also, I was wondering: you now invoke -m build instead of setup.py, but doesn't that actually need build installed as a dependency (as @gcroci2 earlier suggested, see #352 (comment))?

Finally, if you are up for becoming a true UGHist, I warmly invite you to squash some of the commits (happy to do it together, I'm at the office tomorrow), but no worries if you don't have the time 😁

@sjvrijn sjvrijn force-pushed the pyproject-toml-351 branch 2 times, most recently from 7656b95 to 3d9889c Compare October 19, 2023 10:06
@sjvrijn
Copy link
Contributor Author

sjvrijn commented Oct 19, 2023

@egpbos Yep, I indeed had to include build to enable the sdist build according to this StackOverflow question. Unless there's another way you know of?

Also, UGH challenge accepted 💪 I've rebased my commits on top of the merged main, and fixuped some of the corrective commits into the first one.

@egpbos egpbos merged commit d1086f8 into main Oct 19, 2023
16 checks passed
@egpbos
Copy link
Member

egpbos commented Oct 19, 2023

UGH! Super nice addition, thanks!

egpbos added a commit that referenced this pull request Oct 19, 2023
This commit changed the test_subpackage build command, but left the now unnecessary '--sdist' and '--wheel' parameters. They are on by default for `python -m build`
egpbos added a commit that referenced this pull request Oct 20, 2023
This commit changed the test_subpackage build command, but left the now unnecessary '--sdist' and '--wheel' parameters. They are on by default for `python -m build`
@sjvrijn sjvrijn deleted the pyproject-toml-351 branch October 27, 2023 20:37
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.

Use pyproject.toml only for installing the generated package
3 participants