tooling: Add requirements.in and rationalize deps #17984
Conversation
repokitteh-read-only
bot
added
the
deps
phlax
force-pushed
the
tooling-requirements-in
branch
8 times, most recently
from
05cfede
to
adcfb65
Compare
Signed-off-by: Ryan Northey <[email protected]>
phlax
force-pushed
the
tooling-requirements-in
branch
from
adcfb65
to
19c7645
Compare
|
@phlax please try split PRs like this in the future into trivial (i.e. the requirements.in) which can be rubber stamped and the unrelated deps rationalization. |
| @@ -0,0 +1,5 @@ | |||
| coverage | |||
There was a problem hiding this comment.
We're not picking specific versions any more?
There was a problem hiding this comment.
not in the requirements.in file unless we have a specific reason to - the requirements.txt file is generated from these and that does lock/pin/pick specific versions
that said we can set versions here if we need to and pip-compile should respect them (not sure about dependabot)
There was a problem hiding this comment.
some further investigation of using pip-compile
one annoying issue is that it uses sets and doesnt order the hashes, which means running it is non-deterministic (ie the hashes keep reordering) - which is a pita 8/. I had thought that this problem was due to running different versions of pip-compile, but now it seems like the issue is more as described.
the other thing which doesnt work so well with pip-compile is conditional deps
if you set a conditional dep on eg sys_platform = "win32" or somesuch pip-compile respects it in the sense that if you are not on windows it doesnt get included in the requirements.txt file
this is pretty rubbish for mananging repo requirements if you expect different install/run environments
There was a problem hiding this comment.
There was a problem hiding this comment.
this is already an issue as windows seems to be expecting pyreadline in some of my prs
we can just add it as dep, currently that would make linux pull it in unnecessarily, but that also could be resolved with (the new) pip_install_incremental in bazel
the problem with this approach occurs where there is a dep for some particular env that you really dont want to be pulled in, but im not sure that is an issue for envoy. In that case the best thing would be to separate the deps in bazel i think
its not really unrelated, i added the pins so that i could do the rationalization - what i was reluctant to do was add loads of pins, and requirements file updates (which would have been just as large a pr) when rationalizing just got rid of the files altogether i have a few prs in waiting to further tidy up and rationalize tooling, they all intersect, which is why i pushed this up front, to get rid of anything that would otherwise be repeatedly changed |
Signed-off-by: Ryan Northey <[email protected]>
| @@ -4,7 +4,6 @@ | |||
| import re | |||
| import subprocess | |||
| import fileinput | |||
| from six.moves import input | |||
There was a problem hiding this comment.
if we want to keep six here we could depend on it from elsewhere (ie base_pip3)
i would prefer not to. Python >= 3.8 is an assumption baked into a lot of the tooling (even tho much will work with varying mileage with less)
the pytooling repo tests against 3.8 and 3.9 currently
|
#18026 provides a better solution for this as it rationlizes the requirements down to a single file by using the new pip_install_incrememental im therefore closing this in favour of that pr |
Signed-off-by: Ryan Northey [email protected]
Commit Message: tooling: Add requirements.in and rationalize deps
Additional Description:
This adds a
requirements.infile to list actual deps to create therequirements.txtfile from, which will make it much easier to manage pip depsIt also removes several requirements targets that are not necessary
Risk Level:
Testing:
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Deprecated:]
[Optional API Considerations:]