-
Notifications
You must be signed in to change notification settings - Fork 178
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
Use python3's venv module instead of virtualenv #1396
Use python3's venv module instead of virtualenv #1396
Conversation
I've always used Could we replace |
Would be happy to expand on this patch to remove it entirely, I don't see any major distros using Python < 3.3: https://repology.org/project/python/versions |
That wouldn't be important at all, as JM itself requires Python 3.6+. |
7514fa3
to
e5f924b
Compare
Great, updated to just remove |
Seems to be working for me. Tested both with default |
e5f924b
to
f960ee1
Compare
Let me know if anything else needs to be done |
@dongcarl thanks for the extra pair of eyes on our installation :) Looks like a good change. conceptACK based on a quick read up on review: ACK based on file search/grep; I believe this covers all references to partial tACK f960ee1 on Ubuntu 20.04 , specifically testing install and script running. The usual pain point here that apart from Debian and Ubuntu I can't really test anything else, but I guess this should be OK. @kristapsk feel free to merge whenever you think is reasonable. Btw @dongcarl do you have any thoughts or knowledge about the possibility of building an AppImage for our Qt app here? (obviously not directly related to the PR but thought I'd throw it out there). |
I tested on Gentoo. |
I didn't have time to test this yet, but I remember in the past having to install the python3-venv package on Debian and Ubuntu.
I don't know if it's still a thing, but it might be prudent to add |
I can reproduce the above on a clean Debian Bullseye. After installing |
Python 3.3 and above provide a standard venv module which provides all the functionality that install.sh needs from virtualenv. As of this commit, JM itself requires Python 3.6+ anyway.
f960ee1
to
11ddec7
Compare
Thanks @PulpCattel! Didn't know that was the case for Debian/Ubuntu. Fixed now.
Unfortunately not, I don't have any experience with AppImage :-/ |
Can somebody test this on macOS? |
11ddec7 fixes the Debian issue mentioned above. Tested successful installation and basic JM scripts. |
Proposal: merge this and see if a Mac user complains? And if they do they can stay on the 0.9.8 for now (it's not as if we're planning actual functionality changes, for the next release). Our inability to do testing on Mac has hurt us quite a bit. Have heard user reports recently about not being able to install on Mac, but as usual, we are not getting anyone helping us out in this area. It's probably hurting the project a fair bit. Personally while I have messed around with Mac VMs in the past, I have no intention of doing so now, so I will not be giving input on it. In the extreme we could just say that this software is unsupported on Mac, if we still cannot get anyone to test and report on it. But I don't want to do that because I know how many software devs use it as their main environment. |
We could try adding macos-latest in addition to ubuntu-latest in CI. I've done this recently for my bitcoin-scripts project. kristapsk/bitcoin-scripts@2eedf0a. But there were discussions about compute cost in #1218. My brother has a MacBook, will try to get my hands on it. |
Thanks. Additionally, though, do you agree with my proposal? |
It looks I will not be able to test on mac this year, probably not worth waiting for so long, agree we can merge this. About being unsupported officially on mac - as I said above, we could add macos-latest to the CI. |
Merging. And will open a new PR that adds macOS testing to CI. Wanted to add macOS testing on top of current master, but had errors because of not having virtualenv installed and getting rid of it simplifies macOS install. |
Just fyi, this broke the Jam dev environment where JM is installed inside docker (base image
Installing |
Yes, this should not break |
@theborakompanioni Could you check wasn't #1414 instead that broke Jam dev environment setup? |
Did not test yet, but I can tell you that it did work, once I installed |
@theborakompanioni I'm curious that the error you have is:
Maybe I just don't understand the environment it's being run in, but why would that happen? And how would manually installing |
Was surprising for me as well that this really fixed the build. Did not investigate more, as it was working again. Just wanted to let you know. This is the Dockerfile: https://github.com/joinmarket-webui/jam/blob/aa2b237e5208d867fd5c108da7a0a89d92d15b38/docker/regtest/dockerfile-deps/joinmarket/latest/Dockerfile (Note: it is for a local testing environment only). The file did not change lately and only a bug report of another user made me rebuild the images, thereby reproducing the error. Edit: Note that |
I modified Dockerfile to run
|
What I don't understand is why Python is not available when https://github.com/joinmarket-webui/jam/blob/0a7b709d597469951aa670b9f2008e2435c4e782/docker/regtest/dockerfile-deps/joinmarket/latest/Dockerfile runs |
Python 3.3 and above provide a standard venv module which provides all the functionality that install.sh needs from virtualenv.
Detect its existence and use it, fallback to virtualenv if not.