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

Update docs for venv #2144

Closed

Conversation

AlvinSchiller
Copy link
Collaborator

  • update docs for venv
  • fixed some links
  • harmonized paths

@AlvinSchiller AlvinSchiller requested a review from pabera December 10, 2023 00:41
Copy link
Collaborator

@pabera pabera Dec 10, 2023

Choose a reason for hiding this comment

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

I like the idea you have here but using ./run_python.sh (or ../../run_python.sh in some cases) is not very intuitive. If we could add this content to the .*rc file of the Pi during installation, this will reduce the problems when people try to debug. I could foresee a common pattern where the first answer to an issue will be: "Did you enable venv". The same could be done within the Dockerfile

For developers, if you wanna run the those files on your Windows, Mac or Linux, this could be explained as prerequisite to develop. We could still have this file available to enable venv to run once before starting development.

Thoughts?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I didn't like the need for ../../run_python.sh aswell, but run_jukebox.sh needs to be run from its current folder, otherwise all relativ paths are broken, what i just realized in the last tests...

I wouldn't add this to the .*rc or any system startup though, as this would make the venv somehow useless if its always activated and used for everything. Then we could have kept installing to the system.

I just realized its possible to directly use the python from the venv ~/RPi-Jukebox-RFID/.venv/bin/python ./run_XXX.py. This would run the script also in the context of the venv and makes the additional run_python.sh script obsolete and also any activation. But its still rather clunky to use, and can be easily forgotten.

A more convenient way would be to make all relevant scripts in the project, the user is supposed to run manually, handle the activation be itself. For bash scripts that run python commands this is simple (see above), but a problem are the python scripts, as these would need some non trivial checks and activation code in every script before importing libs.
This alternatively could be handled by defining the shebang like #!/home/pi/RPi-Jukebox-RFID/.venv/bin/python but the full path must be given and there is no subtitution possible (so again a harcoded userhome path which makes it not an option).

Maybe another way to handle this, would be to not let the use call any python scripts direclty, but supply proxy bash scripts, e.g. one for every python script.
These scripts could be gathered in a subfolder of the project root and care for any precondition the python scripts needs, like running with the correct venv or switching the folder for run_jukebox.sh.
This way the user can easily access all configuration scripts, and we can make sure that all preconditions are set. Also the python script dont need a shebang anymore and are easier to mainain, as moving or renaming wouldn't reflect to the docs or the user directly.

Thoughts? ^^

Copy link
Collaborator

@pabera pabera Dec 11, 2023

Choose a reason for hiding this comment

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

I wouldn't add this to the .*rc or any system startup though, as this would make the venv somehow useless if its always activated and used for everything. Then we could have kept installing to the system.

I understand. It does not sound too problematic to me though because once installed on a Pi (let's say in a builder scenario), the Jukebox is the only purpose of this device anyways. I don't see why people want to use different Python venvs on their production Phoniebox (and if some people want to do this, they know how to handle venvs).

I can see why adding to .*rc by default might be over the top. Instead, we can add an information message whenever you login via SSH.

Hi there!

In order to run Phoniebox scripts on this Pi, you need to activate a Python virtual environment. 

Just run the following command

$ source ~/RPi-Jukebox-RFID/.venv/bin/activate

If you don't want to be bothered with this in the future, automate it

$ ./automate_venv_activation.sh

Enjoy!

Copy link
Collaborator

Choose a reason for hiding this comment

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

We could then add a bit more information to the new Python.md file explaining the issue.

Then we could have kept installing to the system.

Unlikely, because it was a requirement for Bookworm and it's anyways the proposed usage of Python going forward. We just need to educate the builders and developers properly to reduce churn while debugging.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I can see why adding to .*rc by default might be over the top. Instead, we can add an information message whenever you login via SSH.

How about a different approach:
Make the generation of the rc file a question at installation and make the default answer "yes, create the file".

Advanced users can disable the generation of the rc file and regular users should just do it (as the phoniebox does nothing else anyway).

Copy link
Collaborator

@s-martin s-martin Dec 27, 2023

Choose a reason for hiding this comment

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

How do we want to proceed with this PR?

I agree that the existing scripts (run_jukebox etc .) should handle the venv stuff themselves, so regular users don't even need to know about venvs :)

For more advanced users/developers I think we should make this in docs clear.

What do you think?

I think this PR needs to be rebased, too.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I will into this again and evaluate the possible solutions and make a suggestion.
Didn't had the time so far.
Hope i have some free time the next days.

Copy link
Collaborator

Choose a reason for hiding this comment

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

No hurry 😃

Copy link
Collaborator

Choose a reason for hiding this comment

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

How about a different approach:
Make the generation of the rc file a question at installation and make the default answer "yes, create the file".
Advanced users can disable the generation of the rc file and regular users should just do it (as the phoniebox does nothing else anyway).

I wanted to get back to this. Most people won't understand what this means, hence it does not make sense to ask the questions.

My suggestion is:

  1. When a user ssh's into the Pi, we show a message about venv. (like above).
  2. We allow them to automate this in *.rc file
  3. If automated, we show a message on ssh login saying Phoniebox default venv enabled

This combines the less intrusive approach @AlvinSchiller was looking for allowing user to automate it, if they like it (like me :-))

Copy link
Collaborator

Choose a reason for hiding this comment

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

I like this approach

@AlvinSchiller
Copy link
Collaborator Author

AlvinSchiller commented Dec 11, 2023

I added the not venv and python related doc updates in a new PR, so they could be merged.

@AlvinSchiller AlvinSchiller changed the title Update docs Update docs for venv Dec 11, 2023
@AlvinSchiller AlvinSchiller marked this pull request as draft December 11, 2023 11:57
@AlvinSchiller AlvinSchiller added the future3 Relates to future3 development label Dec 21, 2023
@AlvinSchiller AlvinSchiller self-assigned this Jan 5, 2024
@pabera pabera added this to the v3.5 milestone Jan 16, 2024
@pabera pabera mentioned this pull request Jan 30, 2024
13 tasks
@AlvinSchiller
Copy link
Collaborator Author

AlvinSchiller commented Feb 1, 2024

follow up PR with different approach -> #2233

@AlvinSchiller AlvinSchiller deleted the future3/fix/docs branch February 6, 2024 08:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
future3 Relates to future3 development
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants