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

Moved the metadata from setup.cfg into PEP 621-compliant pyproject.toml #2449

Closed
wants to merge 3 commits into from

Conversation

KOLANICH
Copy link

No description provided.

@codecov
Copy link

codecov bot commented Oct 18, 2022

Codecov Report

Merging #2449 (c9a6343) into master (e930d11) will increase coverage by 0.03%.
The diff coverage is n/a.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2449      +/-   ##
==========================================
+ Coverage   98.96%   99.00%   +0.03%     
==========================================
  Files         117      117              
  Lines       16119    16119              
  Branches     3122     3122              
==========================================
+ Hits        15952    15958       +6     
+ Misses        116      113       -3     
+ Partials       51       48       -3     
Impacted Files Coverage Δ
trio/tests/test_ssl.py 99.86% <0.00%> (+0.55%) ⬆️
trio/_highlevel_ssl_helpers.py 100.00% <0.00%> (+11.76%) ⬆️

@KOLANICH
Copy link
Author

This PR is built on top of an another PR #2448. It is expected that that PR is landed first, and than this one onto that one. The first commit moves the metadata into setup.cfg, that is available for ancient versions of setuptools, but is still declarative and machine-parseable. The second commit moves the metadata into pyproject.toml, it has a drawback of a more narrow range of setuptools versions supported. So if someone still has to use the ancient version, he can git revert the second commit.

pyproject.toml Outdated Show resolved Hide resolved
trio/_version.py Outdated Show resolved Hide resolved
@njsmith
Copy link
Member

njsmith commented Oct 19, 2022 via email

@KOLANICH
Copy link
Author

We want to keep this here because we need to inspect trio.version at runtime. Hatchling supports this.

_version.py file is generated automatically by setuptools_scm

Can't you use pkg_resources.get_distribution("trio").versionfor that?

We can, but it is damn slow.

But this requires making sure it renders well both in GitHub and PyPI.

Checked using https://github.com/pypa/readme_renderer, LGTM.

@KOLANICH KOLANICH force-pushed the pyproject.toml branch 3 times, most recently from fe4b4ce to 18d33bd Compare October 19, 2022 10:27
Copy link
Member

@pquentin pquentin left a comment

Choose a reason for hiding this comment

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

There are still various references to setup.py left.

Also we want to have a setup.py file to give a clear error because someone will inevitably use it, including GitHub itself: https://github.com/urllib3/urllib3/blob/main/setup.py

And I'm still not convinced setuptools_scm is better than hatchling 🤷 but it's not a huge deal. https://github.com/python-trio/trio/blob/master/docs/source/releasing.rst needs to change though if we're going to use that.

@ofek
Copy link

ofek commented Oct 21, 2022

Hatchling maintainer here! I do agree with @pquentin that Hatchling would make things simpler here (and generally speaking too, which is why it's now the default in the official Python packaging tutorial).

For example, the following:

[tool.setuptools]
include-package-data = true

[tool.setuptools.packages]
find = {namespaces = false}

is unnecessary with Hatchling because:

  1. There is no concept of package data; files are just files, which makes selection easier to reason about.
  2. The wheel target defaults would do the appropriate thing here.

Additionally, Hatchling:

  • Supports static analysis tools for editable installations by default whereas the new setuptools does not i.e. it requires enabling an option for IDEs to work.
  • Uses Git-style glob patterns rather than stdlib globs to better match user expectations.
  • Allows easy and explicit control of what gets shipped in the sdist target whereas with setuptools it's an objectively confusing mix of conditional wheel options and a MANIFEST.in file.
  • Hatchling produces a less verbose license section on PyPI because until they support PEP 639 the rendering is not what one may think. It's actually "License: {license_classifier.split(' :: ')[-1]} ({license['text']})" so what you have currently would become License: Apache Software License, MIT License (MIT OR Apache-2.0).

@KOLANICH
Copy link
Author

hatch_vcs has no write_to.

@KOLANICH
Copy link
Author

thanks for the info, probably the docs about write_to should have the build hook being mentioned.

@ofek
Copy link

ofek commented Oct 21, 2022

write_to is a setuptools-scm option

@KOLANICH
Copy link
Author

I mean that hatch-vcs' ReadMe says

The write_to and write_to_template parameters are ignored.

so I propose to write

Use build.hooks.vcs.{version-file,template} instead.

right after that text.

@pquentin
Copy link
Member

I'd rather continue writing to trio/_version.py manually.

@KOLANICH KOLANICH force-pushed the pyproject.toml branch 2 times, most recently from fb2f2da to 9bafbca Compare October 25, 2022 09:23
…tools` can change `build-system` section of `pyproject.toml`, the specific config for it was kept.
Comment on lines +54 to +62
[tool.setuptools]
include-package-data = true

[tool.setuptools.packages]
find = {namespaces = false}

[tool.setuptools_scm]
write_to = "trio/_version.py"
write_to_template = "__version__ = \"{version}\"\n"
Copy link

Choose a reason for hiding this comment

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

No need

@KOLANICH
Copy link
Author

The setuptools-specific code is for the ones who prefer using setuptools over hatchling. It is activated only after patching pyproject.toml.

@ofek
Copy link

ofek commented Oct 25, 2022

I don't think projects should cater to supposed preferences of alternative build pipelines. I remember something similar from earlier this year, I'll try to find the issue

@ofek
Copy link

ofek commented Oct 25, 2022

Oh found it, I didn't realize it was you 😄

@@ -66,11 +66,11 @@ fi

python -c "import sys, struct, ssl; print('#' * 70); print('python:', sys.version); print('version_info:', sys.version_info); print('bits:', struct.calcsize('P') * 8); print('openssl:', ssl.OPENSSL_VERSION, ssl.OPENSSL_VERSION_INFO); print('#' * 70)"

python -m pip install -U pip setuptools wheel
python -m pip install -U pip hatchling build
Copy link
Member

Choose a reason for hiding this comment

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

Backends are never needed in outer envs — the frontends automatically provision them in the build venvs.

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
python -m pip install -U pip hatchling build
python -m pip install -U pip build

@@ -44,7 +48,7 @@ Things to do for releasing:
* push to PyPI::

git clean -xdf # maybe run 'git clean -xdn' first to see what it will delete
python3 setup.py sdist bdist_wheel
python3 -m build -ws
Copy link
Member

Choose a reason for hiding this comment

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

Do not pass both --sdist and --wheel, because this causes wheels building from the Git checkout, as opposed to building wheels from sdist (as pip wheel does).

Suggested change
python3 -m build -ws
python3 -m build

python -m pip --version

python setup.py sdist --formats=zip
python -m pip install dist/*.zip
python -m build -nsx .
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 highly discouraged to use short options with unknown meanings in shell scripts. The typical convention is to use long names so that the readers could know what's happening without having to google or misinterpret things. Also, no need to pass the current dir as it's the default. And two of those options are probably harmful in this case too.

@@ -59,6 +59,8 @@ pytz==2022.4
# via babel
requests==2.28.1
# via sphinx
setuptools-scm==7.0.5
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't seem needed/related. Also, it's a bad idea to change behavior together with refactoring. If this plugin is needed, it should be integrated separately.

Copy link
Member

Choose a reason for hiding this comment

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

this seems like an accidental commit, you should probably add .kate-swp to your global git ignore

@graingert
Copy link
Member

closing in favour of #2860 which was merged

@graingert graingert closed this Jan 7, 2024
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.

8 participants