Fix httpcore dependency; fix style linting #257

c0llab0rat0r · 2021-04-01T04:40:01Z

Fix httpcore dependency by removing comment that broke pip
Add scripts to run tests and typing checks with Docker against multiple versions of Python

Previous to this change, attempts to run

tox -e styleck -e typeck

..resulted in this error:

ERROR: Invalid requirement: 'httpcore (~= 0.10.2)  # Has Unix domain socket support'

The error is caused by this part of that line being present in tox.ini:

# Has Unix domain socket support

This commit does not fix the typing errors referenced in #255 but does demonstrate the typing errors now that the httpcore dependency is fixed.

ntninja · 2021-04-03T14:10:24Z

Hmm… I never considered the typing and style checks to be particularily hard to run. Using docker to run them seems kinda overkill to me, but if people feel this is helpful I'll add this.

However, before we do that, could you tell me what output you are seeing when running the tox -e styleck -e typeck command directly on your system? Also what does tox --version report? (Tox has a feature where it'll provision a newer version of itself, if the host version is too old, so I'd strongly prefer using that instead if that is all there is to it.)

c0llab0rat0r · 2021-04-03T17:37:59Z

I didn't have tox installed at all. In the python:3.8.5 docker image its 3.7.0, and when I install tox into my venv its 3.23.0.

The tox output, both from the docker runs and with tox now installed into the venv, is two categories of errors.

Linting, such as:

./venv/lib/python3.8/site-packages/netaddr/strategy/eui48.py:267:1: WT191 (flake8-tabs) indentation contains spaces, but option indent-style=tab requires tabs

And typing, such as:

ipfshttpclient/client/base.py:212: error: Incompatible return value type (got "Callable[[VarArg(Any), KwArg(Any)], Union[None, List[R], ResponseWrapIterator[T, R]]]", expected "_returns_multiple_wrapper2_t[T, R]") 
[return-value]
    return wrapper2

c0llab0rat0r · 2021-04-03T17:45:43Z

The primary benefit of the docker images is validating against the array of supported python versions easily with a clean build. You could check them individually with separate virtual environments, but then you have to maintain the virtual environments and dependencies could drift. Docker is a clean slate each time.

c0llab0rat0r · 2021-04-03T18:18:59Z

I think there's a combination of a small number of legitimate issues the type and style checks are catching, and a larger number of false positives caused by a lack of ignores for checking dependencies (e.g. warnings for venv/../code.py).

I'm going to make some adjustments and push a revision.

ntninja · 2021-04-03T18:21:57Z

Thank you, now I understand much better what this issue is about! I thought this was merely to workaround some issue you had with tox, but using this for cross-version checks makes much more sense. (I have a setup for this without using docker, but that is not something most people can be asked to reproduce.)

ntninja

You just dropped all the Docker-related code I tried to review. What is the plan there?

ntninja · 2021-04-03T18:23:23Z

verify.sh

+    validate 3.5.10
+    validate 3.6.13
+    validate 3.7.10
+    validate 3.8.8


Is is possible to use aliases like 3.5, 3.6, … here to avoid listing all these detailed versions?

You can; it's a trade off between build consistency and being automatically on a later point release of Python.

Which would you prefer?

I think ideally it would use the same source of truth as the rest of the build and scrape the config files, but I didn't get that far.

Consistency is probably better, you're right! It's probably best to test the supported earliest version of each Python minor release.

I think ideally it would use the same source of truth as the rest of the build and scrape the config files, but I didn't get that far.

The current “single source of truth” would be the tool.flit.metadata.requires-python key in pyproject.toml, whose value would effectively evaluate to the following Python minimum minor version configuration:

validate 3.5.4 validate 3.6.2 validate 3.7.2 validate 3.8.0

It's not straightforward at all to derive these values programmatically through. :-/

ntninja · 2021-04-03T18:23:57Z

verify.sh

+    validate 3.7.10
+    validate 3.8.8
+else
+    echo "Validating only $1..."


Nit: Please use ellipsis (…), instead of three dots (...) here.

ntninja · 2021-04-03T18:24:36Z

verify.sh

+    tag=py-ipfs-http-client-verify:$python_version
+
+    docker build --build-arg PYTHON_VERSION=$python_version -t $tag .
+    docker run -it -v $(pwd):/source:ro -w /usr/src/app $tag tox -e styleck -e typeck


Use docker run --rm to ensure the used image doesn't stick around needlessly.

You just dropped all the Docker-related code I tried to review. What is the plan there?

I thought I was getting some push back on it so I decided to pare down the PR to changes to the library. I can add it back if you like, with changes per the comments you added.

FYI - the linting step of the Travis build now passes. I don't have fixes for the remaining type checks since I'm not familiar enough with protocols to address it.

Do you want me to add the Docker scripts back into this PR, or open a new PR for it?

Please open a new PR for the Docker stuff. The current PR is indeed useful as is, so no need to clutter it further.

ntninja · 2021-04-03T18:31:41Z

Dockerfile

+FROM python:${PYTHON_VERSION}
+
+RUN apt-get update -y && apt-get install -y apt-utils tox


Would it be OK to use the Python -alpine images here for smaller image download size?

Install command would be apk update && apk add --no-cache py3-tox in this case.

ntninja · 2021-04-03T18:32:40Z

verify.sh

+}
+
+if [ -z "$1" ]; then
+    echo "Validating supported versions..."


Nit: Please use ellipsis (…), instead of three dots (...) here.

ntninja · 2021-04-03T18:34:53Z

entrypoint.sh

+#!/usr/bin/env bash
+
+cp -r /source/* /usr/src/app/
+
+exec $@
+


Can this script be inlined into the Dockerfile (less clutter)? Also #!/bin/sh is sufficient here.

ntninja · 2021-04-03T18:38:05Z

verify.sh

+    python_version=$1
+
+    echo "Validating version $python_version..."
+
+    tag=py-ipfs-http-client-verify:$python_version
+
+    docker build --build-arg PYTHON_VERSION=$python_version -t $tag .
+    docker run -it -v $(pwd):/source:ro -w /usr/src/app $tag tox -e styleck -e typeck


Please always add double-quotes (") around variable/process substitutions (ie: $tag → "${tag}") unless you explicitly intend to invoke shell word splitting. Figuring out whitespace issues with shell is a huge PITA otherwise.

ntninja · 2021-04-03T18:43:09Z

test/run-tests.py

-				"# Exclude lines specific to some other Python version from coverage",
-				"exclude_lines =",
-			), map(lambda s: "\t" + s, exclusions))))
+				(


It will behave better here if you move the opening parenthese to the line above. The way it's currently written was actually a workaround to a bug in an older version of the checker. 😉

It will behave better here if you move the opening parenthese to the line above. The way it's currently written was actually a workaround to a bug in an older version of the checker. wink

^ Do this.

I had tried simply moving the parenthesis on line 180 up earlier but it didn't like that. The latest change flattens that entire section and passes the linter.

ntninja · 2021-04-03T18:56:45Z

As a more general note on the Docker thing: It is not really necessary to run the type and style checks in different Python versions. CI only checks the latest version and this is on purpose.

For typing, lot's of workarounds were required for pre-3.8 versions type hints, so the hints are provided on a best-effort basis there. The reason for running the type check at all is to ensure our typings are non-contradictionary and this property should not change depending on the 3.8+ Python version run (many mypy bugs permitting), so checking more than one Python version isn't all that helpful.

For flake8 the Python version is pretty much totally irrelevant, except that the version needs to be new enough to understand all the syntax used in the project's source code (ast and tokenize modules). Again, this is the case for any Python version since 3.6, so no need to run the check on more than one version.

c0llab0rat0r · 2021-04-03T19:00:07Z

Linting step for 3.8 build is passing now.

ntninja

Just some minor stuff now.

ipfshttpclient/encoding.py

ipfshttpclient/filescanner.py

ntninja · 2021-04-04T09:11:07Z

test/run-tests.py

-				"# Exclude lines specific to some other Python version from coverage",
-				"exclude_lines =",
-			), map(lambda s: "\t" + s, exclusions))))
+				(


It will behave better here if you move the opening parenthese to the line above. The way it's currently written was actually a workaround to a bug in an older version of the checker. wink

^ Do this.

ntninja

Thank you!

c0llab0rat0r added 2 commits April 3, 2021 13:34

flake8 linting corrections; ignore venv created by PyCharm

37e8550

Remove comment from http core dependency

06278d1

c0llab0rat0r force-pushed the httpcore-dependency-remove-comment branch from 314d37d to 06278d1 Compare April 3, 2021 18:38

ntninja reviewed Apr 3, 2021

View reviewed changes

ntninja requested changes Apr 4, 2021

View reviewed changes

c0llab0rat0r added 3 commits April 4, 2021 13:27

Remove unnecessary noqa hints related to line length

5e696b7

Correct spelling of compatibility

527160f

Reformat coveragerc.file.writelines in run-tests.py

01cdd88

ntninja approved these changes Apr 4, 2021

View reviewed changes

c0llab0rat0r mentioned this pull request Apr 4, 2021

Add scripts to verify compatibility with multiple Python versions #259

Merged

c0llab0rat0r changed the title ~~Fix httpcore dependency; add scripts demonstrating its fixed~~ Fix httpcore dependency; fix style linting Apr 4, 2021

This was referenced Apr 5, 2021

Unbreak the mypy tests #255

Closed

[hold until 3.6 EOL] Bump min Python version from 3.6.2 to 3.7.2 #260

Open

ntninja merged commit ce535f3 into ipfs-shipyard:master Apr 5, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix httpcore dependency; fix style linting #257

Fix httpcore dependency; fix style linting #257

c0llab0rat0r commented Apr 1, 2021 •

edited

Loading

ntninja commented Apr 3, 2021

c0llab0rat0r commented Apr 3, 2021 •

edited

Loading

c0llab0rat0r commented Apr 3, 2021

c0llab0rat0r commented Apr 3, 2021

ntninja commented Apr 3, 2021

ntninja left a comment

ntninja Apr 3, 2021

c0llab0rat0r Apr 3, 2021

ntninja Apr 4, 2021 •

edited

Loading

ntninja Apr 3, 2021

ntninja Apr 3, 2021

c0llab0rat0r Apr 3, 2021

ntninja Apr 4, 2021

ntninja Apr 3, 2021

ntninja Apr 3, 2021

ntninja Apr 3, 2021

ntninja Apr 3, 2021

ntninja Apr 3, 2021

ntninja Apr 4, 2021 •

edited

Loading

c0llab0rat0r Apr 4, 2021

ntninja commented Apr 3, 2021

c0llab0rat0r commented Apr 3, 2021

ntninja left a comment

ntninja Apr 4, 2021 •

edited

Loading

ntninja left a comment

		FROM python:${PYTHON_VERSION}

		RUN apt-get update -y && apt-get install -y apt-utils tox

Fix httpcore dependency; fix style linting #257

Fix httpcore dependency; fix style linting #257

Conversation

c0llab0rat0r commented Apr 1, 2021 • edited Loading

ntninja commented Apr 3, 2021

c0llab0rat0r commented Apr 3, 2021 • edited Loading

c0llab0rat0r commented Apr 3, 2021

c0llab0rat0r commented Apr 3, 2021

ntninja commented Apr 3, 2021

ntninja left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ntninja Apr 4, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ntninja Apr 4, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ntninja commented Apr 3, 2021

c0llab0rat0r commented Apr 3, 2021

ntninja left a comment

Choose a reason for hiding this comment

ntninja Apr 4, 2021 • edited Loading

Choose a reason for hiding this comment

ntninja left a comment

Choose a reason for hiding this comment

c0llab0rat0r commented Apr 1, 2021 •

edited

Loading

c0llab0rat0r commented Apr 3, 2021 •

edited

Loading

ntninja Apr 4, 2021 •

edited

Loading

ntninja Apr 4, 2021 •

edited

Loading

ntninja Apr 4, 2021 •

edited

Loading