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

Chore: Upgrade to python 3.10 #1688

Merged
merged 71 commits into from
Sep 13, 2022

Conversation

relativisticelectron
Copy link
Collaborator

@relativisticelectron relativisticelectron commented Apr 30, 2022

From #1686

TODO:

@netlify
Copy link

netlify bot commented Apr 30, 2022

Deploy Preview for specter-desktop-docs ready!

Name Link
🔨 Latest commit d2c4ef5
🔍 Latest deploy log https://app.netlify.com/sites/specter-desktop-docs/deploys/631f0bf97666130009422744
😎 Deploy Preview https://deploy-preview-1688--specter-desktop-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@k9ert
Copy link
Collaborator

k9ert commented Apr 30, 2022

I'd prefer to keep all the versions stable as we migrate to 3.10, otherwise we might fight too many battles at the same time. So if you want to upgrade other stuff as well, let's try to do that on a separate PR?
For sure, if we need to migrate a version of a dependency as part of the migration towards 3.10. then do it. However, depending on how complex that individual migration is, consider to do it on a extra PR within python 3.9. Depending on how big of a deal the whole thing gets, it's considered good practice. Have heard of it first in the http://mikadomethod.info/

@relativisticelectron
Copy link
Collaborator Author

relativisticelectron commented Apr 30, 2022

I see 2 possible options:

  1. Keep openssl version 1.1.1 (not available any more in ubuntu 22.04) and upgrade python and necessary python packages.
  2. Use openssl 3 as in ubuntu 22.04, and keep the python packages in older versions, if possible. --> Might only be doable once embit mitigates the missing ripemd160 in openssl3.

Which one did you have in mind?

@k9ert
Copy link
Collaborator

k9ert commented Apr 30, 2022

I'd separate these two issues. Why does an upgrade to python 3.10 involves an upgrade of ubuntu? hashlib is not bringing its own version of openssl so this is a problem of the underlying OS, right?

This has been discussed on hwi: bitcoin-core/HWI#305
There is also a potential solution here which is using a python-only implementation from bitcoin-core created on this issue.

At the end, this must be solved in embit and i'll suggest to create an issue there and discuss there.

@relativisticelectron
Copy link
Collaborator Author

I'd separate these two issues. Why does an upgrade to python 3.10 involves an upgrade of ubuntu? hashlib is not bringing its own version of openssl so this is a problem of the underlying OS, right?

Ok. I will do the upgrade to python 3.10 in an older ubuntu version and reduce dependency version changes to a minimum.

At the end, this must be solved in embit and i'll suggest to create an issue there and discuss there.

Ok, will you create an issue in embit?

@k9ert
Copy link
Collaborator

k9ert commented May 3, 2022

Ok, will you create an issue in embit?

Well, i thought, you started it, you go for it :-) ?! I hope there will be a more consistent and confirmed way to go forward. Maybe @stepansnigirev can judge what's the problem with Simplexum/python-bitcointx#65. Seems like Marco Falke is ok with it: bitcoin/bitcoin#23710 (comment)

@relativisticelectron
Copy link
Collaborator Author

relativisticelectron commented May 3, 2022

When building a docker image

docker build -t registry.gitlab.com/cryptoadvance/specter-desktop/python-bitcoind:v0.20.1 .

following https://github.com/cryptoadvance/specter-desktop/blob/master/docker/python-bitcoind/Readme.md , I get

Reading package lists...
Building dependency tree...
Reading state information...
E: Unable to locate package libqt4-dev
The command '/bin/sh -c apt update && apt install git build-essential autoconf libboost-all-dev libssl-dev libprotobuf-dev protobuf-compiler libqt4-dev libqrencode-dev libtool libevent-dev pkg-config bsdmainutils -y' returned a non-zero code: 100

Am I doing something wrong?

@k9ert : If you have time, it would be great if you could help with the docker images.

Well, i thought, you started it, you go for it :-)

Ok. diybitcoinhardware/embit#28

@stepansnigirev
Copy link
Collaborator

released new version of embit with a pure-python implementation of ripemd160 taken from bitcoin/test_framework
https://github.com/diybitcoinhardware/embit/releases/tag/v0.4.13

@k9ert
Copy link
Collaborator

k9ert commented May 3, 2022

Am I doing something wrong?

I'm not sure why this is happening. However, we should remove it anyway. Qt4 is for the Bitcoin-UI and we don't care about that. Just remove that one. Probably if you migrate to the newest version of bitcoind, you need --without-gui probably like

RUN cd bitcoin && ./autogen.sh --without-gui && ./configure --without-gui && make

I guess only one of them need it. probably configure, not autogen but i'm not sure. If the images builds fine, tell me and i'll do the build here and push to the registry.
Imagename should probably be then:

registry.gitlab.com/cryptoadvance/specter-desktop/python-bitcoind:v23.0

(assuming you use the newest version). Btw, you can also contact me on telegram and/or signal any time.

@relativisticelectron
Copy link
Collaborator Author

relativisticelectron commented May 4, 2022

Thanks. I will try that (looks good so far). I am not clear on which docker images are used for which purpose (I assume some are used for the github automated checks?).

  • Which docker images actually need updating? All that contain python3.8?

- Added libzmq to build process, to enable future use cases such as cryptoadvance#778
- added --without-gui to remove configure warning that gui will not be built.
@k9ert
Copy link
Collaborator

k9ert commented May 4, 2022

Good question. I would start in this order:

  • tests on Cirrus: .cirrus.yml :
    • registry.gitlab.com/cryptoadvance/specter-desktop/cirrus-focal (from focal to jammy)
    • registry.gitlab.com/cryptoadvance/specter-desktop/cypress-python (also from focal to jammy)
  • releasing via gitlab: .gitlab-ci.yml :
    • registry.gitlab.com/cryptoadvance/specter-desktop/python-bitcoind (from 3.8 to 3.10)
    • registry.gitlab.com/cryptoadvance/specter-desktop/cypress-python (done at that point)
    • registry.gitlab.com/cryptoadvance/specter-desktop/electron-builder (oh, that needs research)

I think the best approach is that you push to your own registry and replace the references. When stuff works, i can rebuild and push to the appropiate registry.gitlab.com/cryptoadvance/specter-desktop location. Feel free to change the naming of the images as well.

And thank you very much for your work.

@relativisticelectron
Copy link
Collaborator Author

Thanks, that explanation is very helpful.

  • registry.gitlab.com/cryptoadvance/specter-desktop/cirrus-focal (from focal to jammy)

  • registry.gitlab.com/cryptoadvance/specter-desktop/cypress-python (also from focal to jammy)

When I upgrade to jammy, I also have to increase the embit version to 0.4.13.

@relativisticelectron
Copy link
Collaborator Author

relativisticelectron commented May 4, 2022

I am stuck.... somehow cirrus ci doesn't pull the docker images any more from gitlab registry
image
with the error:

exclamation Failed to start an instance! Failed to pull null image! Repository does not exist or may require authentication.

I guess I have to use a public gitlab project... Trying that next
EDIT: Yes the images have to be on a public gitlab project

@relativisticelectron
Copy link
Collaborator Author

relativisticelectron commented Sep 3, 2022

In 6823a2e I reference the docker image 14-wine-mono-08.22, see electron-userland/electron-builder#6922 (comment)

requirements.in Show resolved Hide resolved
@k9ert
Copy link
Collaborator

k9ert commented Sep 8, 2022

So:

We might still want more testing but i guess we can proceed to the last step, replacing the Docker-images with official ones. As far as i can see, here is the list of image-changes:

cypress-base-ubuntu-jammy (new)

  • Now: registry.gitlab.com/relativisticelectron/specter-desktop/cypress-base-ubuntu-jammy:latest
  • changeTo: registry.gitlab.com/cryptoadvance/specter-desktop/cypress-base-ubuntu-jammy:20220908
    • image built

cypress-python --> cypress-python-jammy

  • Before: registry.gitlab.com/cryptoadvance/specter-desktop/cypress-python:v9.7.0
  • Now: registry.gitlab.com/relativisticelectron/specter-desktop/cypress-python-jammy:v9.7.0
  • changeTo: registry.gitlab.com/cryptoadvance/specter-desktop/cypress-python-jammy:v9.7.0
    • image built

cirrus-focal --> cirrus-jammy

  • Before: registry.gitlab.com/cryptoadvance/specter-desktop/cirrus-focal:20210831
  • Now: registry.gitlab.com/relativisticelectron/specter-desktop/cirrus-jammy:latest
  • changeTo: cryptoadvance/specter-desktop/cirrus-jammy:20220908 (a timestamp)
    • image built

python-bitcoind

  • Before: registry.gitlab.com/cryptoadvance/specter-desktop/python-bitcoind:v0.20.1
  • Now: registry.gitlab.com/relativisticelectron/specter-desktop/python-bitcoind:v22.0
  • changeTo: registry.gitlab.com/cryptoadvance/specter-desktop/python-bitcoind:v22.0
    • image built

electron-builder

  • Was always latest and needs a proper tag: jammy

  • References in gitlab-ci were not uptodate which means we need to redo another test-release

    • image built
  • update all references

@relativisticelectron
Copy link
Collaborator Author

relativisticelectron commented Sep 8, 2022

There is still the docker image registry.gitlab.com/relativisticelectron/specter-desktop/cypress-base-ubuntu-jammy:latest

Copy link
Collaborator Author

@relativisticelectron relativisticelectron left a comment

Choose a reason for hiding this comment

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

see comment


# The block below is created with pip-compile in python 3.8, such that requirements.txt is backwards compatible with python 3.8
# This package is not needed for python 3.10
backports.zoneinfo==0.2.1 ; python_version < '3.10' \
Copy link
Collaborator Author

@relativisticelectron relativisticelectron Sep 8, 2022

Choose a reason for hiding this comment

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

@k9ert : Unfortunately pip-compile does not support these environment markers, and this block has to be placed here manually after doing pip-compile (in python 3.10). Do you know a better way?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure i understand the issue.
requirements.txt is independent on the python version. I see the issue that dependencies might differ between python-versions but have no idea how this could potentially be addressed.

Copy link
Collaborator Author

@relativisticelectron relativisticelectron Sep 13, 2022

Choose a reason for hiding this comment

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

The issue is a difference of pip-compile in different python versions:

  1. Delete requirements.txt
  2. pip-compile --generate-hashes requirements.in in python 3.10
  3. backports.zoneinfo will be missing in requirements.txt and will have to be manually copied in again.

or:

  1. Delete requirements.txt
  2. pip-compile --generate-hashes requirements.in in python 3.8
  3. backports.zoneinfo will be in requirements.txt.

@k9ert
Copy link
Collaborator

k9ert commented Sep 8, 2022

I've just created yet another test-release git tag v1.13.0-pre2 && git push upstream v1.13.0-pre2 which should show up soon:
https://github.com/cryptoadvance/specter-desktop/releases/tag/v1.13.0-pre2

@@ -17,6 +17,9 @@ docker run --rm -ti \
build the image like:
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@k9ert : This command

docker run --rm -ti \
 --env-file <(env | grep -iE 'DEBUG|NODE_|ELECTRON_|YARN_|NPM_|CI|CIRCLE|TRAVIS_TAG|TRAVIS|TRAVIS_REPO_|TRAVIS_BUILD_|TRAVIS_BRANCH|TRAVIS_PULL_REQUEST_|APPVEYOR_|CSC_|GH_|GITHUB_|BT_|AWS_|STRIP|BUILD_') \
 --env ELECTRON_CACHE="/root/.cache/electron" \
 --env ELECTRON_BUILDER_CACHE="/root/.cache/electron-builder" \
 -v ${PWD}:/project \
 -v ${PWD##*/}-node-modules:/project/node_modules \
 -v ~/.cache/electron:/root/.cache/electron \
 -v ~/.cache/electron-builder:/root/.cache/electron-builder \
 registry.gitlab.com/relativisticelectron/specter-desktop/electron-builder:latest

returns an error for me

docker: open /proc/self/fd/11: no such file or directory.
See 'docker run --help'.

So I have no way of testing the electron builder inside docker...

Copy link
Collaborator

Choose a reason for hiding this comment

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

I was not able to reproduce your issue with:

  • Docker version 20.10.14, build a224086
  • Ubuntu 20.04.4 LTS

developing/fixing those pipelines is always a horrible pain in the ass. Ever been and i can't imagine that will ever change much. Sometimes it's worth, sometimes better (e.g. cirrus has a way to run the pipeline locally).
I usually do a mixture but often enough let the pipeline run for the 20th time rather than trying to reproduce locally (while doing something else in the meantime).

I've created a new tag and pushed it this time to my fork in order to not pollute the main project. You could potentially do that as well: https://github.com/cryptoadvance/specter-desktop/blob/master/docs/continuous-integration.md#cicd-dev-env-setup

@k9ert
Copy link
Collaborator

k9ert commented Sep 9, 2022

There is a major roadblock with the electron-builder:jammy image. As it looks serious and might take longer/complicated to be fixed, i've created a separated issue with #1879 .

@k9ert
Copy link
Collaborator

k9ert commented Sep 9, 2022

ok, after upgrading docker (now 20.10.18) on the buildagent and three reboots, it seem to somehow be more stable now.
Another tag (k9ert/v1.13.0-pre4) worked fine :
https://gitlab.com/k9ert/specter-desktop/-/pipelines/635874801

I will now create yet another one but for the main repo:

git tag v1.13.0-pre5 && git push upstream v1.13.0-pre5

soon here:
https://github.com/cryptoadvance/specter-desktop/releases/tag/v1.13.0-pre5

@relativisticelectron
Copy link
Collaborator Author

relativisticelectron commented Sep 9, 2022

  • @k9ert : We should test all plugins for compatibility, or are they covered by pytest?

@k9ert
Copy link
Collaborator

k9ert commented Sep 10, 2022

We should test all plugins for compatibility, or are they covered by pytest?

No, testing the plugins which are shipped directly is not covered by any tests. We don't even have a good test-concept for the plugins.

But we might even have more severe issues here at hand. When i try to run Specter-1.13.0-pre5.AppImage on my Ubuntu 20.04.4 LTS, i get:

2022-09-10T13:17:28.636Z [info] : stderr-[2631121] Error loading Python lib '/tmp/_MEI55Z2s7/libpython3.10.so.1.0': dlopen: /lib/x86_64-linux-gnu/libm.so.6: version `GLIBC_2.35' not found (required by /tmp/_MEI55Z2s7/libpython3.10.so.1.0)

Not good. I guess i have to dive into the the compatibility details of glibc again.

@relativisticelectron
Copy link
Collaborator Author

relativisticelectron commented Sep 10, 2022

2022-09-10T13:17:28.636Z [info] : stderr-[2631121] Error loading Python lib '/tmp/_MEI55Z2s7/libpython3.10.so.1.0': dlopen: /lib/x86_64-linux-gnu/libm.so.6: version `GLIBC_2.35' not found (required by /tmp/_MEI55Z2s7/libpython3.10.so.1.0)

I can reproduce your error in Virtualbox with Ubuntu 20.04.

The error suggests that libpython3.10 needs at least glibc 2.35.
image

Ubuntu 20.04 has however (GLIBC 2.31-0ubuntu9.7) 2.31 (that explains also why python 3.10 is not available in Ubuntu 20.04).

My initial idea for a path forward would be (also suggested here):

  • the binaries have to be build in ubuntu 20.04 with python 3.8, which also runs in ubuntu 22.04 --> we have to downgrade the docker containers again
  • after this PR is merged, pip installation should work with python 3.8 (ubuntu 20.04) and python 3.10 (ubuntu 22.04)

@k9ert
Copy link
Collaborator

k9ert commented Sep 11, 2022

Fully agree to all your points. Effectively, i came to the same conclusion almost exactly 2 years ago but forgot about it 🤦 .
So IMHO all we need to do is to rollback the electron-builder changes. Will do that next week.

@k9ert
Copy link
Collaborator

k9ert commented Sep 12, 2022

Ok, rolled back the electron-builder and created yet another test-release.

soon here:
https://github.com/cryptoadvance/specter-desktop/releases/tag/v1.13.0-pre6

I've also smoketested the extensions with python3.10 in a pip-installation, works!
For some reason, the build for the documentation doesn't seem to be in trouble anymore. So that's solved as well.

I guess after testing the mentioned pre6, we might be ready to merge, no?!

@relativisticelectron
Copy link
Collaborator Author

I guess after testing the mentioned pre6, we might be ready to merge, no?!

Yes.

  • AppImage Pre6 on Ubuntu 20.04 works
  • pip installation on Ubuntu 22.04 with python 3.8 and python 3.10 works

@k9ert k9ert merged commit a8f1b80 into cryptoadvance:master Sep 13, 2022
@k9ert
Copy link
Collaborator

k9ert commented Sep 13, 2022

Thank you very much appreciated 🙇‍♂️

https://twitter.com/k9ert/status/1569609110093729792

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.

None yet

3 participants