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

Added swap config on low-mem nodes #2063

Closed
wants to merge 11 commits into from

Conversation

The-Judge
Copy link

As discussed in #1815 (comment),

I added code which creates and activates a swap file if too few available memory is detected (less than 2 GB, which should be enough even for recent gst-plugin-spotify builds).

@s-martin
Copy link
Collaborator

s-martin commented Sep 5, 2023

Hi, thanks for the PR! 🎉

could you extend/update the existing install script buster-install-default.sh instead of adding a new file?
From a maintenance point of view we don’t want to add another install script for 2.x, because then we have to maintain two scripts.
Thanks!

I agree the current name buster (which also works for bullseye) is a little bit misleading, but we would change that later.

@The-Judge
Copy link
Author

Sure thing - here you go!

@s-martin
Copy link
Collaborator

s-martin commented Sep 5, 2023

Little bit more on the actual change:
Is the creation of a swap file just for compiling the gst-plugin?

@The-Judge
Copy link
Author

Little bit more on the actual change: Is the creation of a swap file just for compiling the gst-plugin?

More or less, yes. I don't know what else may fail in case of too less RAM. But the compile is definitely failing with 1 GB Pis.
Once everything is running, including samba, bluetooth, RFID daemom, gstreamer, lighthttpd, ... may fail with no RAM available, too.

@The-Judge
Copy link
Author

Sorry for those follow up commits - now it's done.

@The-Judge
Copy link
Author

This is done for 2 weeks now - what are we waiting for?

Copy link
Collaborator

@s-martin s-martin left a comment

Choose a reason for hiding this comment

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

Sorry to bother again.

Some CI checks fail, see https://github.com/MiczFlor/RPi-Jukebox-RFID/actions/runs/6606776086/job/17961018222#step:6:1

I think this is because you added an additional config parameter. Could you have a look at ./scripts/installscripts/tests/run_installation_tests3.sh?

Marc Richter and others added 2 commits October 23, 2023 19:57
…er dialogues

Made the yes/no sequence a bit easier to read

Added comments, clarifying which dialogue needs an
extra-Enter
@The-Judge The-Judge requested a review from s-martin October 23, 2023 18:00
@The-Judge
Copy link
Author

I added some changes to both, my script additions and the testfile, which read horribly and contained plain wrong comments.

I don't know why this does not re-trigger the test currently ...

@AlvinSchiller
Copy link
Collaborator

Hi, please add the changes from develop to the testfiile again (the line which calls the install Script). They haven't been merged entirely.

@The-Judge
Copy link
Author

Done

@AlvinSchiller
Copy link
Collaborator

I think you got me wrong. Now all the changes from develop are reverted.
Before only the changes in line 30 where missing.

@s-martin
Copy link
Collaborator

s-martin commented Nov 8, 2023

@The-Judge, I think there was some misunderstanding what @AlvinSchiller suggested (and there is a conflict).
Could you fix this, so we could merge this PR?

# In general: If less than 2 GB RAM is available in your system, the
# build of gst-plugin-spotify will most certainly fail.
#
# This script can detect if a swapfile is required and have one
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is the question below, before or after the necessity for a swap file is checked?


# Test installation
./test_installation.sh $INSTALLATION_EXITCODE
./scripts/installscripts/tests/test_installation.sh
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this change necessary?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thats part of the uncomplete merge and should be reverted

@s-martin s-martin added the legacy_v2 Issues, discussions and PRs related to Version 2.x label Dec 25, 2023
@s-martin
Copy link
Collaborator

s-martin commented Mar 2, 2024

#2064 may now be obsolete (see #2064 (comment)).

Therefor this PR would be obsolete as well.

@s-martin s-martin mentioned this pull request Apr 3, 2024
@AlvinSchiller
Copy link
Collaborator

Obsolete with implementation of #2315

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement legacy_v2 Issues, discussions and PRs related to Version 2.x spotify edition
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants