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

Remove build hacks #1001

Merged
merged 7 commits into from Jun 19, 2023
Merged

Remove build hacks #1001

merged 7 commits into from Jun 19, 2023

Conversation

flying-sheep
Copy link
Member

@flying-sheep flying-sheep commented Jun 12, 2023

Also switches from flit-core to hatchling, but at this point, that’s just a backend change.

It uses setuptools-scm (via the hatch-vcs plugin) as a build hook, so the version will ship in a generated _version.py file.

@codecov
Copy link

codecov bot commented Jun 12, 2023

Codecov Report

Merging #1001 (287c981) into main (d346beb) will decrease coverage by 0.03%.
The diff coverage is 64.28%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1001      +/-   ##
==========================================
- Coverage   84.11%   84.08%   -0.03%     
==========================================
  Files          34       33       -1     
  Lines        4727     4707      -20     
==========================================
- Hits         3976     3958      -18     
+ Misses        751      749       -2     
Impacted Files Coverage Δ
anndata/__init__.py 64.28% <64.28%> (-35.72%) ⬇️

@Koncopd
Copy link
Member

Koncopd commented Jun 12, 2023

Hi.
What is this hatch? Is it something like poetry?
I would really prefer to keep flit as it is pretty mininal.

@flying-sheep
Copy link
Member Author

flying-sheep commented Jun 12, 2023

Hatchling is the build backend we use in the scverse-cookiecutter template.

Compared to flit-core, it’s less opinionated (doesn’t derive version metadata from <pkg>.__version__, doesn’t derive the description from the docstring – a feature we haven’t been using) and supports plugins (which this PR uses by replacing the whole hacky _metadata.py file with the plugin-generated _version.py.

Its maintainer is also quicker with fixes and incorporation of new packaging standards. But as said: the main reason for this is that the _metadata.py file has accumulated a bunch of hacks to accomodate flit, and this makes things much simpler.

@ivirshup
Copy link
Member

ivirshup commented Jun 15, 2023

I'm fine with going from flit to hatch, less sure on hatch-vcs.

Does hatch-vcs add commit info, or is it all tag based? My recollection is that it will be wrong most of the time for development builds/ editable installs.

Is hatch + setuptools-scm an option?

See also: https://github.com/maresb/hatch-vcs-footgun-example

@flying-sheep
Copy link
Member Author

Is hatch + setuptools-scm an option?

See initial comment:

[This PR] uses setuptools-scm (via the hatch-vcs plugin) as a build hook, so the version will ship in a generated _version.py file.

{name = "Alex Wolf", email = "f.alex.wolf@gmx.de"},
]
readme = {file = "README.md", content-type="text/markdown"}
readme = "README.md"
Copy link
Member

Choose a reason for hiding this comment

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

I recall this throwing errors if I didn't specify the content-type. Was that flit specific?

Copy link
Member Author

Choose a reason for hiding this comment

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

not for many years: pypa/flit#169

Copy link
Member

Choose a reason for hiding this comment

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

Pretty sure I was running into this more recently than that. But I believe twine check was the one complaining, so it's fine as long as the build check is passing.

@ivirshup
Copy link
Member

It's more that I don't see how setuptools-scm is being used from the changes.

How does this not have the behavior described here:

Editable installs

The version file is only updated upon install or build. Thus the version number in an editable install (Hatch's dev mode) will be incorrect if the version changes and the project is not rebuilt. An unsupported workaround for keeping the version number up-to-date can be found at hatch-vcs-footgun-example.

@flying-sheep
Copy link
Member Author

flying-sheep commented Jun 15, 2023

To summarize our in-person discussion:

There’s no way to have the version in the package metadata automatically up-to-date. It can only be updated when (dev-)installing your package anew. There’s a discussion about adding dynamic version metadata here

The __version__ convention is of dubious value (packages usually look things up from package metadata using importlib.metadata), but of course it can be calculated using runtime code. That’s explained and demonstrated here.

“Ideally”, we’d run that code in dev mode, and a simple from ._version import __version__ in an installed package, but there seems to be no easy way to use hatch-vcs to generate different code based on the dev-ness of of the install.

Copy link
Member

@ivirshup ivirshup left a comment

Choose a reason for hiding this comment

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

Review

Could we add a step to the build check where we check that the built package imports and has a __version__?

One other point from discussion

We'd also discussed if we could strip out all the version finding logic and just hardcode the version for releases.

{name = "Alex Wolf", email = "f.alex.wolf@gmx.de"},
]
readme = {file = "README.md", content-type="text/markdown"}
readme = "README.md"
Copy link
Member

Choose a reason for hiding this comment

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

Pretty sure I was running into this more recently than that. But I believe twine check was the one complaining, so it's fine as long as the build check is passing.

.azure-pipelines.yml Outdated Show resolved Hide resolved
Co-authored-by: Isaac Virshup <ivirshup@gmail.com>
@flying-sheep flying-sheep merged commit 89d6906 into main Jun 19, 2023
9 checks passed
@flying-sheep flying-sheep deleted the simplify-build-metadata branch June 19, 2023 08:22
@ivirshup ivirshup mentioned this pull request Jan 18, 2024
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants