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

installation: untar libzmq in venv folder instead of /usr/local #2305

Conversation

notapirate
Copy link
Contributor

  • on a linux-host docker build, the user can't write to /usr/local and installing pyzmq fails
  • untar libzmq to venv instead
  • normal installation also changed because it's sensible to have all pyzmq dependencies in the venv
  • jukebox.Dockerfile: removing ZMQ_PREFIX=${PREFIX} since PREFIX variable doesn't exist in docker environment
  • jukebox.Dockerfile: use ENV to set ZMQ_DRAFT_API

@notapirate notapirate force-pushed the feature/untar-libzmq-to-venv branch from 227795b to 8e74ebe Compare March 23, 2024 10:59
- on a linux-host docker build, the user can't write to /usr/local and installing pyzmq fails
- normal installation also changed because it's sensible to have all pyzmq dependencies in the venv
- this also makes the separate 'sudo rsync' command obsolete
- jukebox.Dockerfile: removing ZMQ_PREFIX=${PREFIX} since PREFIX variable doesn't exist in docker environment
- jukebox.Dockerfile: use ENV to set ZMQ_DRAFT_API
@notapirate notapirate force-pushed the feature/untar-libzmq-to-venv branch from 8e74ebe to 54d3baa Compare March 23, 2024 11:26
@AlvinSchiller
Copy link
Collaborator

Hi, thanks for your contribution.

My thoughts about this:

  • To solve access rights in the docker build, the libzmq download and unpacking has to be done before the user is set.
  • libzmq is not a python lib and so it does not belong in the venv, in my opinion. A venv is also designed to be reproducable by removing and reinstalling all python modules, which libzmq cant fullfill. The recommended installation for libzmq is via a package installer, but as we need to build it ourself, "/usr/local/" is the correct folder to place it.
    IF we would decide to not put it on system level, i would recommend to place it in a new folder in the project root, like .libs, but i am not aware of any problems with the current solution.
  • Theres already a PR to address some problems with the docker setup (Fix Docker for pyzmq #2167), so i would suggest to bring this together (i think the problem with the access right is an important hint, which was missed there.

@notapirate
Copy link
Contributor Author

notapirate commented Mar 24, 2024

* libzmq is not a python lib and so it does not belong in the venv, in my opinion. A venv is also designed to be reproducable by removing and reinstalling all python modules, which libzmq cant fullfill. The recommended installation for libzmq is via a package installer, but as we need to build it ourself, "/usr/local/" is the correct folder to place it.
  IF we would decide to not put it on system level, i would recommend to place it in a new folder in the project root, like `.libs`, but i am not aware of any problems with the current solution.

I'm not a python expert, so if you say that's something one doesn't do that's fine with me. My reasoning was that libzmq, though not a python package, is a python package dependency, but I haven't found a clear rule/suggestion on the web whether non-python dependencies should be put in the venv or not, so I'll just use your suggestion to untar as root to /usr/local in docker and leave the normal installation unchanged.

* Theres already a PR to address some problems with the docker setup ([Fix Docker for pyzmq #2167](https://github.com/MiczFlor/RPi-Jukebox-RFID/pull/2167)), so i would suggest to bring this together (i think the problem with the access right is an important hint, which was missed there.

OK so how does this work, I make the changes and create a merge request with MR #2167?

@AlvinSchiller
Copy link
Collaborator

OK so how does this work, I make the changes and create a merge request with MR #2167?

@pabera has created a new PR #2307, where all relevant changes from the different PRs will be gathered.
Feel free to test this change and give feedback :)

@notapirate
Copy link
Contributor Author

notapirate commented Mar 26, 2024

PR #2307 works and fixes this issue, so I'm closing this PR, thanks @AlvinSchiller for your time to review this PR.

@notapirate notapirate closed this Mar 26, 2024
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.

2 participants