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
Kernel Bump v2 #14913
Kernel Bump v2 #14913
Conversation
@oliv3r Is there a reason why you aren't doing something like |
Maybe add a change from patches-6.1 to patches-6.6 in this version?
|
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: ***@***.***>
|
febfcdc
to
81e3773
Compare
@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? @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 :) |
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 :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, great job!
change |
What would that change, other then causing: ^-- SC3045 (warning): In POSIX sh, read -d is undefined. |
@hauke Your opinion would really help here |
I am unsure I've quite replicated |
I have three comments: |
Let me re-validate this. After the 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.
I don't have windows, so cannot validate this. I'd need your help to debug this. Can you set the environment variable 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
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 ' It will now print, for example:
in addition to the 'usage'. |
0ca1726
to
4512f68
Compare
@oliv3r The script is creating
|
Curious:
and
|
"-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 |
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 :)
|
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!
|
@namiltd curious how you invoked the program however, and @robimarko that caused this weird double dash |
@oliv3r Well, fun fact is that today I cannot reproduce the dash bein added, it just worked as its supposed to :) |
@oliv3r I downloaded the branch oliv3r:kernel_bump |
Hmm, guess I should mention. The first of the patches for turning |
What do you mean? I'm not sure I follow ...
|
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.
|
Your script is doing most processing of variables (ensuring they're set, adding/removing prefixes, etc) in your 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. |
sort -u); do | ||
if [ ! -s "${_path}" ] || \ | ||
[ "${_path}" = "${_path%%"-${source_version}"}" ]; then | ||
continue |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
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.
|
The section at the top of I'll admit I'm more concerned about showing Meanwhile there are some rather major issues waiting in a review submission, but I'm awaiting at least testing on another pull... |
Uhm... No activity? |
There is, just behind the shadows :)
|
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). |
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>
Thanks! Rebased on top of main and merged! |
Still listening to the crickets here. |
A bunch of fixes and improvements, also based on discussions in the first version, found in #14713.