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

scripts: Kernel bumper script #14713

Merged
merged 1 commit into from Mar 11, 2024
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
225 changes: 225 additions & 0 deletions scripts/kernel_bump.sh
@@ -0,0 +1,225 @@
#!/bin/sh
# SPDX-License-Identifier: GPL-2.0-or-later
#
# Copyright (C) 2024 Olliver Schinagl <oliver@schinagl.nl>

set -eu
if [ -n "${DEBUG_TRACE_SH:-}" ] && \
[ "${DEBUG_TRACE_SH:-}" != "${DEBUG_TRACE_SH#*"$(basename "${0}")"*}" ] || \
[ "${DEBUG_TRACE_SH:-}" = 'all' ]; then
set -x
fi

REQUIRED_COMMANDS='
[
oliv3r marked this conversation as resolved.
Show resolved Hide resolved
basename
command
echo
exit
git
printf
set
shift
'

_msg()
{
_level="${1:?Missing argument to function}"
shift

if [ "${#}" -le 0 ]; then
echo "${_level}: No content for this message ..."
return
fi

echo "${_level}: ${*}"
}

e_err()
{
_msg 'err' "${*}" >&2
}

e_warn()
{
_msg 'warning' "${*}"
}

e_notice()
{
_msg 'notice' "${*}"
}

usage()
{
echo "Usage: ${0}"
echo 'Helper script to bump the target kernel version, whilst keeping history.'
echo " -p Optional Platform name (e.g. 'ath79' [PLATFORM_NAME]"
echo " -s Source version of kernel (e.g. 'v6.1' [SOURCE_VERSION])"
echo " -t Target version of kernel (e.g. 'v6.6' [TARGET_VERSION]')"
echo
echo 'All options can also be passed in environment variables (listed between [BRACKETS]).'
echo 'Example: scripts/kernel_bump.sh -p realtek -s v6.1 -t v6.6'
}

cleanup()
{
trap - EXIT HUP INT QUIT ABRT ALRM TERM

if [ -n "${initial_branch:-}" ] && \
[ "$(git rev-parse --abbrev-ref HEAD)" != "${initial_branch:-}" ]; then
git switch "${initial_branch}"
fi
}

init()
{
initial_branch="$(git rev-parse --abbrev-ref HEAD)"
initial_commitish="$(git rev-parse HEAD)"

trap cleanup EXIT HUP INT QUIT ABRT ALRM TERM
}

do_source_target()
{
_target_dir="${1:?Missing argument to function}"
_op="${2:?Missing argument to function}"

}

bump_kernel()
{
platform_name="${platform_name##*'/'}"

if [ -z "${platform_name:-}" ]; then
platform_name="${PWD##*'/'}"
fi

if [ "${PWD##*'/'}" = "${platform_name}" ]; then
_target_dir='./'
else
_target_dir="target/linux/${platform_name}"
fi

if [ ! -d "${_target_dir}/image" ]; then
e_err 'Cannot find target linux directory.'
exit 1
fi

git switch --force-create '__openwrt_kernel_files_mover'
Copy link
Contributor

Choose a reason for hiding this comment

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

Applies both to this line and several later ones. Quoting git help switch, "THIS COMMAND IS EXPERIMENTAL. THE BEHAVIOR MAY CHANGE.", uhm that seems a poor choice.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But checkout/branch don't have a nice force-create flag that works in the same way. Also, I was under the impression that switch was to replace checkout ...

Anyway, if it changes, we'll fix it when the need arises :)

It's not like this is a highly dependent upon, daily used script :p


for _path in "${_target_dir}/"*; do
if [ ! -s "${_path}" ] || \
[ "${_path}" = "${_path%%"-${source_version}"}" ]; then
continue
fi

_target_path="${_path%%"-${source_version}"}-${target_version}"
if [ -s "${_target_path}" ]; then
e_err "Target '${_target_path}' already exists!"
exit 1
fi

git mv \
"${_path}" \
"${_target_path}"
done
Comment on lines +111 to +126
Copy link
Contributor

Choose a reason for hiding this comment

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

If a device variant ends up growing a device-specific patches directory this will need updating. Given there are device-specific configurations, other kernel-version-dependent files showing up seems likely.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you mean? If you refer to @namiltd 's comment on sub-targets. Yeah that is probably needed.

If your talking about future, but not yet existent features, we should add them once the need arise, and not try to create a perfect script now, with all the features we can possibly imagine for all possible future usecases we don't know. Merge early, iterate often :D


find "${_target_dir}" -iname "config-${source_version}" | while read -r _config; do
_path="${_config%%"/config-${source_version}"}"
git mv "${_config}" "${_path}/config-${target_version}"
done
Comment on lines +111 to +131
Copy link
Contributor

Choose a reason for hiding this comment

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

Related to the above, these two loops have an odd similarity.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's true, thanks for pointing that out. I initially had just the for loop, but realized I missed config files.

I don't have in-depth knowledge of all targets. I know atheros and realtek (and generic I suppose) where things are simple (atheros) and a bit more complex (realtek).

Big difference though is the first loop looks only at the directories, and the find bit only at config files. So they are different.

Copy link
Contributor

Choose a reason for hiding this comment

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

I just realized, there is a very serious problem with the second loop. find, the shell executing the loop and git run in parallel. find is reading directories, git will be writing directories. Doing those at the same time generates undefined behavior. ext4 is a pretty friendly filesystem, but according to standards the filesystem can give find some weird data (ReiserFS was famous for doing unusual things in undefined cases).

I want to say the first loop should be okay, but I'm less than certain the shell is required to finish reading the directory before the loop starts executing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure, doesn't find run first, generate a list of files and pipes the whole list to the while loop, which then in turn reads line-by-line?

Otherwise, yes, find is a big problem, where git changes folder view under find's feet. I think there might be a flag or some sort of intermediate command needed to buffer the output first (e.g. like sort does, which could simply also be used).


git commit \
--signoff \
Copy link
Contributor

Choose a reason for hiding this comment

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

There is no need for sign-off here. Since these are generated by a script, rather than typing into a shell this cannot be copyrighted.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why not? You are the author of this change, just because a script did it, you still take ownership of the change.

I think this is a slippery slope, what if AI wrote 5 commits, I would still need to relinquish ownership.
What if I use a script to change all spaces to tabs, and make the script fancy to make the commits also. What if I did the git commit work manually, due to a bug with the script and my target. Should I then omit it?

So maybe there's not a 'hard-need', but it certainly does not hurt.

Copy link
Contributor

Choose a reason for hiding this comment

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

Issue is this cannot by copyrighted. The sole human generated input is the arguments given to the script. One would be laughed out of Court for trying to assert copyright over running a script with 2 version numbers (which come from elsewhere) and a device name (which comes from the repository). I'm a bit worried about implicitly claiming copyright, when it cannot be. I am not a lawyer, but to me this seems worrisome.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But what are you copyrighting? The code or the 'move'. (e.g. the action performed by a human using a script.

So if I executes the commands manually that the script would, I also don't have to sign-off the commits I just? I don't see the difference between me running the commands, or me, through a script. I still did the work, I just had a little help :)

I still hold responsibility here in the end.

Copy link
Contributor

Choose a reason for hiding this comment

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

Read about copyright.

The term I've run across is "sweat of the brow". If you manually type in each command by hand, then yes there could be some degree of copyright claim. Typing the 3 arguments to this script is simply too little work.

--message "kernel/${platform_name}: Create kernel files for v${target_version} (from v${source_version})" \
--message 'This is an automatically generated commit.' \
--message 'During a `git bisect` session, `git bisect --skip` is recommended.'
Comment on lines +136 to +137
Copy link
Contributor

Choose a reason for hiding this comment

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

Mentioned elsewhere, the level of similarity of language to mine (which was publicly visible earlier) makes one wonder about copyright issues. While I'm not going to go after Olliver Schinagl, the OpenWRT project still has an issue...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if we leave out the commands, we're left with

During a session, is recommended. That's 4 words, but you know what, I'll ask chatGPT to rewrite it so we all feel better.


git checkout 'HEAD~' "${_target_dir}"
git commit \
--signoff \
--message "kernel/${platform_name}: Restore kernel files for v${source_version}" \
--message "$(printf "This is an automatically generated commit which aids following Kernel patch history,\nas git will see the move and copy as a rename thus defeating the purpose.\n\nSee: https://lists.openwrt.org/pipermail/openwrt-devel/2023-October/041673.html\nfor the original discussion.")"
Comment on lines +141 to +143
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above.

Pointing to the mailing list discussion seems unhelpful in every single commit message. The URL is better left as a comment inside the script than appearing over and over in git history.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why not? Will there be any bits harmed by being explicit? The immutability is not so nice of course, I'll agree with you on that. So we could add a note in the documentation in addition. But I'll end with https://oftc.irclog.whitequark.org/openwrt-devel/2024-03-05#32992079

Copy link
Contributor

Choose a reason for hiding this comment

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

Different reviewers can generate different opinions. I dislike this, other reviewers may prefer.

git switch "${initial_branch:?Unable to switch back to original branch. Quitting.}"
GIT_EDITOR=true git merge --no-ff '__openwrt_kernel_files_mover'
git branch --delete '__openwrt_kernel_files_mover'

echo "Original commitish was '${initial_commitish}'."
echo 'Kernel bump complete. Remember to use `git log --follow`.'
}

check_requirements()
{
for _cmd in ${REQUIRED_COMMANDS}; do
if ! _test_result="$(command -V "${_cmd}")"; then
_test_result_fail="${_test_result_fail:-}${_test_result}\n"
else
_test_result_pass="${_test_result_pass:-}${_test_result}\n"
fi
done

echo 'Available commands:'
# As the results contain \n, we expect these to be interpreted.
# shellcheck disable=SC2059
printf "${_test_result_pass:-none\n}"
echo
echo 'Missing commands:'
# shellcheck disable=SC2059
printf "${_test_result_fail:-none\n}"
echo

if [ -n "${_test_result_fail:-}" ]; then
echo 'Command test failed, missing programs.'
test_failed=1
fi
}

main()
{
while getopts 'hp:s:t:' _options; do
case "${_options}" in
'h')
usage
exit 0
;;
'p')
platform_name="${OPTARG}"
;;
's')
source_version="${OPTARG#v}"
;;
't')
target_version="${OPTARG#v}"
;;
':')
e_err "Option -${OPTARG} requires an argument."
exit 1
;;
*)
e_err "Invalid option: -${OPTARG}"
exit 1
;;
esac
done
shift "$((OPTIND - 1))"
Comment on lines +180 to +205
Copy link
Contributor

Choose a reason for hiding this comment

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

Since all 3 are required, why bother having them as options? Easier for use to simply have them as 3 arguments with required order.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because it's always better to be explicit rather then implicit. It's nice that we are not bound to a fixed argument order. It's nice we make it specific what argument is for what purpose. It's nice if we want to add options, we don't have to worry about order and location.


platform_name="${platform_name:-${PLATFORM_NAME:-}}"
source_version="${source_version:-${SOURCE_VERSION:-}}"
target_version="${target_version:-${TARGET_VERSION:-}}"

if [ -z "${source_version:-}" ] || [ -z "${target_version:-}" ]; then
e_err "Source (${source_version}) and target (${target_version}) versions need to be defined."
exit 1
fi

check_requirements

init
bump_kernel
cleanup
}

main "${@}"

exit 0