-
Notifications
You must be signed in to change notification settings - Fork 122
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
build: migrate to pyproject.toml #1068
Changes from 30 commits
47f8537
c942cc7
a160ce8
a0feaeb
0d18f51
0b8f710
b58abed
edca060
9fedd2e
66b8057
96d0c39
183ff1a
3790aa4
54a471e
412b68a
2ce49c1
063abdc
c856db6
e4d41a7
5a921a6
3ad12d0
e9f0cb6
2947d92
fc47085
993892d
3190143
3fe6176
20ca414
adde28f
2a50930
71cedc2
3b7c10a
e6a23c3
7f32a09
9edc6fc
339740d
556d139
b6837c9
0d726d9
50b6386
1560224
fadd413
e1be35e
06de826
8867b1d
84fcb74
99ae4ee
18db4e5
c9c325d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,18 @@ | ||
recursive-exclude .github/ | ||
recursive-exclude docs/ | ||
exclude .readthedocs.yaml | ||
exclude .gitignore | ||
|
||
recursive-include test/ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It seems weird to me that we have to both include some dirs and exclude others (above). Why is that? Would it be more explicit to exclude everything and then start including explicit files/dirs? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The recommended approach is to include things broadly and then exclude them. The excludes remove from the collected list so need to be after the includes (I had this wrong originally). This is mostly a leftover from the earlier version of the PR where I was going to use setuptools-scm for the version. I'm not sure we want to bother with this now. The difference between the default of not having a MANIFEST.in and having the current one is just this:
The last one is interesting - it's not a file, it's a link. The default behaviour is that it's just skipped, but if it's included then it actually follows the link and there's a second copy of I feel that if you get the sdist then you're not looking for the I lean towards keeping the MANIFEST.in as it is now, but am open to arguments that we just get rid of it. |
||
recursive-include ops/ | ||
|
||
include .pyproject.toml | ||
include CHANGES.md | ||
include HACKING.md | ||
include README.md | ||
include CODE_OF_CONDUCT.md | ||
tonyandrewmeyer marked this conversation as resolved.
Show resolved
Hide resolved
|
||
include LICENSE.txt | ||
include MANIFEST.in | ||
include requirements.txt | ||
include requirements-dev.txt | ||
include tox.ini |
This file was deleted.
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 wonder -- should/could we make these tox actions, rather than having the developer manually install pip-compile and enter these commands (also below)?
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.
It's only required when you actually want to bump a dependency, but yes a tool is likely better.
We could have it like this instead, avoiding any lock file at all:
The biggest issue is that if the dependencies change, pip doesn't pick up on that, and you generally have to blow away the .tox/env folder, which is not much better than running a command.
We could have the dev dependencies in
tox.ini
, particularly since they're being spilt out, and especially if we don't group many of them (or if there is a way to say "use this same dependency list for multiple envs", other than having them in another file). One advantage of having them in standard places is that tools (like security scanners) easily find them, though, and I'm not sure if they would there.Or I can just do as you suggest and we keep the generated files (probably not in version control, per a different thread) and I can add a
tox -e update-deps
(name to be bikesheded 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.
Just having the
.[unit]
styledeps
value intox.ini
is no good - it avoids needing any compilation of what's inpyproject.toml
but the failure to pick up on changes is too inconvenient.In terms of speed (I did these 3 times and picked the median - not super accurate, but should be enough to spot order-of-magnitude differences), for a full
tox
:However, most of the time is (thankfully!) in the tests. For a quick env, like
lint
:I did some more investigation, and dependabot will find dependencies in
pyproject.toml
without having a lock file committed, but won't find them in tox.ini. Other tools seem mixed, but as far as I know we aren't using any at the moment, so I guess we could address that when/if that changes.I don't want to be significantly slower than current main. So that eliminates having both the
pip-compile
andpip-sync
intox.ini
.Having the dependencies in
optional-dependencies
sections inpyproject.toml
is recommended in some places and in others it seems like that's used only for optional features rather than dev dependencies. Being able to do a local install withpip install .[static,unit]
is nice, but it seems like the vast majority of the time people will be usingtox
, and doingpip install ops[unit]
doesn't seem like it's of use. Having requirements files generated seems inconvenient if we're not using them for anything else.So, on balance, I think putting them directly in
tox.ini
is the cleanest for now, and we can reconsider this if the scanning issue ever comes up.Except for the docs: readthedocs doesn't have tox, so we do need to have a generated
requirements.txt
file for there.