Kernel Bump v2 #14913

oliv3r · 2024-03-17T07:56:26Z

A bunch of fixes and improvements, also based on discussions in the first version, found in #14713.

scripts/kernel_bump: Delete merge commit
scripts/kernel_bump: Allow bumping sub-targets
scripts/kernel_bump: Use the git index to find the needed config files
scripts/kernel_bump: Use git to obtain the list of files
scripts/kernel_bump: Allow for migrating only configuration files
scripts/kernel_bump: Do no run on dirty repositories
scripts/kernel_bump: Drop unused function
scripts/kernel_bump: Avoid potential copyright claim

rany2 · 2024-03-17T08:00:28Z

@oliv3r Is there a reason why you aren't doing something like git ls-files 'target/*/config-6.1'?

namiltd · 2024-03-17T08:38:17Z

Maybe add a change from patches-6.1 to patches-6.6 in this version?

find "${_target_dir}" -iname "patches-${source_version}" |
	sort -u |
	while read -r _patches; do
		_path="${_patches%%"/patches-${source_version}"}"
		git mv "${_patches}" "${_path}/patches-${target_version}"
	done

oliv3r · 2024-03-17T08:49:33Z

Nope, I think that's a much better improvement to simply rely on what got already knows. I'll play with that and use that for my v2!

…

On March 17, 2024 9:00:51 a.m. GMT+01:00, rany2 ***@***.***> wrote: @oliv3r Is there a reason why you aren't doing something like `git ls-files 'target/*/config-6.1'`? -- Reply to this email directly or view it on GitHub: #14913 (comment) You are receiving this because you were mentioned. Message ID: ***@***.***>

oliv3r · 2024-03-18T13:37:16Z

@rany2 so now it uses git ls-tree and git ls-files :D

@namiltd what made you say that, as this was the purpose of this script. Anyway, v2 should now do what you expected?
Also, I tested x86 and seemed to work fine! I used ./scripts/kernel_bump.sh -p x86 -s 6.1 -t 6.6 to see. I've also tested with -r generic which works, but then the root config file doesn't get migrated (as expected), so x86 might be a bit 'special' in that case. Though in the future, if updating only specific subtargets turns out to be a 'thing', this can be refined.

@robimarko so hopefully v2 improves, adds new features and makes things better!

@ehem this should address any concerns you have raised before! Thanks for your review, and hope to see more review here :)

oliv3r · 2024-03-18T13:39:34Z

One important note to make, I've added a 'hack' to drop the merge commit, just rebase the last commit. So now we have just our two clean commits. However, I don't know if there's any negative impact. It seems to work fine! So now we have just the two commits :)

rany2

Looks good to me, great job!

namiltd · 2024-03-18T15:12:29Z

change
while IFS=',' read -r
to
while read -d, -r

oliv3r · 2024-03-18T15:50:53Z

change while IFS=',' read -r to while read -d, -r

What would that change, other then causing:

^-- SC3045 (warning): In POSIX sh, read -d is undefined.

robimarko · 2024-03-20T09:16:39Z

@hauke Your opinion would really help here

ehem · 2024-03-20T20:20:55Z

I see issues to point to in a review. I was rather hoping someone would at least test #14907 before I submit a review of #14913. Someone who cannot review is entirely capable of testing.

ehem · 2024-03-21T04:24:17Z

I am unsure I've quite replicated kernel_bump.sh's functionality exactly, but I am fairly confident I've got something which is mostly compatible based on invoking kernel_upgrade.pl from #14907.

namiltd · 2024-03-21T07:19:22Z

I have three comments:
-The script does not work (err: Cannot find target linux directory.) if the name of the folder in which the branch is equal to the name of the platform
-On Windows systems (TartoiseGIT - GIT Bash here), finding the patches-version folder does not work. The script only copies .config files.
-When we run the script without parameters, the following is displayed: "err: Source () and target () versions need to be defined. In my opinion, the same text as with the -h option should be displayed here.

oliv3r · 2024-03-21T08:40:49Z

I have three comments: -The script does not work (err: Cannot find target linux directory.) if the name of the folder in which the branch is equal to the name of the platform

Let me re-validate this. After the git ls-files change I checked that it would work (produce output) from the target directory, but as I was checking multiple platforms, I actually only tested from the root. Valuable feedback, thank you.

However I can not replicate this. Might be a windows vs. linux thing. Not sure how 'supported' OpenWRT on windows is, but this should just work (tm). as it's just regular shell logic.

-On Windows systems (TartoiseGIT - GIT Bash here), finding the patches-version folder does not work. The script only copies .config files.

I don't have windows, so cannot validate this. I'd need your help to debug this. Can you set the environment variable export DEBUG_TRACE_SH=all and run the script again post the problematic output?

As I cannot reproduce it, I need more information to figure out what is happening here.

I've added a change, where we can now run the script from anywhere, by using the local script location. Which does require the script to always be, and always run, from the scripts subdir. Not a problem, but something to be aware of.

-When we run the script without parameters, the following is displayed: "err: Source () and target () versions need to be defined. In my opinion, the same text as with the -h option should be displayed here.

Since you are the second one to say this, I will add this (and improve the output a little). Since the idea was that if ONE parameter was missing, you get 'err: Source (v5.15) and target () versions need to be defined. e.g. it would be obvious. I see now it is not as obvious, and will improve the message and print the usage.

It will now print, for example:

./scripts/kernel_bump.sh -s v5.15
err: Source (5.15) and target (missing target version) versions need to be defined.

in addition to the 'usage'.

robimarko · 2024-03-21T11:16:31Z

@oliv3r The script is creating config--v6.6 and like folders instead of config-6.6

robimarko@fedora:~/Building/AX3600/qualcommax-6.6$ ls target/linux/qualcommax/
config-6.1     config--v6.6   files/         image/         ipq60xx/       ipq807x/       Makefile       patches-6.1/   patches--v6.6/

oliv3r · 2024-03-21T15:17:31Z

@oliv3r The script is creating config--v6.6 and like folders instead of config-6.6

robimarko@fedora:~/Building/AX3600/qualcommax-6.6$ ls target/linux/qualcommax/
config-6.1     config--v6.6   files/         image/         ipq60xx/       ipq807x/       Makefile       patches-6.1/   patches--v6.6/

Curious:

scripts/kernel_bump.sh -p at91 -s v5.15 -t 6.6
ls target/linux/at91/*
target/linux/at91/Makefile  target/linux/at91/modules.mk

target/linux/at91/base-files:
etc

target/linux/at91/files:
arch

target/linux/at91/image:
Config.in  dfboot  gen_at91_sdcard_img.sh  Makefile  sam9x.mk  sama5.mk  sama7.mk  uboot-env.txt

target/linux/at91/patches-5.15:
...

target/linux/at91/patches-6.6:
...

target/linux/at91/sam9x:
config-5.15  config-6.6  target.mk

target/linux/at91/sama5:
config-5.15  config-6.6  target.mk

target/linux/at91/sama7:
config-5.15  config-6.6  target.mk

and

scripts/kernel_bump.sh -p qualcommax -s v6.1 -t 6.6
ls target/linux/qualcommax/*
target/linux/qualcommax/config-6.1  target/linux/qualcommax/config-6.6  target/linux/qualcommax/Makefile

target/linux/qualcommax/files:
arch  include

target/linux/qualcommax/image:
ipq60xx.mk  ipq807x.mk  Makefile

target/linux/qualcommax/ipq60xx:
base-files  config-default  target.mk

target/linux/qualcommax/ipq807x:
base-files  config-default  target.mk

target/linux/qualcommax/patches-6.1:
...

target/linux/qualcommax/patches-6.6:
...

namiltd · 2024-03-21T15:22:10Z

"-t 6.6" should be "-t v6.6"

But in fact the script should ignore this "v" because it is an unnecessary syntax complication.

robimarko · 2024-03-21T15:57:01Z

"-t 6.6" should be "-t v6.6"

But in fact the script should ignore this "v" because it is an unnecessary syntax complication.

I agree, the v is just complication

oliv3r · 2024-03-21T22:28:31Z

But it does. As you saw in my example I used both forms on purpose, to indicate the script ignores the leading v, so users don't have to care which format to use :)

oliv3r · 2024-03-21T22:32:42Z

Hah, I found the error of my ways. Or atleast, one. The leading v is stripped very early on, so early, it o ly happens for cmdline arguments, not if set via environment variables. I'll fix that!

oliv3r · 2024-03-22T07:42:23Z

@namiltd curious how you invoked the program however, and @robimarko that caused this weird double dash config--v6.6 to appear. A debug lug or command invocation would be very useful.

robimarko · 2024-03-22T09:01:51Z

@oliv3r Well, fun fact is that today I cannot reproduce the dash bein added, it just worked as its supposed to :)

namiltd · 2024-03-22T09:23:46Z

@oliv3r I downloaded the branch oliv3r:kernel_bump

ehem · 2024-03-22T23:20:10Z

Hmm, guess I should mention. The first of the patches for turning kernel_bump.sh into a wrapper for kernel_upgrade.pl seems suspiciously like a design suggestion. I created it strictly for the purpose of doing the wrapper, but does seem like it might be better for the script anyway.

oliv3r · 2024-03-23T02:16:28Z

What do you mean? I'm not sure I follow ...

oliv3r · 2024-03-23T02:23:21Z

As discussed on IRC, breaking got bisect is expect but shouldn't be a problem. I'm campaigning on getting something into fit natively, but having a wrapper script for bisect, and using `git notes ` is certainly feasible. Without native git support, it does mean bisect requires a wrapper, which is unfortunate of course but hardly a problem. The discussion then becomes, use a script vs loosing history (ish) which I think is to e far more often.

ehem · 2024-03-23T02:24:59Z

Your script is doing most processing of variables (ensuring they're set, adding/removing prefixes, etc) in your main() function. Yet $platform_name is left mostly unprocessed by main() and instead adjusted inside bump_kernel().

For the conversion to wrapper I had to fix this. I suspect you might want to consider grabbing the first commit.

oliv3r · 2024-03-23T08:43:52Z

Your script is doing most processing of variables (ensuring they're set, adding/removing prefixes, etc) in your main() function. Yet $platform_name is left mostly unprocessed by main() and instead adjusted inside bump_kernel().

For the conversion to wrapper I had to fix this. I suspect you might want to consider grabbing the first commit.

The main function does just some argument setup, it's part of the arguments. You could argue it should be part of init maybe, but not main.

namiltd · 2024-03-24T17:49:28Z

scripts/kernel_bump.sh

+			       sort -u); do
+			if [ ! -s "${_path}" ] || \
+			   [ "${_path}" = "${_path%%"-${source_version}"}" ]; then
+				continue


This testing doesn't work for Windows systems (TartoiseGIT - GIT Bash here).
For Windows, folders always have size 0.
Is it possible to check if the system is Windows by checking if the MINGW_CHOST constant is set.
You can also check: uname -s
MINGW64_NT-10.0-19045

oliv3r · 2024-03-24T22:33:19Z

Gah, OK I'll change that. I prefer normally using g -s as it works on files, symlinks and directories in Linux on my fs at least. The difference compared to -e is that it will fail on a zero byte file. Thanks for debugging stupid NTFS.

ehem · 2024-03-25T22:07:51Z

Your script is doing most processing of variables (ensuring they're set, adding/removing prefixes, etc) in your main() function. Yet $platform_name is left mostly unprocessed by main() and instead adjusted inside bump_kernel().
For the conversion to wrapper I had to fix this. I suspect you might want to consider grabbing the first commit.

The main function does just some argument setup, it's part of the arguments. You could argue it should be part of init maybe, but not main.

The section at the top of bump_kernel() looks suspiciously like argument setup.

I'll admit I'm more concerned about showing kernel_upgrade.pl can do everything this does. This isn't something I could reject as a reviewer (something about ethics and conflict of interest), but it does seem a bit odd.

Meanwhile there are some rather major issues waiting in a review submission, but I'm awaiting at least testing on another pull...

ehem · 2024-04-04T04:40:32Z

Uhm... No activity?

oliv3r · 2024-04-04T05:56:56Z

There is, just behind the shadows :)

ehem · 2024-04-04T06:22:08Z

I do have a review pretty well ready, but my theory was this was in exchange for assistance elsewhere (even if you cannot review, having someone simply state other things work has value).

oliv3r · 2024-04-04T21:28:30Z

I remember, but super busy with $work

Due to potential fears of copyright infringement noted by Elliott Mitchell [0], rewrite our message to belong to OpenWRT. Note, AI was used to aid in construction of this sentence. [0]: https://lists.openwrt.org/pipermail/openwrt-devel/2024-March/042422.html Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>

No need to keep unused empty functions. A left over from early development. Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>

Naivly and lazyly the leading v was only dropped from optarg, not from any environment variable. Lets do this properly and ensure a leading 'v' is always dropped. Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>

If a version string was not supplied, we currently print an empty string. We can do better here. Also by popular demand, print the usage information in case of error. Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>

We want to avoid starting a process when we know it will fail later. Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>

Determine the target directory based on the script location, which might work better in some cases, but at least also allows the script to be ran from with any location in the OpenWRT repository. Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>

In some cases, we want to only migrate configuration files, e.g. if the kernel was bumped already. Lets add a flag for this case to offer flexibility. By default we will migrate configuration flags as before. Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>

Instead of looping of a directory to find directories related to kernel changes, use the git index instead. Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>

The current solution using `find` introduces a racecondition, where `find` and `git mv` get in each others way. While this could be fixed with more-utils sponge command (or even sort -u) to buffer the output of find. However, a much better approach, is to query the git index directly, which will not change, and is far more accurate. Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>

Some targets may need to bump specific sub-targets only. So lets offer this as an option. Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>

While we have included the needed changes via a merge commit, there is no need to keep it. Lets drop the merge commit, which we can do as we haven't pushed anything. Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>

robimarko · 2024-04-12T16:21:08Z

Thanks! Rebased on top of main and merged!

ehem · 2024-05-07T23:20:38Z

I remember, but super busy with $work

Still listening to the crickets here.

github-actions bot added the build/scripts/tools pull request/issues for build, scripts and tools related changes label Mar 17, 2024

oliv3r force-pushed the kernel_bump branch 2 times, most recently from febfcdc to 81e3773 Compare March 18, 2024 13:15

oliv3r changed the title ~~Draft: Kernel Bump v2~~ Kernel Bump v2 Mar 18, 2024

oliv3r force-pushed the kernel_bump branch from 81e3773 to 409623b Compare March 18, 2024 13:37

rany2 approved these changes Mar 18, 2024

View reviewed changes

oliv3r force-pushed the kernel_bump branch 2 times, most recently from 0ca1726 to 4512f68 Compare March 21, 2024 09:47

oliv3r force-pushed the kernel_bump branch from 4512f68 to 972aad4 Compare March 22, 2024 07:41

namiltd reviewed Mar 24, 2024

View reviewed changes

ehem mentioned this pull request Mar 25, 2024

scripts: create kernel configuration upgrade script #14907

Open

oliv3r force-pushed the kernel_bump branch from 972aad4 to 0c1418a Compare March 27, 2024 11:54

oliv3r added 11 commits April 12, 2024 18:20

scripts/kernel_bump: Drop unused function

1bbc249

No need to keep unused empty functions. A left over from early development. Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>

scripts/kernel_bump: Always drop v prefix

03f2f53

Naivly and lazyly the leading v was only dropped from optarg, not from any environment variable. Lets do this properly and ensure a leading 'v' is always dropped. Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>

scripts/kernel_bump: Improve omitted version error

ed896c3

If a version string was not supplied, we currently print an empty string. We can do better here. Also by popular demand, print the usage information in case of error. Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>

scripts/kernel_bump: Do no run on dirty repositories

f9ac106

We want to avoid starting a process when we know it will fail later. Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>

scripts/kernel_bump: Use git to obtain the list of files

8ed187e

Instead of looping of a directory to find directories related to kernel changes, use the git index instead. Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>

scripts/kernel_bump: Allow bumping sub-targets

71c663b

Some targets may need to bump specific sub-targets only. So lets offer this as an option. Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>

scripts/kernel_bump: Delete merge commit

98235e6

While we have included the needed changes via a merge commit, there is no need to keep it. Lets drop the merge commit, which we can do as we haven't pushed anything. Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>

robimarko force-pushed the kernel_bump branch from 0c1418a to 98235e6 Compare April 12, 2024 16:20

openwrt-bot merged commit 98235e6 into openwrt:main Apr 12, 2024
2 checks passed

oliv3r deleted the kernel_bump branch April 12, 2024 16:47

Kernel Bump v2 #14913

Kernel Bump v2 #14913

Conversation

oliv3r commented Mar 17, 2024 • edited

rany2 commented Mar 17, 2024

namiltd commented Mar 17, 2024 • edited

oliv3r commented Mar 17, 2024 via email

oliv3r commented Mar 18, 2024

oliv3r commented Mar 18, 2024

rany2 left a comment

Choose a reason for hiding this comment

namiltd commented Mar 18, 2024

oliv3r commented Mar 18, 2024 • edited

robimarko commented Mar 20, 2024

ehem commented Mar 20, 2024

ehem commented Mar 21, 2024

namiltd commented Mar 21, 2024

oliv3r commented Mar 21, 2024 • edited

robimarko commented Mar 21, 2024 • edited

oliv3r commented Mar 21, 2024

namiltd commented Mar 21, 2024 • edited

robimarko commented Mar 21, 2024

oliv3r commented Mar 21, 2024 via email • edited

oliv3r commented Mar 21, 2024 via email • edited

oliv3r commented Mar 22, 2024

robimarko commented Mar 22, 2024

namiltd commented Mar 22, 2024

ehem commented Mar 22, 2024 • edited

oliv3r commented Mar 23, 2024 via email

oliv3r commented Mar 23, 2024 via email

ehem commented Mar 23, 2024

oliv3r commented Mar 23, 2024

namiltd Mar 24, 2024 • edited

Choose a reason for hiding this comment

oliv3r commented Mar 24, 2024 via email

ehem commented Mar 25, 2024

ehem commented Apr 4, 2024

oliv3r commented Apr 4, 2024 via email

ehem commented Apr 4, 2024

oliv3r commented Apr 4, 2024 via email

robimarko commented Apr 12, 2024

ehem commented May 7, 2024

oliv3r commented Mar 17, 2024 •

edited

namiltd commented Mar 17, 2024 •

edited

oliv3r commented Mar 18, 2024 •

edited

oliv3r commented Mar 21, 2024 •

edited

robimarko commented Mar 21, 2024 •

edited

namiltd commented Mar 21, 2024 •

edited

oliv3r commented Mar 21, 2024 via email •

edited

oliv3r commented Mar 21, 2024 via email •

edited

ehem commented Mar 22, 2024 •

edited

namiltd Mar 24, 2024 •

edited