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.

Sign up for GitHub

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

Jump to bottom

fix: Use correct raspi-config command for bookworm #2450

Merged
merged 7 commits into from
Nov 7, 2024
Merged

fix: Use correct raspi-config command for bookworm #2450

merged 7 commits into from
Nov 7, 2024

Conversation

s-martin
Copy link
Collaborator

@s-martin s-martin commented Nov 3, 2024 •
edited
Loading

@s-martin s-martin added bug needs testing ext dependency future3 Relates to future3 development labels Nov 3, 2024
@s-martin s-martin added this to the v3.6 milestone Nov 3, 2024
@s-martin s-martin linked an issue Nov 3, 2024 that may be closed by this pull request
RDM6300 configuration bug #2444
Open
@s-martin
Copy link
Collaborator Author

s-martin commented Nov 3, 2024

@Darkcast you could test this fix, if you want

pabera
pabera reviewed Nov 3, 2024
View reviewed changes
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.

Semantically, this is a great approach. Thanks for adding this function.

To be consistent, I wonder if we want to update the below function as well (in 02_helpers.sh). It would even reduce the code. Also, there is a condition that would never be reached.

RPi-Jukebox-RFID/installation/includes/02_helpers.sh

Lines 91 to 108 in 027ec29

_get_boot_file_path() {
local filename="$1"
if [ "$(is_raspbian)" = true ]; then
local debian_version_number=$(get_debian_version_number)
# Bullseye and lower
if [ "$debian_version_number" -le 11 ]; then
echo "/boot/${filename}"
# Bookworm and higher
elif [ "$debian_version_number" -ge 12 ]; then
echo "/boot/firmware/${filename}"
else
echo "unknown"
fi
else
echo "unknown"
fi
}

To be changed to this

_get_boot_file_path() {
    local filename="$1"
    if [ "$(is_raspbian)" = true ]; then
        if [ "$(is_bookworm_or_higher)" = true ]; then
            echo "/boot/firmware/${filename}"
        else
            echo "/boot/${filename}"
        fi
    else
        echo "unknown"
    fi
}

@AlvinSchiller
Copy link
Collaborator

I'm fine with the changes basically, but they collide with changes from #2425 .

Suggestion: merge #2425 first and integrate here.

I will also post a suggestion later.

@AlvinSchiller
Copy link
Collaborator

Merged future3/develop and applied changes.

restored previous functions for debian checks.
Introduced a generic function "is_debian_version_at_least" to test against a version number (may be used in the future).
This brings more flexibility then static version checks (or multiple functions for every version).

Example:

if [ "$(is_debian_version_at_least 12)" = true ]; then
    commands for debian 12 and higher
elif [ "$(is_debian_version_at_least 11)" = true ]; then
    commands for debian 11 (as previous check was false)
else
     commands for earlier debian version or no debian distro at all
end

s-martin
s-martin commented Nov 7, 2024
View reviewed changes
Copy link
Collaborator Author

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

👍🏼

@s-martin s-martin merged commit bdc1a23 into future3/develop Nov 7, 2024
31 checks passed
@s-martin s-martin deleted the future3/fix_disable_bookworm branch November 7, 2024 21:26
This was referenced Nov 7, 2024
Open
Open
@s-martin s-martin changed the title Use correct raspi-config command for bookworm fix: Use correct raspi-config command for bookworm Nov 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pabera pabera pabera left review comments

@AlvinSchiller AlvinSchiller Awaiting requested review from AlvinSchiller

Assignees
No one assigned
Labels
bug ext dependency future3 Relates to future3 development needs testing
Projects
None yet
Milestone
v3.6
Development

Successfully merging this pull request may close these issues.

RDM6300 configuration bug
3 participants
@s-martin @AlvinSchiller @pabera