Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
scripts: Kernel bumper script #14713
Changes from all commits
3561015
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
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.
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
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.
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.
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
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.
Related to the above, these two loops have an odd similarity.
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.
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.
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 andgit
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 givefind
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.
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).
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.
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.
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.
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.
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.
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.
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.
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.
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.
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.
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.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.
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.
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
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.
Different reviewers can generate different opinions. I dislike this, other reviewers may prefer.
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.
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.
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.