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

included venv activation for python scripts #2233

Merged

Conversation

AlvinSchiller
Copy link
Collaborator

@AlvinSchiller AlvinSchiller commented Feb 1, 2024

New PR for the venv activation problem adressed in #2144
This implements the ideas from #2144 (comment)

  • have "proxy" shell scripts that handle the venv activation and setup the needed working dir, to be run from any relativ path.
  • better accessability of the scripts for the user, e.g. group with alike scripts like installation or tools
  • run_*.py scripts can't be moved right now, as the module imports are not accessable from sub dirs, so they have to stay in the root folder. This could be a future addition (possible solution https://stackoverflow.com/a/50194143)

@AlvinSchiller AlvinSchiller added the future3 Relates to future3 development label Feb 1, 2024
@coveralls
Copy link

coveralls commented Feb 1, 2024

Pull Request Test Coverage Report for Build 7769607281

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 38.384%

Totals Coverage Status
Change from base Build 7748821301: 0.0%
Covered Lines: 418
Relevant Lines: 1089

💛 - Coveralls

@AlvinSchiller AlvinSchiller added this to the v3.6 milestone Feb 1, 2024
@s-martin
Copy link
Collaborator

s-martin commented Feb 2, 2024

Would it be a problem, if a user manually activates a venv and then runs a sh script, which also activates the venv?

@AlvinSchiller
Copy link
Collaborator Author

Would it be a problem, if a user manually activates a venv and then runs a sh script, which also activates the venv?

Nope, tested multiple activations and couldn't see a sideeffect. The installations process does the same with the RFID Reader setup script.

Copy link
Collaborator

@pabera pabera left a comment

Choose a reason for hiding this comment

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

Looks good. In another iteration, a way to abstract the repetition of the individual bash files could be to build a single installer script with an argument.

$ run.sh rpc_tool
$ run.sh publicity_sniffer
$ run.sh register_rfid_reader

This could then prepare again for Whiptail 🙃

@AlvinSchiller
Copy link
Collaborator Author

Looks good. In another iteration, a way to abstract the repetition of the individual bash files could be to build a single installer script with an argument.

$ run.sh rpc_tool
$ run.sh publicity_sniffer
$ run.sh register_rfid_reader

This could then prepare again for Whiptail 🙃

Thought about that too 😁👍

@AlvinSchiller AlvinSchiller marked this pull request as ready for review February 2, 2024 08:46
@AlvinSchiller
Copy link
Collaborator Author

AlvinSchiller commented Feb 2, 2024

I will run some final test over this, otherwise ready to merge.

Edit: Do we then need the motd message at all (#2225)?

@pabera
Copy link
Collaborator

pabera commented Feb 2, 2024

Edit: Do we then need the motd message at all (#2225)?

Not for the reason of informing about the venv, but I truly believe we should keep this! 🚀

##################################################
   ___  __ ______  _  __________ ____   __  _  _
  / _ \/ // / __ \/ |/ /  _/ __/(  _ \ /  \( \/ )
 / ___/ _  / /_/ /    // // _/   ) _ ((  O ))  (
/_/  /_//_/\____/_/|_/___/____/ (____/ \__/(_/\_)

            Welcome to your Phoniebox
##################################################

@AlvinSchiller
Copy link
Collaborator Author

👍 i will update the Message to only have the banner :)

@AlvinSchiller AlvinSchiller force-pushed the future3/fix/venvdoc_shell branch from 388b1ac to a587bd8 Compare February 2, 2024 09:11
refactored into own postinstall script
@AlvinSchiller
Copy link
Collaborator Author

With this condition that updates are broken and a fresh installation is needed, should we add this already to v3.5.0?

@pabera
Copy link
Collaborator

pabera commented Feb 2, 2024

... should we add this already to v3.5.0?

Aligned!

@AlvinSchiller AlvinSchiller modified the milestones: v3.6, v3.5 Feb 2, 2024
@AlvinSchiller AlvinSchiller mentioned this pull request Feb 2, 2024
13 tasks
@AlvinSchiller
Copy link
Collaborator Author

Last fix included. Good to go, if nothing left.

@pabera
Copy link
Collaborator

pabera commented Feb 4, 2024

Merge it!

@AlvinSchiller AlvinSchiller merged commit b2a6517 into MiczFlor:future3/develop Feb 4, 2024
21 checks passed
@AlvinSchiller AlvinSchiller deleted the future3/fix/venvdoc_shell branch February 4, 2024 18:29
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.

4 participants