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

Why do the installation instructions encourage "sudo pip install" #3066

Open
robvdl opened this issue Oct 19, 2024 · 7 comments
Open

Why do the installation instructions encourage "sudo pip install" #3066

robvdl opened this issue Oct 19, 2024 · 7 comments

Comments

@robvdl
Copy link

robvdl commented Oct 19, 2024

First of all, I personally don't run this project (I run a zoneminder installation), but I am a Python developer and a friend came across it and installed it.

I was a bit shocked to see "sudo pip install" instructions, especially because that is going away in future versions of pip.

https://stackoverflow.com/questions/61452582/why-is-using-sudo-pip-a-bad-idea

https://pages.charlesreid1.com/dont-sudo-pip/

Why are we encouraging this sort of installation? Why can't we recommend users to create a virtualenv instead and teach good practices?

I don't want to come across negative, but "sudo pip install" is always a bad idea.

@MichaIng
Copy link
Member

motionEye has a low number of dependencies without any specific version requirements. So IMO a venv/virtualenv does not add any benefit. If anyone is running into any conflicts with the system-wide installation, I would be interested to know, since it must be a very rare and special case.

However, you are right that Python generally moves towards venv only, recet Debian and Ubuntu install flag files with their distro packages which prevent pip from installing system-wide, without adding another flag or config option. So yes, we should switch to venv in our install instructions.

Please open a PR for the readme, if someone finds time. Also the install tests should be updated accordingly.

@robvdl
Copy link
Author

robvdl commented Oct 19, 2024

I dunno if I agree with the "does not add any benefits" because my friend did run into conflicts. I'll have a look at it when I can, but I'm going to try zoneminder with that setup instead.

There has to be a better way to install it. But I thought that we got over that "sudo pip install" thing years ago, almost nobody does that anymore.

@MichaIng
Copy link
Member

Did you even read the second half of my comment?

@robvdl
Copy link
Author

robvdl commented Oct 19, 2024

You're taking about the --user flag?

@MichaIng
Copy link
Member

No, that is still prevented on Debian and Ubuntu. This part of my comment is what I mean:

So yes, we should switch to venv in our install instructions.

Please open a PR for the readme, if someone finds time. Also the install tests should be updated accordingly.

It is one more package python3-venv and two more commands to set it up and load it. The problem is that the systemd service needs to be changed as well. Not yet sure how to do this best. We would need to detect the venv in the linux_init script, if present, and in that case load it and get the meyectl path accordingly.

@robvdl
Copy link
Author

robvdl commented Oct 19, 2024

I'll have to have a good read of the code first to get an understanding of the installation, it looks like motioneye_init.py invokes a shell script which makes me wonder what the point of that is and why not just run the shell script directly. But there may have been a reason.

Makes me wonder "who not just make install" when there already is a Makefile there in the project root.

@MichaIng
Copy link
Member

MichaIng commented Oct 19, 2024

Here is the shell script and systemd unit: https://github.com/motioneye-project/motioneye/tree/dev/motioneye/extra

I was wondering as well what the Python script purpose is. At least it is installed into PATH. However now it comes in handy, since it assures the shell script is executed within the venv, so that we can know the location of meyectl.

The Makefile is for localization only. It is indeed misleading there. I already wanted to move it into the l10n dir. But workflows need to be adjusted as well then.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

2 participants