-
Notifications
You must be signed in to change notification settings - Fork 507
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
Create requirements-relaxed file #271
Conversation
b4a7d8c
to
6163643
Compare
tests/requirements-relaxed.in
Outdated
sphinx | ||
sphinx-notfound-page | ||
sphinx-ansible-theme | ||
rstcheck < 6 # # rstcheck 6.x has problem with rstcheck.core triggered by include files w/ sphinx directives https://github.com/rstcheck/rstcheck-core/issues/3 |
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.
Broken versions should still be in a separate file. It's possible to make an additional constraints file for this. If rstcheck is not a direct dependency of we don't rely on a public API that is missing from v6, it's not right to put it into the direct dependencies file and such a restriction should come from constraints.
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, we do call rstcheck directly in https://github.com/ansible/ansible-documentation/blob/devel/tests/checkers/rstcheck.py. I'm not sure why we need a separate file for that. It's a direct dependency.
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.
But we don't depend on < 6
features, we depend on any version. < 6
happens to be a temporary constraint, not related to features we directly depend on. So semantically, it doesn't belong among the direct dependencies. A lower boundary would make sense if we knew since which version features we use appeared in the dependency, 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.
I don't think it's a good idea to add yet another constraints file, just so that we can remove all <
, >
from this file. The new file would contain half of the lines from this file (while this file won't get any shorter, only in terms of line length), and the packages mentioned in it still have to be mentioned in requirements-relaxed.in.
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 pushed 577fbd8 so we have something concrete to discuss, but yeah, I'm not convinced it's worthwhile to add yet another file. It may be more semantically correct, but it seems overly complicated to me.
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 so that we can remove all
<
,>
from this file
I disagree. This is not about adding or removing arbitrary bytes. This is about not making a mess of concepts being mixed up randomly with no meaning.
|
||
jinja2 >= 3.0.0 # used by hacking/build_library/build_ansible/command_plugins/generate_man.py and dump_keywords.py | ||
pyyaml >= 5.1 # used by ansible-core | ||
resolvelib >= 0.5.3, < 1.1.0 # used by ansible-core |
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.
Same here. It's a transitive dependency, it should be in a constraints file.
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 why we have some constraints in one file and others in another.
Anyways, if we need to grab stuff from ansible-core, what do you think about adding -r ../requirements.txt
to pull from the ansible-core requirements.txt created copied by docs/bin/clone-core.py
?
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 why we have some constraints in one file and others in another.
There are inputs to pip-compile and outputs. Requirements declare direct dependencies (as in things that we use known features from). Then, there's known broken versions of (possibly transitive) dependencies. We want to exclude them from the dependency resolution, but not confusingly mark them as direct dependencies. All of these are inputs.
What pip-compile output is the true lockfile. But I still want to separate semantically differing things by not dumping them into the same place.
Anyways, if we need to grab stuff from ansible-core, what do you think about adding
-r ../requirements.txt
to pull from the ansible-core requirements.txtcreatedcopied bydocs/bin/clone-core.py
?
I was considering this, but this will not work with Dependabot since it doesn't know about other repos and would just break. Having all the input files here allows managing the lockfiles without depending on unexpected external states.
577fbd8
to
2d60ca6
Compare
# This file is autogenerated by pip-compile with Python 3.10 | ||
# by the following command: | ||
# | ||
# pip-compile --allow-unsafe --output-file=tests/requirements-relaxed.txt --strip-extras tests/requirements-relaxed.in |
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 this needs to be pip-compiled again without the tests
folder. Probably also need to get the requirements.txt
file 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.
It depends on the CWD, usually. And with Dependabot, that may also change. Ideally, this would be in nox with pip-compile arguments to produce a custom header with a custom command stated here.
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 is fine. It's simpler than changing directories and then running pip-compile
.
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.
sure, but I think it should be consistent with the existing requirements.txt
file right?
# pip-compile --allow-unsafe --output-file=requirements.txt --strip-extras requirements.in |
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 noxfile in #258 is set up to run from the repo root, so this will be changed in the next dependency update PR.
tests/requirements-relaxed.in
Outdated
rstcheck | ||
antsibull-docs ~= 2.0 | ||
|
||
-c constraints-base.in |
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 if it's the convention or not but I would prefer to have any referenced files before the dependencies.
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 don't think I've ever seen this being at the end. It's usually at the beginning.
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 seems like this line belongs in constraints.in
instead of this file.
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.
constraints.in
is only for antsibull
and sphinx
which are pinned to a specific version in requirements.txt
but updated to the latest version by pip-compile in requirements-relaxed.txt
. That's the whole point of this change.
@gotmax23 it'd be nice to add a README file inside the |
This moves around the requirements a little bit so newer antsibull-docs and sphinx versions can be tested independently of production ansible documentation builds. rstcheck and resolvelib constraints were moved out of constraints.in. This file will only be used for strict pins that we want to manually update separately from the rest of the dependencies. Co-authored-by: Sviatoslav Sydorenko <wk.cvs.github@sydorenko.org.ua> Co-authored-by: Don Naro <dnaro@redhat.com>
f098d0d
to
592a3fc
Compare
Thank you @oraNod, @webknjaz, and @felixfontein for your valuable feedback! |
|
Co-authored-by: Sviatoslav Sydorenko <wk.cvs.github@sydorenko.org.ua>
592a3fc
to
66b7492
Compare
Force pushed to fix a commit message. I'll merge afterwards. |
I opened #312 to track this. We can address it in another PR. |
This moves around the requirements a little bit so newer antsibull-docs
and sphinx versions can be tested independently of production ansible
documentation builds.
rstcheck and resolvelib constraints were moved out of constraints.in.
This file will only be used for strict pins that we want to manually update
separately from the rest of the dependencies.