scripts: Kernel bumper script #14713
Conversation
|
I know a perl version 0 was posted to the list, but after v2 and some comments (one being a license issue), I didn't see a v3. Personally the fact that it's very unreadable perl code, and perl is really not needed for this, I wrote it in (posix) shell. I think it is much more readable, as it just keeps all git commands very clear. The script itself is kept simple and I'm sure there's some nasty edges we can resolve in the future. This should work for 99% of the cases. |
github-actions
bot
added
the
build/scripts/tools
|
@oliv3r Does this script intentionally run in verbose mode by default, and creates a merge commit as its product? |
|
@robimarko Hey Robert!
Verbose as in As for the merge commit, I'll quote the mailing list post 0 here again.
So to me, as mentioned in the list, it's a choice between two evil's. A 'move' commit, followed by a 'copy' commit, which is needed to outsmart git, where we have a useless commit. Or, the merge commit, with the added benefit of the history of the 'original' copy. Personally, I'd actually favor the 'useless copy' commit with the history. All history is after all on the 'new' version. But I also see the added value of having the history, as you don't have to think. That choice, I suppose is left with the maintainers. @Ansuel might have some thoughts too. For now, this is just the addition of the script, so we can do the work consistently. |
|
Rebased and fix applied. |
|
@robimarko can you also post your 'tested by' here, and let me know what is missing to get this script merged? |
|
@hauke You mentioned elsewhere
is it worthwhile to automate this (probably better to do this in a second iteration of the script)? What is the usage for other target platforms. Are they supposed to use the same perl-script to migrate their configs? Or is this JUST to sort things? |
|
It worked for me on ath25, so: |
|
@robimarko excellent. I think @hackpascal could try it for 'generic' which should work the same way, but might be slightly special. So what would be left to get this merged? Note, that I've tested it with the |
|
I dont know, it works for me but I would like comments from some of other maintainers as well |
I can have a try for my local repo first. |
Sure thing, hopefully you can nudge them into this direction ;) |
|
@oliv3r I asked for some feedback on the irc, please look at: |
I got kicked off IRC due to 'needs nicked reg' and I haven't had the motiviation to figure out, again, how to do this with the matrix bridge. Would have been nicer if openwrt was on matrix or had a default bridge so users can join either :) I've taken in the comments from both @jow- and @stintel Though I was hoping @Ansuel would chip in too :) |
scripts/kernel_bump.sh
Outdated
| git commit \ | ||
| --signoff \ | ||
| --message "kernel/${platform_name}: Restore kernel files for v${source_version}" \ | ||
| --message '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.' |
There was a problem hiding this comment.
Not a fan of putting changeable URL's here, but the mailing list will be more reliable then a github pull request. Who knows when openwrt will finally move to gitlab instead :D
There was a problem hiding this comment.
@ehem As you can see, I do agree; but if OpenWRT maintainers say so, I don't mind. It's just some bytes in a commit message, it doesn't hurt, even though I'm not a fan either.
|
I tested this script with my loongarch64 branch, and so far it works perfect: So But I found a bug, that '\n' wasn't converted to real new lines in commit message: |
Thanks! I see! https://stackoverflow.com/questions/5064563/how-to-add-line-break-to-git-commit-m-from-the-command-line :p I'll fix it. If possible, if you can also test it for your work on the 'generic' 6.6 upgrade, I think that'll be useful too. |
|
One more problem remains, for some reason, the sign-off is directly after the final text, no idea why though. |
I've already done all works on the branch I mentioned 🤣, and the result is the contents of kernel-bumper-test-v6.6 and linux-v6.6 are identical. So I will switch to kernel_bumper if this PR gets merged. |
|
So I fixed the final problem, by adding some random characters after the URL. No idea why this works, but also no desire to figure it out any more :) the sentence still reads acceptably in my opinion :p |
|
Fixed @jow- 's |
For years, we have struggled and been frustrated at loosing history of files in git, due to the 'copy + add' strategy. This could have been prevented with a double-commit 'mv + add' trick. On the mailing list [0] the discussion was started to put the instructions in a wiki. Instead, it is much better to just script it and put it in the repo. Instead of doing mv + copy, which leads to two commits, but no history on the copied files, it uses move, + copy and merge, which results in three (merge) commits, but keeps the history of all files. As always with renames, `--follow` will be needed. The tool is trivial and works either in the OpenWrt git root directory, or in the actual target directory. Tested on the `realtek` and generic targets. Note, that the tool does not do any of the labor needed after the move, such as updating configs, dropping patches etc. To make sure this script is easily found by any developer, who just wants to do a kernel bump, the script is added here and not to maintainer-tools repo as those scripts are a little bit more specialized. Bumping a kernel is a trivial task that often regular developers do, where most do not even know the existence of maintainer tools, are not part of the main repo they'd clone, not part of the docker container they'd use and so discoverability is probably much more important. [0]: https://lists.openwrt.org/pipermail/openwrt-devel/2023-October/041673.html Signed-off-by: Olliver Schinagl <oliver@schinagl.nl> Tested-by: Robert Marko <robimarko@gmail.com> Tested-by: Weijie Gao <hackpascal@gmail.com>
|
Thanks! Rebased on top of main and merged! |
There was a problem hiding this comment.
Since this started on the mailing list, I hadn't expected the discussion to be squirreled away. As a result this is a bit late for review, but...
scripts/kernel_bump.sh => "err: Source () and target () versions need to be defined.", quite unhelpful.
git checkout origin/main; scripts/kernel_bump.sh -p tegra -s 5.15 -t 6.6 the repository is left in an unhelpful state, dissimilar to where it started.
Due to being so similar to the educational blog post, this generates 3 commits to do the task. Whereas with simple refinement, only 2 are needed. Guessing that post was expecting people to handle the optimization themselves.
| exit 1 | ||
| fi | ||
|
|
||
| git switch --force-create '__openwrt_kernel_files_mover' |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
| 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 | ||
|
|
||
| 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 |
There was a problem hiding this comment.
Related to the above, these two loops have an odd similarity.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
| --message 'This is an automatically generated commit.' \ | ||
| --message 'During a `git bisect` session, `git bisect --skip` is recommended.' |
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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.
| done | ||
|
|
||
| git commit \ | ||
| --signoff \ |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| --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.")" |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Different reviewers can generate different opinions. I dislike this, other reviewers may prefer.
| 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))" |
There was a problem hiding this comment.
Since all 3 are required, why bother having them as options? Easier for use to simply have them as 3 arguments with required order.
There was a problem hiding this comment.
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.
|
What about the patches folder (e.g. patches-6.1)? The script could additionally copy this folder. |
|
It should andere does, unless you uncovered a big ;)
It schans for Evert file or folder with the kernel version postix
…On March 16, 2024 12:20:02 p.m. GMT+01:00, Mieczyslaw Nalewaj ***@***.***> wrote:
What about the patches folder (e.g. patches-6.1)? The script could additionally copy this folder.
--
Reply to this email directly or view it on GitHub:
#14713 (comment)
You are receiving this because you were mentioned.
Message ID: ***@***.***>
|
|
For x86, when changing from 6.1 to 6.6, the patches-6.1 folder was not copied. |
How is it unhelpful? It tells you that both source is empty and target is empty. and refuses to try and do something. Supply only one, and you get told that you have source set, but target not etc. I think it's quite helpful :)
I suppose this would be where fast-import would be an advantage, as we'd not touch the repositories working dir. oth, this also allows you to manually intervene, or switch back to the original state. I wonder though why this is failing on tegra. Good thing nothing is set in stone and we can iterate and deal with these situations as they arise :) Best not let perfect be the enemy of good enough :p
I suppose we are left guessing here as well. Please do share, as my git knowledge is far inferior as a simple user :) I'd love to see a patch that improve the commit efficiency hereof. Btw, could be that the blog post wasn't even looking or thinking of this improvement maybe? Idk. |
Curious, I'll try to reproduce, but the script is quite dumb as it just loops over folder names. |
|
Thanks for the review @ehem. I'll address raised issues for a v2, and hope to see suggestions and recommendations for further improvements! |
|
The Subtargets/Subfolders option would also be helpful. |
Filesystem behavior might explain this. The second loop is definitely both reading and writing the directory at the same time. If the shell doesn't behave quite the way I expect, the same might be true of the first loop. Then filesystem not providing the data you expect could explain the situation.
That is a classic situation where it should provide the usage message.
Tegra was merely a handy example, it happens with everything. That is an example of the repository wasn't in the state you assumed it would be in.
Given that I was unaware of the approach used, the blogger definitely had knowledge I lacked. Question is whether their overall Switch the order of the second ( |
|
Hmm, might be |
Yeah, I'll try to look at it and reproduce/improve. I do think this would be good to have. How do you expect this to work, exclude subtargets or only include when supplied. E.g. by default do all subtargets (like it does now, but only for config files, as thats the only thing i see in ramips and realtek). and if you supply a subtarget, only the that specific one is done? |
Meh, choices, sometimes it helps to print both, sometimes its annoying. Usage is cool, but also noisy, and I have to find the actual error message, if there is even one. Classical poor UX imo. So 'it depends' is the answer, and I agree doing both here is not too bad.
Still don't see anything special about Tegra. If it is the 'find' issue, I was testing it ontop of btrfs, not sure what @robimarko was using when he tested it.
|
|
Maybe it would be enough to add option -i (like ignore) and add a list of subfolders/files to skip in a given target. E.g.: "config-6.1:mt7620" will skip config-6.1 in the main folder and all files in the mt7620 subfolder. Or multiple -i with different files/folders to ignore. |
For years, we have struggled and been frustrated at loosing history of files in git, due to the 'copy + add' strategy. This could have been prevented with a double-commit 'mv + add' trick.
On the mailing list 0 the discussion was started to put the instructions in a wiki. Instead, it is much better to just script it and put it in the repo.
Instead of doing mv + copy, which leads to two commits, but no history on the copied files, it uses move, + copy and merge, which results in three (merge) commits, but keeps the history of all files. As always with renames,
--followwill be needed.The tool is trivial and works either in the openwrt git root directory, or in the actual target directory.
Tested on the realtek and generic targets.
Note, that the tool does not do any of the labor needed after the move, such as updating configs, dropping patches etc.