Further Optimizations #2092

AlvinSchiller · 2023-11-06T01:22:08Z

This introduces further optimizations for maintenance and test runtime.

moved apt package definition in packages*.txt file (like requirements.txt for pip). The install/setup and test scripts now use this files.
use requirements.txt in test script to determine installed packages to test
optimized check of installed packages in test script, decreasing check runtime from several minutes to 30s
Dockerfile now uses new apt packages*.txt files to preinstall internal packages. This results in lowered runtime of the single tests (around 10 - 15 min), as most packages are already installed. The baseimage is cached so the installation is not performed in subsequent runs for that branch (considering cache policy).
--- downside: image size in cache increases! from 300 MB to around 500 MB per debian version
save run specific image as artifact instead of using the cache (prevent cache trashing) (see Further Optimizations #2092 (comment))
correct variable usage
check homedir as prerequisite

Runtime summary:
after the first workflow run, which takes longer as it creates the docker image which is then cached, the subsequent runs finish in about 10 min (before around 25 min).

add comments in package step

test reads the same file. only one place to maintain

dont install resolvconf as it will fail in containers

coveralls · 2023-11-06T01:23:25Z

Pull Request Test Coverage Report for Build 6805930329

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 76.82%

Totals
Change from base Build 6804842786:	0.0%
Covered Lines:	633
Relevant Lines:	824

💛 - Coveralls

Groovylein

Looks good.

I currently don't have any setup available to test.

My suggestions are not blocking to integrate, but would make the code more readable.

Groovylein · 2023-11-06T16:23:17Z

scripts/helperscripts/setup_autohotspot.sh

+    shift
+
+    # read line from the file and remove comments. Pass it over xargs as arguments to the given command.
+    sed 's/#.*//g' ${package_file} | xargs "$@"


Isn't it better understandable, if you call here the apt-get command instead of using the passed arguments?!

I mean:
sed 's/#.*//g' ${package_file} | xargs ${apt_get} install

The name of the function already implies, that something with apt will happen

yep could be done. I just wanted it to be in sync with the install and test script (see below) to not have the same logic solved in multiple ways.

renamed the function to call_with_args_from_file and added a function usage comment

Groovylein · 2023-11-06T16:25:15Z

scripts/installscripts/buster-install-default.sh

+    shift
+
+    # read line from the file and remove comments. Pass it over xargs as arguments to the given command.
+    sed 's/#.*//g' ${package_file} | xargs "$@"


Isn't it better understandable, if you call here the apt-get command instead of using the passed arguments?!

I mean:
sed 's/#.*//g' ${package_file} | xargs ${apt_get} install

The name of the function already implies, that something with apt will happen

I didn't want to define apt_get and allow_downgrades again as these are still used in the install_main function. And making them script wide available doesn't seem right either, as its only used in install_main.

The function body is used the same way in the test_installation script, only with the echo command.

Inthe test_installation script it named it "apt" mainly to distinguish from the function for the "pip" packages and carried over the name.

Shell scripts being shell scripts, maybe document the expected parameter(s) and that one should call this with a variant of sudo apt get install or something else that can take 1 parameter that takes a package name? (Obvious example: echo would just print the package and do nothing, but equally valid)

renamed the function to call_with_args_from_file and added a function usage comment

Groovylein · 2023-11-06T16:29:05Z

scripts/installscripts/buster-install-default.sh

@@ -327,7 +334,7 @@ check_existing() {

    # The install will be in the home dir of user pi
    # Move to home directory now to check
-    cd ~ || exit
+    cd "${home_dir}"


I think this is not your fault, but using now the same name for a local variable is confusing.

I would rename it within the function to local_home_diror something similar

i just used the already existing variable home_dir to get rid of ~.

But i get your point. I renamed it to local_home_dir

Groovylein · 2023-11-06T16:31:39Z

scripts/installscripts/tests/test_installation.sh

+    shift
+
+    # read line from the file and remove comments. Pass it over xargs as arguments to the default echo command.
+    sed 's/#.*//g' ${package_file} | xargs "$@"


Isn't it better understandable, if you call here the apt-get command instead of using the passed arguments?!

I mean:
sed 's/#.*//g' ${package_file} | xargs ${apt_get} install

The name of the function already implies, that something with apt will happen

I named it "apt" mainly to distinguish from the function for the "pip" packages.
Also the command given here is not apt-get install but echo, as the test only reads the file content.

Oh right. That was just a copy paste fail.

merged "pip" and "apt" function into one. renamed the function to call_with_args_from_file and added a function usage comment

AlvinSchiller · 2023-11-07T22:29:10Z

I would make an additional change:
Currently the final docker images for the testjobs are also put in the cache, but they are only valid for a single workflow run (see #2082). With the additional file size and the soon coming bookworm test, this will most likely cause cache trashing (to many new items so the older but still need ones are already purged, causing a new creation of items). The explicit deletion of cache items is possible but not for pull request as the needed write permissions can't be granted (what makes this approach unusable).

The proposed change:
The final images are not put in the cache put uploaded as workflow artifacts. This does not count to the cache limits.
A new clean up job after the tests will delete this artifacts if the run was successful or cancelled.
If the tests failed the artifacts will be kept with a retention time of 2 days (possible values: 1-90 days). This makes reruns of single failured test possible in this period.
I didn't find any documentation about a maximum storage limit for artifact though.

This strategie will take a few minutes longer to run (upload) but is much safer in regards of health and time saving from using the cache properly.

s-martin · 2023-11-08T21:41:50Z

@AlvinSchiller, I just mergend #2084, so now your PR has some conflicts. Can you fix this?

AlvinSchiller · 2023-11-08T22:14:27Z

Fixed also the mpd service enablement for Spotify Installations ->
#2084 (comment)

Develop: https://github.com/MiczFlor/RPi-Jukebox-RFID/actions/runs/6804326866/job/18501808766#step:6:2496

s-martin · 2023-11-08T22:39:06Z

Sorry, I think the merge of the docs PR caused also conflicts again.

* optimize check for installed packages (apt, pip) * fix jukebox_dir * remove package optimization from dockerfile add comments in package step * add package installation from file test reads the same file. only one place to maintain * install internal packages in Dockerfile dont install resolvconf as it will fail in containers * added textfile for arm sepcific packages * removed as already part of install script * add package textfile for autohotspot setup * correct variable usage. * check homedir as prerequisite * some fixes * read modules to test from file * renamed package txt for raspberry pi specifics * update sed call * renamed local variable * save images as artifact * clarify function name. add function comment * fixed mpd service enablement * fix unnecessary rebuild of unrelated package files

AlvinSchiller added 13 commits November 5, 2023 23:44

optimize check for installed packages (apt, pip)

9dc697a

fix jukebox_dir

9f89135

remove package optimization from dockerfile

d39b32b

add comments in package step

add package installation from file

9fcdaa6

test reads the same file. only one place to maintain

install internal packages in Dockerfile

180a97a

dont install resolvconf as it will fail in containers

added textfile for arm sepcific packages

65d2022

removed as already part of install script

73b0d2b

add package textfile for autohotspot setup

3e24115

correct variable usage.

7124e63

check homedir as prerequisite

6760439

some fixes

b8b0043

read modules to test from file

21ffd16

renamed package txt for raspberry pi specifics

739d3cb

update sed call

850eaf8

s-martin added the enhancement label Nov 6, 2023

AlvinSchiller force-pushed the feature/optimizations branch from 649d42e to 850eaf8 Compare November 6, 2023 14:55

AlvinSchiller marked this pull request as ready for review November 6, 2023 15:08

AlvinSchiller requested a review from Groovylein as a code owner November 6, 2023 15:08

Groovylein approved these changes Nov 6, 2023

View reviewed changes

AlvinSchiller mentioned this pull request Nov 6, 2023

Make changes for Raspberry Pi OS Bookworm #2084

Merged

3 tasks

renamed local variable

5dc4b71

AlvinSchiller added 2 commits November 8, 2023 02:44

save images as artifact

ecea84c

clarify function name. add function comment

3193be1

AlvinSchiller added 2 commits November 8, 2023 23:01

Merge branch 'develop' into feature/optimizations

94aaa8c

fixed mpd service enablement

0f31a5d

AlvinSchiller added 2 commits November 9, 2023 00:08

Merge branch 'develop' into feature/optimizations

e0e20b6

fix unnecessary rebuild of unrelated package files

e8ad5bd

s-martin added this to the 2.5 milestone Nov 9, 2023

s-martin merged commit f610e5e into MiczFlor:develop Nov 9, 2023
33 of 34 checks passed

AlvinSchiller deleted the feature/optimizations branch November 9, 2023 06:47

AlvinSchiller mentioned this pull request Nov 20, 2023

Add installation checks + CI Workflow #2119

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Further Optimizations #2092

Further Optimizations #2092

AlvinSchiller commented Nov 6, 2023 •

edited

Loading

coveralls commented Nov 6, 2023 •

edited

Loading

Groovylein left a comment

Groovylein Nov 6, 2023

AlvinSchiller Nov 6, 2023 •

edited

Loading

AlvinSchiller Nov 8, 2023

Groovylein Nov 6, 2023

AlvinSchiller Nov 6, 2023

DivineDominion Nov 8, 2023

AlvinSchiller Nov 8, 2023

Groovylein Nov 6, 2023

AlvinSchiller Nov 6, 2023

Groovylein Nov 6, 2023

AlvinSchiller Nov 6, 2023

Groovylein Nov 7, 2023

AlvinSchiller Nov 8, 2023 •

edited

Loading

AlvinSchiller commented Nov 7, 2023 •

edited

Loading

s-martin commented Nov 8, 2023

AlvinSchiller commented Nov 8, 2023

s-martin commented Nov 8, 2023

Further Optimizations #2092

Further Optimizations #2092

Conversation

AlvinSchiller commented Nov 6, 2023 • edited Loading

coveralls commented Nov 6, 2023 • edited Loading

Pull Request Test Coverage Report for Build 6805930329

💛 - Coveralls

Groovylein left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlvinSchiller Nov 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlvinSchiller Nov 8, 2023 • edited Loading

Choose a reason for hiding this comment

AlvinSchiller commented Nov 7, 2023 • edited Loading

s-martin commented Nov 8, 2023

AlvinSchiller commented Nov 8, 2023

s-martin commented Nov 8, 2023

AlvinSchiller commented Nov 6, 2023 •

edited

Loading

coveralls commented Nov 6, 2023 •

edited

Loading

AlvinSchiller Nov 6, 2023 •

edited

Loading

AlvinSchiller Nov 8, 2023 •

edited

Loading

AlvinSchiller commented Nov 7, 2023 •

edited

Loading