Skip to content

feat(qmk): made the install script safer on rerun. #81

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

feat(qmk): made the install script safer on rerun. #81

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
79 changes: 74 additions & 5 deletions qmk/arsenik-qmk.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,8 @@ if [ "$#" -lt 1 ]; then
echo "$0 <keyboard_name> [-n] [-b,-f]"
echo " # Install Arsenik for your keyboard, opens your editor to edit the config"
echo ' # -n: no editor: doesn’t open the config with your $EDITOR'
echo " # -b: build the newly created keymap (after you edited the config)"
echo " # -f: build the newly created keymap (after you edited the config) then flash"
echo " # -c: compile the newly created keymap (after you edited the config)"
echo " # -f: compile the newly created keymap (after you edited the config) then flash"
Copy link
Contributor

@AkiroPhi AkiroPhi Mar 19, 2025

Choose a reason for hiding this comment

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

May be you can do something like
-f: flash the newly created keymap and compile it (if needed ?) after you edited the config
At first reading, we can misunderstand the use of -f and precise flash after -f is more clear according to me

exit 0
fi

Expand Down Expand Up @@ -39,6 +39,74 @@ function get_keymaps_folder() {
echo "$QMK_PATH/keyboards/$keyboard_name/keymaps"
}

function handle_keymap_conflict() {
local arsenik_folder="$1"
local script_name=$(basename $0)

echo -e "\x1b[1;31mError :\x1b[0m The Arsenik keymap already exists for this keyboard."
echo -e "$script_name should \x1b[4mnot\x1b[0m be used to recompile your keymap, this is an \x1b[4minstall\x1b[0m script."
echo "This is because some keyboards may need extra info to properly compile or flash the"
echo "firmware (like if you need to specify the bootloader)"
echo

local old_arsenik_keymap_folder="${arsenik_folder::-1}"
local old_arsenik_keymap_id=1

while [ -d "$old_arsenik_keymap_folder"_"$old_arsenik_keymap_id" ]; do
old_arsenik_keymap_id=$((old_arsenik_keymap_id + 1))
done

local keymap_backup_folder="$old_arsenik_keymap_folder"_"$old_arsenik_keymap_id"

echo "If you want to reinstall the Arsenik keymap, your old keymap will be moved to this folder:"
echo " \`$keymap_backup_folder\`"
echo

echo "If you want to recompile and flash your keymap, you should do so with the commands provided by QMK"
echo " Using the QMK CLI: \`qmk flash -kb $keyboard_name -km arsenik\`"
echo " (docs: https://docs.qmk.fm/cli_commands)"
echo " Using the \`make\` command: \`make $keyboard_name:arsenik:flash\` run in $QMK_PATH"
echo " (docs: https://docs.qmk.fm/getting_started_make_guide)"
echo
echo "Note: Those commands may not work out of the box, some keyboards need extra cmdline arguments"
echo "(for instance if you need to specify the bootloader explicitely)"
echo "If you have an error, check the docs mentionned above."
echo

echo "What do you want to do?"
COLUMNS=1 # Display options from select in a single column
select cmd in \
"Backup keymap and reinstall Arsenik" \
"Recompile and flash keymap with the QMK CLI" \
"Recompile and flash keymap with \`make\`" \
"Abort"
do
case $cmd in
"Backup keymap and reinstall Arsenik")
mv "$arsenik_folder" "$keymap_backup_folder"
return 0
;;
"Recompile and flash keymap with the QMK CLI")
cd "$QMK_PATH"
make "$keyboard_name":arsenik:flash
return 1
;;
"Recompile and flash keymap with \`make\`")
qmk flash -kb $keyboard_name -km arsenik
return 1
;;
"Abort")
echo "Aborting."
return 1
;;
*)
echo "Unknown command chosen. Aborting."
return 1
;;
Comment on lines +89 to +105
Copy link
Contributor

@AkiroPhi AkiroPhi Mar 19, 2025

Choose a reason for hiding this comment

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

Should be the return code used ? If so why returning 1when aborting, and doing make, and doing qmk flash ?
Is it possible to return the code of previous command ?

Copy link
Contributor Author

@Nuclear-Squid Nuclear-Squid Mar 20, 2025

Choose a reason for hiding this comment

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

Yeah, I wanted the function to be able to tell if we should still reinstall the keymap after this case statement (see this comment), but it’s kinda dumb. If you have a better idea to return a status from this function, I’m all ears.

esac
done
}

function make_new_arsenik_keymap() {
local keyboard_name="$1"
local qmk_cmd="$2"
Expand All @@ -48,8 +116,9 @@ function make_new_arsenik_keymap() {
local arsenik_folder="$keymap_folder/arsenik/"
local default_keymap_folder="$keymap_folder/default"

[ -d "$arsenik_folder" ] && handle_keymap_conflict "$arsenik_folder"

Comment on lines +119 to +120
Copy link
Contributor

@AkiroPhi AkiroPhi Mar 19, 2025

Choose a reason for hiding this comment

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

The return code of handle_keymap_conflict is not used.
Is it ok ? commit seems to prevent rerun but aborting don’t abort the execution ?

Copy link
Contributor Author

@Nuclear-Squid Nuclear-Squid Mar 20, 2025

Choose a reason for hiding this comment

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

It is but in a pretty roundabout way. Basically, the set -euo pipefail at the start of the file makes the whole script exit with an error if one command returns an error. So if the exit code from handle_keymap_conflict is not 0, then the script will exit out (and thus, not reinstall the keymap).

I thought it was a bit dirty but maybe fine ? But if you don’t get it then it’s not clear enough and should be fixed.

cp -r "$default_keymap_folder" "$arsenik_folder"
ls -l "$arsenik_folder"

local layout=""
case $(ls "$default_keymap_folder"/keymap.* | sed 's/.*\(keymap.*\)/\1/') in
Expand Down Expand Up @@ -78,7 +147,7 @@ function make_new_arsenik_keymap() {

case "$qmk_cmd" in
"none") ;;
"build") cd "$QMK_PATH" && make "$keyboard_name:arsenik";;
"compile") cd "$QMK_PATH" && make "$keyboard_name:arsenik";;
"flash") cd "$QMK_PATH" && make "$keyboard_name:arsenik:flash";;
esac
}
Expand All @@ -89,7 +158,7 @@ qmk_cmd="none"
for arg in "${@:2}"; do
case "$arg" in
"-n") no_editor=true;;
"-b") qmk_cmd="build";;
"-c") qmk_cmd="compile";;
"-f") qmk_cmd="flash";;
*) echo "Unknown argument $arg."; exit 1;;
esac
Expand Down
2 changes: 1 addition & 1 deletion qmk/arsenik_config.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,7 @@


// Lists of layouts supported by Arsenik. Some parts of the config are dependent
// on keyboard layout used on your computer. If they don’t match up some
// on the keyboard layout used on your computer. If they don’t match up some
// characters may not be correctly placed or missing entirely. If multiple
// options are toggled at the same time, the first one is chosen.
#define ARSENIK_HOST_LAYOUT_QWERTY
Expand Down