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

venv activation #17

Closed
wants to merge 24 commits into from
Closed

Conversation

AlvinSchiller
Copy link
Owner

@AlvinSchiller AlvinSchiller commented Jan 31, 2024

POC für eine mögliche Strukurierung. Bisher keinerlei Test der angepassten skripte!
Wie im Kommentar hier beschrieben MiczFlor#2144 (comment)

  • run*.py skripte in die fachlichen Ordner verschoben. Vorerst rückgängig gemacht
    • Falls man die run skripte in unterordner haben will, müsste man den python import anpassen, sodass die gesamte struktur verfügbar ist. Eine möglichkeit wäre z.B. eine installation mit pip (https://stackoverflow.com/a/50194143). Damit müssten aber evtl alle imports angepasst werden.
  • shell skripte für die Aufrufe der python skripte inkl venv + workingdir handling hinzugefügt.
    • platzierung in den fachlichen Bereichen z.B.
      /installation/components für rfid reader und audio
      /tools für rpc und sniffer
      ./ für jukebox

@coveralls
Copy link

coveralls commented Jan 31, 2024

Pull Request Test Coverage Report for Build 7738986943

  • 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 7717240653: 0.0%
Covered Lines: 418
Relevant Lines: 1089

💛 - Coveralls

@AlvinSchiller AlvinSchiller marked this pull request as draft January 31, 2024 14:13
documentation/developers/coreapps.md Outdated Show resolved Hide resolved
documentation/developers/coreapps.md Show resolved Hide resolved
installation/routines/setup_rfid_reader.sh Show resolved Hide resolved
src/jukebox/components/volume/run_configure_audio.py Outdated Show resolved Hide resolved
tools/run_publicity_sniffer.sh Outdated Show resolved Hide resolved
installation/components/setup_audio_sink.sh Outdated Show resolved Hide resolved
Copy link

@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.

Progressive change, but I like the idea a lot! +1
This change requires a proper update mechanism, though. Otherwise you won't be able to execute this. So maybe this is not something for v3.5

Comment on lines 2 to 16

# Runner script to ensure
# - correct venv activation
# - independent from working directory

# Change working directory to project root
SOURCE=${BASH_SOURCE[0]}
SCRIPT_DIR="$(dirname "$SOURCE")"
PROJECT_ROOT="$SCRIPT_DIR"/../..
cd "$PROJECT_ROOT" || { echo "Could not change directory"; exit 1; }

source .venv/bin/activate || { echo "ERROR: Failed to activate virtual environment for python"; exit 1; }

cd src/jukebox || { echo "Could not change directory"; exit 1; }
python run_configure_audio.py $@
Copy link

Choose a reason for hiding this comment

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

I see this code a few times. Could it be abstracted?

Copy link
Owner Author

Choose a reason for hiding this comment

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

not very much sadly.
If we wanted to source another script, we still need the calculation for the project root, to access it.
And after that its only the venv activation. This could be extracted if we bother, to make future changes easier, but don't know if its worth.

src/jukebox/run_register_rfid_reader.py Outdated Show resolved Hide resolved
documentation/developers/coreapps.md Outdated Show resolved Hide resolved
src/jukebox/run_configure_audio.py Outdated Show resolved Hide resolved
@AlvinSchiller
Copy link
Owner Author

Progressive change, but I like the idea a lot! +1 This change requires a proper update mechanism, though. Otherwise you won't be able to execute this. So maybe this is not something for v3.5

Yes, i would also see this for the v3.6.0 release.

As you state it, i think few have a problem with the current update steps already for v3.5.0 -> MiczFlor#2211 (comment)

@AlvinSchiller
Copy link
Owner Author

AlvinSchiller commented Feb 1, 2024

Warum eigentlich englisch... ist ja ein PR auf meinem Repo :D

Wenn ihr sagt das Vorgehen passt und das sollten wir weiter verfolgen, würde ich die Änderungen auf den ursprünglichen PR MiczFlor#2144 commiten, damit weitere Kommentare da getrackt werden und erhalten bleiben.
Das hier war erstmal nur als POC damit ihr es euch ansehen könnt

@AlvinSchiller
Copy link
Owner Author

Wie seht ihr übrigends den Punkt? Sollte man es ermöglichen die Run Skripte auch in den Unterordner zu haben? Grundsätzlich wäre das wohl erstrebenswert, um ne klare struktur zu haben, und evtl auch mehr in python skripten zu entwickeln. Wäre natürlich schon etwas aufwand, und ich bin da in python auch nicht tief drin, um das genau zu bewerten.

  • run*.py skripte in die fachlichen Ordner verschoben. Vorerst rückgängig gemacht
    • Falls man die run skripte in unterordner haben will, müsste man den python import anpassen, sodass die gesamte struktur verfügbar ist. Eine möglichkeit wäre z.B. eine installation mit pip (https://stackoverflow.com/a/50194143). Damit müssten aber evtl alle imports angepasst werden.

updated docs path.
@s-martin
Copy link
Collaborator

s-martin commented Feb 1, 2024

Wie seht ihr übrigends den Punkt? Sollte man es ermöglichen die Run Skripte auch in den Unterordner zu haben? Grundsätzlich wäre das wohl erstrebenswert, um ne klare struktur zu haben, und evtl auch mehr in python skripten zu entwickeln. Wäre natürlich schon etwas aufwand, und ich bin da in python auch nicht tief drin, um das genau zu bewerten.

  • run*.py skripte in die fachlichen Ordner verschoben. Vorerst rückgängig gemacht

    • Falls man die run skripte in unterordner haben will, müsste man den python import anpassen, sodass die gesamte struktur verfügbar ist. Eine möglichkeit wäre z.B. eine installation mit pip (https://stackoverflow.com/a/50194143). Damit müssten aber evtl alle imports angepasst werden.

Wäre sicher sauberer strukturiert, aber ich denke das könnte man auch noch später machen, falls wir jetzt noch keine Lösung dafür haben

@AlvinSchiller
Copy link
Owner Author

closed
PR für main repo erstellt -> MiczFlor#2233

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.

4 participants