-
Notifications
You must be signed in to change notification settings - Fork 11
fix: the generated sdist file should contain everything needed to check and test the code, and to build the documentation as well #967
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
base: main
Are you sure you want to change the base?
Conversation
…ck and test the code, and to build the documentation as well
I think most of your Makefile is usable without a git repo - and even pre-commit config can be used by creating a new blank git repo (that's really what pre-commit maintainers suggest 🤷 ). So I'd be inclined to leave them in the sdist. But it's definitely a matter of taste, once you go beyond the tests & docs that downstream packagers typically expect. 🙂 |
pyproject.toml
Outdated
".gitignore", | ||
".pre-commit-config.yaml", | ||
".gitattributes", | ||
"CHANGELOG.md", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would also tend to include the changelog in the sdist, as part of the documentation. Also probably the security info.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Commit 098bcd1.
# See also: https://github.com/pypa/flit/issues/565 | ||
# See also: https://github.com/pypa/flit/discussions/745 | ||
[tool.flit.sdist] | ||
include = [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Heads up, what happens here will currently depend on whether you run flit build
directly, or make an sdist through a generic tool that calls flit as a backend, like python -m build
. With flit build
, it currently uses info from git to include all checked-in files, like tests and docs sources, similar to git archive
, except for those listed in exclude. However, Flit as a backend to general tools starts from only the files you need for installation. From Flit 4.0 this will be the default mode for flit build
as well, although you'll be able to specify --use-vcs
to keep the old behaviour.
https://flit.pypa.io/en/stable/pyproject_toml.html#including-files-committed-in-git-hg
So you might want to explicitly list includes for things you do want in the sdist.
(I can get into how we ended up in this scenario if you want. But the short version is, I like git-archive style sdists, but I don't think the backend can assume the presence of git, and I don't want it to behave differently if git is present.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @takluyver! With that in mind, I think it would make sense to
- document source distribution better in our README; and
- add a
make build-from-sdist
goal to our Makefile which takes an sdist and- runs the tests; then
- produces a wheel (using build or flit build); then
- produces the documentation.
@behnazh what do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One of the primary goals of using make build-from-sdist
would be to reliably reproduce build artifacts I guess. To validate that the artifact built from source matches the original, it would be ideal to ensure the resulting package is bitwise equivalent. Achieving this level of reproducibility would eliminate the need for additional testing, and significantly simplifies the validation process.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To validate that the artifact built from source matches the original, […]
With “original” I assume you mean a previously built wheel from the same commit hash? Basically:
- Build a wheel from the repo at commit hash H, then
- Build the sdist from the repo at commit hash H, then build a wheel from that sdist using the build timestamp from 1. and both wheels must be the same?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With “original” I assume you mean a previously built wheel from the same commit hash? Basically:
1. Build a wheel from the repo at commit hash _H_, then 2. Build the sdist from the repo at commit hash _H_, then build a wheel from that sdist using the build timestamp from 1. and both wheels must be the same?
Yes, exactly.
So, it takes a tad more to make this work on a folder unpacked from a After unpacking the archived repository — as the thread hints — we need to |
This change supersedes PR #948 based on discussion pypa/flit#745, and contributes to PR #952.
The generated sdist file now contains these files:
This is essentially a git archive of the folder allowing somebody to build the package and documentation, to run all code checks, and to run the tests.
However, because this generated archive is not a git folder but our development process assumes both git and pre-commit, the archive doesn’t quite work out of the box. I think it would be ok to remove Makefile and .pre-commit-config.yaml, and let the user figure out the rest?
Tagging @takluyver