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
fix some CI issues (#636) #637
Conversation
This renames the file, adds it to the index, and reworks the doctest blocks which won't work in our docs build environment so they're code blocks instead. That does mean they're untested, but that's much easier to support for now.
This fixes the problem with pip-tools < 6.5.0 and pip 22 by adding pip-tools to the dev requirements, setting it to 6.5.0, and installing that before checking the requirements. This also moves requirements checking code to the scripts/run_tests.sh script so it's easier to do in a local dev environment. This also rebuilds the dev requirements.
The 3.6 tests (3) are failing because we've got dependencies that don't work with that. I'm pretty sure #629 will fix that, so I'm not going to bother with those. The 3.7 tests (3) are failing with this:
I think we can fix this by building requirements-dev.txt using Python 3.7 rather than Python 3.9. The Windows checks (3) fail with this:
I'm not sure offhand how to fix that. Maybe a platform-specific requirements-dev.txt file? Ugh. The pypy checks (3) fail with the same issue as the 3.6 tests:
Stepping back a bit, I'm not sure we should be doing a requirements-dev.txt at all and instead should put the top-level requirements in I'll think more about this tomorrow and talk with @g-k about issue #620. |
+1 on dropping 3.6 it'll make cleaner type syntax available to use too.
wfm
Yeah, kinda gross. I've had to do this for other projects. Commented 👍 on #620 |
@@ -34,17 +35,16 @@ jobs: | |||
restore-keys: | | |||
${{ matrix.os }}-${{ matrix.python-version }}- | |||
|
|||
- name: Install dev dependencies | |||
run: | | |||
python -m pip install -r requirements-dev.txt |
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 moved pip-tools
to be a requirement because it's easier/better to maintain all the dev requirements in one place and that's a dev requirement. Thus we need to install the requirements before comparing pip-compile output.
@@ -12,6 +12,7 @@ Contents | |||
goals | |||
dev | |||
changes | |||
migrating |
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 chapter wasn't included in the index before, but it should have been.
@@ -19,50 +20,63 @@ replacement for the html5lib one. | |||
|
|||
Here is an example of replacing the sanitization method: | |||
|
|||
.. doctest:: | |||
.. code:: |
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 order for these to be doctest blocks and pass our tests, we'd have to install html5lib as a dev requirement and I don't want to do that because I think it'll make it harder to maintain everything if html5lib is both a dev requirement and also a vendored package.
I don't want to use the vendored version because it makes these illustrative examples harder to understand.
Further, these examples are missing bits, so we'd have to flesh them out as complete examples.
Given all that, I thought it'd be easier to just turn them into code blocks for now.
@@ -1,6 +1,7 @@ | |||
# Requirements for installing other packages | |||
pip | |||
setuptools>=18.5 | |||
pip-tools==6.5.0 |
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.
pip-tools 6.5.0 fixes the issue that prevents it from working with pip 22, so we should at least require 6.5.0.
@@ -1,5 +1,5 @@ | |||
# | |||
# This file is autogenerated by pip-compile with python 3.7 | |||
# This file is autogenerated by pip-compile with python 3.9 |
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 curious. The github workflow had comparing running with all the python versions, but pip-compile puts the Python version number in the file, so it'd never diff without raising issues.
I ended up rebuilding the file with Python 3.9, but maybe we should do it with 3.7 and some of the issues go away? I don't know. I think the check for determining whether the requirements-dev.txt file is updated isn't working right. Maybe it should be in the lint workflow instead since that only runs with one version of Python?
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.
+1 on using 3.7 or moving dev deps to a dev pkg
echo "diffing requirements-dev.txt and requirements-dev.txt.orig" | ||
diff requirements-dev.txt requirements-dev.txt.orig | ||
rm requirements-dev.txt | ||
mv requirements-dev.txt.orig requirements-dev.txt |
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 added this because it's difficult to have non-trivial bits in the CI workflows that we can't easily run in a local dev environment. However, I'm not thrilled with how this works. pip-compile is a little finicky about things.
Maybe it makes more sense to copy things to a tmp directory, compile it there, and then diff it?
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.
r+
@@ -1,5 +1,5 @@ | |||
# | |||
# This file is autogenerated by pip-compile with python 3.7 | |||
# This file is autogenerated by pip-compile with python 3.9 |
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.
+1 on using 3.7 or moving dev deps to a dev pkg
Thank you! |
Fix doctest linting issues with migrating chapter (#636)
This renames the file, adds it to the index, and reworks the doctest blocks which won't work in our docs build environment so they're code blocks instead. That does mean they're untested, but that's much easier to support for now.
Fix pip-tools problem and rework requirements (#636)
This fixes the problem with pip-tools < 6.5.0 and pip 22 by adding pip-tools to the dev requirements, setting it to 6.5.0, and installing that before checking the requirements.
This also moves requirements checking code to the
scripts/run_tests.sh
script so it's easier to do in a local dev environment.This also rebuilds the dev requirements.