-
Notifications
You must be signed in to change notification settings - Fork 252
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
refactor: use flit as a backend #546
Conversation
Forget, |
I also didn't notice #536, which this is a partial dup of. I was trying to avoid changing anything (like keeping |
pyproject.toml
Outdated
|
||
|
||
[tool.flit.sdist] | ||
include = ["LICENSE*", "tests/", "docs/", ".coveragerc", ".flake8", "mypy.ini", ".pre-commit-config.yaml"] |
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.
Let’s exclude everything other than the licenses, and stick to flit’s defaults?
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.
We do need at least tests. Tests are very important to include in the SDist, distribution packages and conda-forge need the tests folder. But all the other stuff is not needed as far as I know.
include = ["LICENSE*", "tests/", "docs/", ".coveragerc", ".flake8", "mypy.ini", ".pre-commit-config.yaml"] | |
include = ["LICENSE*", "tests/", "docs/"] |
(Added docs because apparently some downstream may like those too)
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.
As I understand it, the license and readme files are automatically included when referenced as files in the metadata. (worth double-checking though)
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.
The license is not. See the discussion in the corresponding issue and pypa/flit#549
f58dc9a
to
c4ca63a
Compare
pyproject.toml
Outdated
|
||
[tool.flit.sdist] | ||
include = ["LICENSE*", "tests/", "docs/", ".coveragerc", ".flake8", "mypy.ini", ".pre-commit-config.yaml"] | ||
exclude = ["docs/_build", "tests/manylinux/build-hello-world.sh", "tests/musllinux/build.sh", "tests/hello-world.c"] |
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.
The docs/_build
directory isn't automatically ignored?
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'm not sure. I think including "docs" grabs everything in it. Hopefully pycache and such are ignored... Yeah, I need to look at the internal ignore rules. I'm mildly irritated that the standards-mode doesn't use git for includes, while directly using filt <commands>
does.
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.
Unfortunately, __pycache__
is not ignored.
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.
Yeah, I'm not seeing any default ignores at all. There's also no recursive ignore, that is, I can't write **/__pycache__
as a rule, everything must be relative to the base dir. Flit pretty much assumes you'll always use flit build
or whatever the command is and even says the PEP 517 backend is not intended for distribution - see https://github.com/pypa/flit/blob/048c87c380ac41efc4b26222114e54f6581c64f6/flit_core/flit_core/sdist.py#L70-L72. @takluyver?
CI will always be a clean checkout, so this is pretty safe, but irritating that we'd have to build possibly long ignore list over time.
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 thought Flit, by default, included everything in Git and left everything out?
Another option is to move to Hatch.
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.
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.
In comparison to that flit-core has no external dependencies your list is massive.
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.
Also it depends on this package. Cyclical dependencies cannot definitely be allowed in bootstrap chain.
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.
Yes, there's nothing wrong with dependencies, but the core packages need a bootstrap able package that doesn't depend on them too. That's okay, this is what Flit is intended for. Every other package besides this core list can use hatchling or setuptools. :)
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 be fair, flit has a lot of dependencies (most likely indirectly this package). Flit-core works well as part of bootstrap chain.
c4ca63a
to
69a2542
Compare
|
||
[tool.flit.sdist] | ||
include = ["LICENSE*", "tests/", "docs/"] | ||
exclude = ["docs/_build", "tests/manylinux/build-hello-world.sh", "tests/musllinux/build.sh", "tests/hello-world.c", "tests/__pycache__", "build/__pycache__"] |
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.
This does not include everything that MANIFEST.in has. Also MANIFEST.in is not removed. I think it's not anymore used with flit?
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 think the differences are explained above (unless you see something specific). If I forgot MANIFEST.in, that's a mistake. :)
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.
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.
@henryiii I just tried to test your branch to see if my change to flit helps. I'm confused. Running nox for tests, docs or lint does not create tests/__pycache__
or build/__pycache__
for me. Are you sure these are really relevant to exclude anymore with flit?
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.
... does not create
tests/__pycache__
I don't know why that would be unless you are running Python with -B
or the equivalent env var.
or
build/__pycache__
I think that was added by mistake, we are not bundling a build
folder.
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 was not running Python explicitly. I was running this project's test runner and it did not create tests/__pycache__
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 suggest confirming whether build is necessary and removing tests in follow-up when flit is fixed to exclude bytecode automatically and this project depends on newer version of flit.
69a2542
to
ddf3e34
Compare
|
||
[tool.flit.sdist] | ||
include = ["LICENSE*", "tests/", "docs/"] | ||
exclude = ["docs/_build", "tests/manylinux/build-hello-world.sh", "tests/musllinux/build.sh", "tests/hello-world.c", "tests/__pycache__", "build/__pycache__"] |
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 don't think we need to exclude __pycache__
.
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.
__pycache__
is only automatically excluded from the package folder.
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.
We might want to report another issue about that... I don't think it makes any sense for sdist to contain Python bytecode.
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 don't think it makes any sense for sdist to contain Python bytecode.
Correct, Hatchling hard-codes that exclusion.
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.
Created pypa/flit#568
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.
That said, even if that is fixed, it would imply requiring a very new version of flit. Maybe it makes sense just go with this and have an issue about cleaning it up later.
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.
There hasn't been a flit-core release with recursive globs FYI.
@henryiii what do you want to do with this PR? Do you want to address any feedback and take it off draft? |
ddf3e34
to
65bcac1
Compare
We can improve this once there's a new Flit release (possibly with a few more small changes), but this will work fairly well for now (especially from a fresh directory, there should be no issues). |
@pradyunsg you good with this? |
I'm good with this -- I'd like to revisit what files we put in our sdist in a follow up though. :) |
@henryiii can you resolve the one conflict so we can merge this? And then if you're up for it open a new issue to examine what files are going into the sdist? |
Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>
Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>
Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>
Co-authored-by: Brett Cannon <brett@python.org>
db2f626
to
75bae36
Compare
Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com> Co-authored-by: Brett Cannon <brett@python.org>
Closes #340. Moves to using Flit. Closes #536.
I've tried to replicate the original structure, though I don't really
understand why things like
.pre-commit-config.yaml
are put in the SDist (butnoxfile.py
is not).