Skip to content
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

Add python3.8 support #1471

Merged
merged 6 commits into from
Nov 6, 2019
Merged

Add python3.8 support #1471

merged 6 commits into from
Nov 6, 2019

Conversation

voith
Copy link
Contributor

@voith voith commented Oct 17, 2019

What was wrong?

web3.py doesn't support python3.8.

How was it fixed?

Add support for python 3.8.

Todo:

Cute Animal Picture

Put a link to a cute animal picture inside the parenthesis-->

@voith
Copy link
Contributor Author

voith commented Oct 17, 2019

I expect the build to fail. cytoolz doesn't build against python3.8 yet. related issue

@voith
Copy link
Contributor Author

voith commented Oct 29, 2019

Most of Ethereum's python tooling won't be able to update to python 3.8 unless pytoolz/cytoolz#136 is resolved. I have been actively following the issue, but there haven't been any updates to it.

@voith
Copy link
Contributor Author

voith commented Nov 4, 2019

A fix for cytoolz has been released. I have opened a PR in eth-utils to add support for python3.8.

Copy link
Contributor Author

@voith voith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note for self.

setup.py Outdated Show resolved Hide resolved
@voith voith force-pushed the python3.8 branch 4 times, most recently from 30c5064 to b8bbfdf Compare November 4, 2019 08:35
@@ -179,7 +179,8 @@ jobs:
- image: circleci/python:3.6
environment:
TOXENV: py36-ethpm
WEB3_INFURA_PROJECT_ID: 4f1a358967c7474aae6f8f4a7698aefc
WEB3_INFURA_PROJECT_ID: ae13436d29b34850b640af9c80f80871
WEB3_INFURA_API_SECRET: 067ddad6021048f0840a38a4e52eb568
Copy link
Contributor Author

@voith voith Nov 4, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kclowes @njgheorghita Tests were failing without WEB3_INFURA_API_SECRET . Here's a failing build. It seems to me that the handshake in websockets is much more strict in this new version(v8.1). I have generated this temporary key pair from my personal account. Please create a new key pair. The infura API is rate limited, so it'd be better to add these environment variables through circleci's config. Else the tests might fail randomly with rate limit errors if someone starts abusing this key.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔 that's interesting. It seems like you shouldn't need the secret key unless something changed with that project. I wonder if the project settings associated with the key changed recently? It's not my key so I can't see it.

it'd be better to add these environment variables through circleci's config.

We opted to leave the keys in the code (usually with a comment that says please don't abuse) because anyone who contributes has access to the circle env variables anyway and we thought it's better to just put them out there, rather than have someone assume at some point that the CircleCI env vars are actually secret and expose a real vulnerability. If someone abuses we'll just change them out.

Copy link
Contributor

@njgheorghita njgheorghita Nov 4, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests were failing without WEB3_INFURA_API_SECRET .

It seems like you shouldn't need the secret key unless something changed with that project.

Yup, nothing's changed with the project settings, so I'm not sure that this was the cause of the error, as it looks like we're not close to hitting the rate limits on that key. For the 4f1.... key's project settings, you don't need a secret. I'm guessing it was just a glitchy request with Infura. I just ran the tests with the 4f1... key and everything seemed to pass just fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

well, I couldn't replicate the error locally on OSX. However, after entering circleCI in debug mode, the secret key is the only thing that fixed the issue. I was getting a 401 unauthorized error.
Here's a similar issue in websockets.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I couldn't get circle tests to pass without the secret key either ¯_(ツ)_/¯. Updated to a throw-away key.

py38-integration-parity-ipc:
<<: *parity_steps
docker:
- image: circleci/python:3.8
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I want to point out that it seems like stretch images are no longer needed. Back when this commit was added parity would fail to build on other distros.

@voith voith mentioned this pull request Nov 4, 2019
1 task
Copy link
Collaborator

@kclowes kclowes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me! Thanks @voith! I'll swap out keys before merging.

@voith
Copy link
Contributor Author

voith commented Nov 4, 2019

argh, looks like the checksum of parity binary has changed. I can take a look at this tomorrow.
If anyone else wants to take a stab at this then please be my guest.

@voith
Copy link
Contributor Author

voith commented Nov 4, 2019

argh, looks like the checksum of parity binary has changed

Tests are green now. The test failures were random and had nothing to do with the change in the checksum.

@kclowes kclowes merged commit 40c292d into ethereum:master Nov 6, 2019
@voith voith deleted the python3.8 branch November 6, 2019 16:53
@kclowes kclowes mentioned this pull request Oct 9, 2020
@gabriellm1 gabriellm1 mentioned this pull request Oct 14, 2020
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants