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

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

Merged
merged 7 commits into from
Nov 7, 2024

Conversation

s-martin
Copy link
Collaborator

@s-martin s-martin commented Nov 3, 2024

@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
@s-martin
Copy link
Collaborator Author

s-martin commented Nov 3, 2024

@Darkcast you could test this fix, if you want

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.

_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

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
@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
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RDM6300 configuration bug
3 participants