Re: Making historical committed code/patches comply with latest checkpatch
On Fri, Apr 7, 2023 at 9:00 AM Tim Newsome wrote: > > >> Can you use Checkpatch-ignore in the fork? >> > > Possibly. Part of the problem is that I haven't figured out in the github > action how to find which changes are part of the pull request. Erhan > suggested some clever git commands in > https://github.com/riscv/riscv-openocd/pull/816#issuecomment-1478245399, > but they don't work when the action is running. > I think I've got this figured out in https://github.com/riscv/riscv-openocd/pull/822. Assuming that works right, it's still annoying that checkpatch fires when we merge code down from mainline, but I can live with that. Tim
Re: Making historical committed code/patches comply with latest checkpatch
On Fri, Apr 7, 2023 at 7:46 AM Antonio Borneo wrote: > indeed cherry-picking old patches can result in checkpatch screaming > hysterically ! > Note this is not the result of cherry-picking. It's simply merging all of mainline openocd up to a certain change into riscv-openocd. > Can you use Checkpatch-ignore in the fork? > Possibly. Part of the problem is that I haven't figured out in the github action how to find which changes are part of the pull request. Erhan suggested some clever git commands in https://github.com/riscv/riscv-openocd/pull/816#issuecomment-1478245399, but they don't work when the action is running. I guess we'll have to figure that out, and then we can add checkpatch-ignore, or simply override the check when merging. Tim > > Well, > https://github.com/riscv/riscv-openocd/pull/816#issuecomment-1474425966 > is clearly a corner case. I pushed an incorrect SPDX tag to comply > with the older checkpatch, then pushed the fix once the new checkpatch > was merged. > Maybe the easier way is to manually skip the -1 in github. > > For review.openocd.org, I would prefer to receive mainly patches that > pass checkpatch, without excessive use of Checkpatch-ignore. > > Feel free to report here any issue with checkpatch; hopefully we can > improve it. > > Antonio > > > This obviously causes problems with the merging of upstream patches > intoto the RISC-V fork. For example: > > > > https://github.com/riscv/riscv-openocd/pull/816#issuecomment-1474425966 > > > > I was just wondering if there were any opinions on the merit or > otherwise of going back through historical patches (and other already > committed code?) to ensure that they comply with the latest checkpatch at > any point in time? > > > > The main pros seem to be that it would make the OpenOCD source code more > compliant with checkpatch checks and make the pulling of OpenOCD patches to > a downstream fork more straightforward (the latter, perhaps understandably, > not really being a priority for the "main" OpenOCD project?). > > > > But I can certainly also think of some cons :-) - e.g. some patches are > allowed deviations from the checkpatch rules, messing with existing > committed code/patches risks destabilising things, why do RISC-V > development in a fork and not in the upstream project in the first place > etc. > > > > Anyway - I was just interested in feedback on this and, for what it's > worth, if going back through historical patches and getting them to pass > the latest checkpatch checks is considered "a good thing" I am willing to > help out if necessary. > > > > Thanks a lot > > > > Regards > > Tommy > >
Re: Making historical committed code/patches comply with latest checkpatch
Hi Antonio Thanks a lot for the quick reply. > Can you use Checkpatch-ignore in the fork? Maybe that would be a more pragmatic option alright. Let me check with Tim Newsome who leads the work on the RISC-V OpenOCD fork. Unless he's reading this and wants to pitch in himself? :-) Thanks again. Regards Tommy
Re: Making historical committed code/patches comply with latest checkpatch
On Fri, Apr 7, 2023 at 1:13 PM Tommy Murphy wrote: > > Hi there > > As many of you probably know, RISC-V OpenOCD development continues to be done > on this fork: > > https://github.com/riscv/riscv-openocd > > Periodically, changes here are upstreamed to the "main" OpenOCD project > and/or patches upstream are pulled down to more closely sync/align the two > repos. > > However an issue that has arisen from time to time is that the latest > checkpatch: > > https://github.com/openocd-org/openocd/blob/master/tools/scripts/checkpatch.pl > > has been modified/updated such that previously commited code/patches now fail > the latest checks. Hi Tommy, indeed cherry-picking old patches can result in checkpatch screaming hysterically ! I didn't have this case in mind when I added to checkpatch the possibility of dropping some failing tests. Checkpatch comes from the Linux kernel, where the patch review is done on the mailing list; it's known that checkpatch has several limitations, and maintainers accept patches that fail at checkpatch if the developer provides good reasons. Instead OpenOCD review is done in gerrit, and a failure with checkpatch can cause maintainers to ignore the patch. Dropping some check is sometimes mandatory, e.g. https://review.openocd.org/7568 https://review.openocd.org/7579 Adding 'Checkpatch-ignore:' in the commit message does the trick. It is explained inside HACKING Back to re-sync upstream and RISC-V fork, I don't know what is the easier or better way to proceed. Can you use Checkpatch-ignore in the fork? Well, https://github.com/riscv/riscv-openocd/pull/816#issuecomment-1474425966 is clearly a corner case. I pushed an incorrect SPDX tag to comply with the older checkpatch, then pushed the fix once the new checkpatch was merged. Maybe the easier way is to manually skip the -1 in github. For review.openocd.org, I would prefer to receive mainly patches that pass checkpatch, without excessive use of Checkpatch-ignore. Feel free to report here any issue with checkpatch; hopefully we can improve it. Antonio > This obviously causes problems with the merging of upstream patches intoto > the RISC-V fork. For example: > > https://github.com/riscv/riscv-openocd/pull/816#issuecomment-1474425966 > > I was just wondering if there were any opinions on the merit or otherwise of > going back through historical patches (and other already committed code?) to > ensure that they comply with the latest checkpatch at any point in time? > > The main pros seem to be that it would make the OpenOCD source code more > compliant with checkpatch checks and make the pulling of OpenOCD patches to a > downstream fork more straightforward (the latter, perhaps understandably, not > really being a priority for the "main" OpenOCD project?). > > But I can certainly also think of some cons :-) - e.g. some patches are > allowed deviations from the checkpatch rules, messing with existing committed > code/patches risks destabilising things, why do RISC-V development in a fork > and not in the upstream project in the first place etc. > > Anyway - I was just interested in feedback on this and, for what it's worth, > if going back through historical patches and getting them to pass the latest > checkpatch checks is considered "a good thing" I am willing to help out if > necessary. > > Thanks a lot > > Regards > Tommy
Making historical committed code/patches comply with latest checkpatch
Hi there As many of you probably know, RISC-V OpenOCD development continues to be done on this fork: * https://github.com/riscv/riscv-openocd Periodically, changes here are upstreamed to the "main" OpenOCD project and/or patches upstream are pulled down to more closely sync/align the two repos. However an issue that has arisen from time to time is that the latest checkpatch: * https://github.com/openocd-org/openocd/blob/master/tools/scripts/checkpatch.pl has been modified/updated such that previously commited code/patches now fail the latest checks. This obviously causes problems with the merging of upstream patches intoto the RISC-V fork. For example: * https://github.com/riscv/riscv-openocd/pull/816#issuecomment-1474425966 I was just wondering if there were any opinions on the merit or otherwise of going back through historical patches (and other already committed code?) to ensure that they comply with the latest checkpatch at any point in time? The main pros seem to be that it would make the OpenOCD source code more compliant with checkpatch checks and make the pulling of OpenOCD patches to a downstream fork more straightforward (the latter, perhaps understandably, not really being a priority for the "main" OpenOCD project?). But I can certainly also think of some cons :-) - e.g. some patches are allowed deviations from the checkpatch rules, messing with existing committed code/patches risks destabilising things, why do RISC-V development in a fork and not in the upstream project in the first place etc. Anyway - I was just interested in feedback on this and, for what it's worth, if going back through historical patches and getting them to pass the latest checkpatch checks is considered "a good thing" I am willing to help out if necessary. Thanks a lot Regards Tommy
