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

Improvement: Requirements installation in venv #686

Merged
merged 5 commits into from
Nov 7, 2023

Conversation

jdsika
Copy link
Contributor

@jdsika jdsika commented Nov 4, 2023

  • moved startup checks to launch_common
  • python venv mandatory due to Ubuntu > 23.xx
  • cleaned up checks for better understanding

Work effort: 4h

…buntu > 23.xx, cleaned up checks for better understanding

Signed-off-by: jdsika <carlo.van-driesten@vdl.digital>
@jdsika jdsika added the enhancement New feature or request label Nov 4, 2023
@jdsika jdsika added this to the v11.5 (Lima) milestone Nov 4, 2023
@jdsika jdsika requested a review from vkresch November 4, 2023 00:25
@jdsika
Copy link
Contributor Author

jdsika commented Nov 6, 2023

@vkresch please a quick review here

Signed-off-by: jdsika <carlo.van-driesten@vdl.digital>
@jdsika jdsika merged commit 030bdee into master Nov 7, 2023
3 of 8 checks passed
@jdsika jdsika deleted the feature/python-virtual-venv branch November 7, 2023 10:30
@nicolasochem
Copy link
Contributor

Hi! Can we revert this? It no longer works in a container.

│ Checking python version ...                                                                                                                                                                │
│                                                                                                                                                                                            │
│ ... installed Python version 3.10 is greater then minimum required version 3.7. OK!                                                                                                        │
│                                                                                                                                                                                            │
│ Please make sure to activate a virtual environment for python due to breaking changes in Ubutu >= 23.XX:                                                                                   │
│ https://packaging.python.org/en/latest/guides/installing-using-pip-and-virtual-environments/                                                                                               │
│                                                                                               

When running containerized, there is little reason to use a venv... Can you make this check more specific? Like, check for the exact reason you are making this mandatory? What's the ubuntu 23 thing?

@jdsika
Copy link
Contributor Author

jdsika commented Nov 14, 2023

After Ubuntu 23 it is considered a bad practice to install python packages system wide as it may interfere with the system functionality. You cannot use pip install anymore without an error. I actually think it makes sense and haven't thought about it before.

The message obviously does not tell the right story...
https://fostips.com/pip-install-error-ubuntu-2304/?amp=1

@lukeisontheroad
Copy link

For me, this broke the dockerized version, it's also not running with the build pushed yesterday and since the protocol EOL was hard coded in the constants I can't get it running anymore.

lukeisontheroad added a commit to lukeisontheroad/tezos-reward-distributor that referenced this pull request Jan 1, 2024
lukeisontheroad added a commit to lukeisontheroad/tezos-reward-distributor that referenced this pull request Jan 1, 2024
…virtual-venv

Revert "Improvement: Requirements installation in venv (tezos-reward-distributor-organization#686)"
@nicolasochem
Copy link
Contributor

yes, please remove the in_venv() check from main branch @jdsika

@vkresch
Copy link
Contributor

vkresch commented Jan 5, 2024

@jdsika I am removing the in_venv check in this PR #690. Let me know if it's fine. Imo the user is responsible for setting up his environment for TRD correctly. We might actually also consider removing the requirements_installed check since we provide the requirements.txt file which lists the packages the user should have installed before running TRD.

@jdsika
Copy link
Contributor Author

jdsika commented Jan 5, 2024

As long as the program shuts down gracefully if the packages are missing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants