[Linux-kernel-mentees] POC script to apply patches into a test-branch and run clang-format

Aditya yashsri421 at gmail.com
Sat Feb 6 09:21:18 UTC 2021


On 6/2/21 1:26 am, Aditya wrote:
> On 23/12/20 6:46 pm, Aditya wrote:
>> On 22/12/20 4:23 pm, Lukas Bulwahn wrote:
>>> On Tue, Dec 22, 2020 at 11:34 AM Dwaipayan Ray <dwaipayanray1 at gmail.com> wrote:
>>>>
>>>> On Tue, Dec 22, 2020 at 3:55 PM Aditya <yashsri421 at gmail.com> wrote:
>>>>>
>>>>> On 22/12/20 1:17 am, Dwaipayan Ray wrote:
>>>>>>> Okay, I downloaded and tried it:
>>>>>>>
>>>>>>> perl ./scripts/applypatch.pl --clang-format test.patch
>>>>>>> --branch=test-clang-format-bfq-cgroup
>>>>>>>
>>>>>>> but it failed:
>>>>>>>
>>>>>>> git checkout test-clang-format-bfq-cgroup
>>>>>>> Already on 'test-clang-format-bfq-cgroup'
>>>>>>> git am $patchtest.patch
>>>>>>> fatal: could not open '.patch' for reading: No such file or directory
>>>>>>> Clang formatted patchfile generated at:
>>>>>>> test.patch.EXPERIMENTAL-clang_format-fixes
>>>>>>> git checkout test-clang-format-bfq-cgroup
>>>>>>> Already on 'test-clang-format-bfq-cgroup'
>>>>>>>
>>>>>>> How about setting up a minimal travis.ci check that runs this command
>>>>>>> on a small git repository with one file and one patch on top for
>>>>>>> testing purposes?
>>>>>>>
>>>>>>> The script does not have error handling yet, so the next steps are
>>>>>>> just executed blindly even if the one before failed and is really
>>>>>>> required for the next one to do the right thing.
>>>>>>>
>>>>>>> I generally like the script, let us work a bit on tuning it for proper
>>>>>>> error handling, add some basic tests and then discuss it with Miguel.
>>>>>>>
>>>>>>> Lukas
>>>>>>>
>>>>>>
>>>>>> Hello Lukas,
>>>>>> Sorry for being late on this update. I have my college exams going on
>>>>>> right now, so that caused a bit of delay.
>>>>>>
>>>>>> Nevertheless I have applied some changes along with some error handling fixes.
>>>>>> The script is available here:
>>>>>> https://github.com/raydwaipayan/apply-patch
>>>>>>
>>>>>> The script has the following options now:
>>>>>> usage: $P [options] patchfile(s)
>>>>>> Options:
>>>>>> --branch=<branch-name> => test branch name
>>>>>> --format => Use clang format
>>>>>> --clang-format-diff=<s> => clang-format-diff path
>>>>>> --help => show this help information
>>>>>> --root=<root_dir> => root dir location
>>>>>>
>>>>>> By default script searches for "clang-format-diff.py" in path. If it goes
>>>>>> by any other name that should be supplied using
>>>>>> --clang-format-diff="FULL_PATH_OF_EXECUTABLE"
>>>>>>
>>>>>> I ran the script like this:
>>>>>> $ ./scripts/applypatch.pl
>>>>>> 0001-scripts-mod-cleanup-a-few-checkpatch.pl-warnings.patch --format
>>>>>>
>>>>>> Expected output:
>>>>>> git checkout -b test-master
>>>>>> Switched to a new branch 'test-master'
>>>>>> git am 0001-scripts-mod-cleanup-a-few-checkpatch.pl-warnings.patch
>>>>>> Patch applied successfully:
>>>>>> 0001-scripts-mod-cleanup-a-few-checkpatch.pl-warnings.patch
>>>>>> Running clang-format
>>>>>> Diff written to clang-format-fixes.diff
>>>>>> git checkout master
>>>>>> Switched to branch 'master
>>>>>>
>>>>>> Lukas, could you check if this script works on your system as expected?
>>>>>>
>>>>>> Also Aditya, I think the latest clang-format-diff goes by the name
>>>>>> 'clang-format-diff-8' on debian based systems. While in my arch based one
>>>>>> it goes by 'clang-format-diff.py'. Can you verify whether the script works
>>>>>> directly or is the executable path flag needed?
>>>>>>
>>>>>
>>>>> For debian based system, the user will have to add
>>>>> --clang-format-diff="/usr/bin/clang-format-diff-8".
>>>>>
>>>> I think that is currently an extra step for the user. Maybe we can
>>>> target most used flavours and fill the binary location automatically.
>>>>
>>>>> A few more improvements can be made. For eg., the script should exit
>>>>> if the patch isn't applying. Currently, it continues a step further
>>>>> and stops after giving error: "Error: This script requires
>>>>> clang-format-diff to be installed.".
>>>>> Also, this puts the git status in mid of git am, which needs to be reset.
>>>>>
>>>> Yeah maybe instead of die we can just return by saying "clang-format won't
>>>> apply".
>>>>
>>>>> Maybe we can improve the error message by displaying them about
>>>>> "--clang-format-diff" option and giving install options in the error
>>>>> message. This should make the script easier to be used.
>>>>>
>>>>
>>>> In that case we may have to give instructions for all the available
>>>> distributions. Maybe we can point them to the LLVM page?
>>>>
>>>
>>> Good discussion. Consider that the first users will fight with the installation.
>>> So, the tool should carefully check if the environment is set up
>>> before trying to do any steps; otherwise, it might be in some strange
>>> intermediate stage and it requires a lot of scripting to roll back
>>> operations etc.
>>>
>>> Also, developers do read README; so, any hints there are always helpful.
>>>
>>>>> One more improvement I guess we can make with the .diff file, by
>>>>> renaming .diff after patchfile name. This way it won't be overwritten
>>>>> each time and also be in consistency with checkpatch.pl --fix.
>>>>>
>>>>
>>>> About this I was thinking we should avoid creating individual diffs.
>>>> Since in the future the script may apply a patch series too, we can
>>>> just generate a single diff and name accordingly. That way in a single
>>>> step the committer can apply all fixes.
>>>>
>>>
>>> Well, the developer that has a patch series would want to have the
>>> stylistic patches to just fold into their original patch order.
>>>
>>> So, you have two reasonable alternatives:
>>>
>>> 1. You simply create some diff commits on top of the patch series,
>>> i.e., one diff commit for each patch in the series. That allows the
>>> developer to review the format change for each patch and then, the
>>> developer can simply squash the stylistic cleanup on the appropriate
>>> patch in the series.
>>> 2. You simply "automate" that workflow in option 1, where you generate
>>> new patches with the format patch applied, just as Aditya suggested.
>>>
>>> For now, as we are still experimenting, I am in favor of 1; simply
>>> because it is easier to review for now, when somebody applies it for
>>> testing. Option 2 is probably good once it works reliably and we have
>>> trust in the formatting setup.
>>>
>>
> 
> Hi Lukas
> 
> I and Dwaipayan have been working on clang-format script. We have
> refined it to a great extent. Now it is not dependent on version
> specific clang-format-diff as well, as was the case earlier.
> I have also tested it with patch series, and it works fine.
> 
> All the formatted changes(with patch commits) are made to the test
> branch, while the original branch is left untouched.
> We have also managed to handle command errors and break the script
> execution at relevant places.
> 
> The file and the steps to be followed can be found here:
> https://github.com/raydwaipayan/apply-patch/tree/dev
> 
> I have also added test patches for easier testing.
> You can follow these steps for quicker testing of script:
> https://github.com/raydwaipayan/apply-patch/tree/dev#steps-to-test-the-script
> 
> The readme has also been updated correspondingly.
> 
> Please test it and let us know your views on it :)
> 
> I think one of the limitations currently is that
> clang-format-fixes.diff gets overwritten and just tells the diff wrt
> last patch.
> I am working on it. Probably, I'll be able to fix this by tomorrow.

Hi Lukas
I have made necessary changes for this feature as well.
Now, for all the patches in patch series, a diff file is getting
created, and it will be stored alongside the patchfile, as
"patchfile.clang-format-fixes.diff"

This is in compliance with your suggestion here:

>>> 1. You simply create some diff commits on top of the patch series,
>>> i.e., one diff commit for each patch in the series. That allows the
>>> developer to review the format change for each patch and then, the
>>> developer can simply squash the stylistic cleanup on the appropriate
>>> patch in the series.
>>> 2. You simply "automate" that workflow in option 1, where you generate
>>> new patches with the format patch applied, just as Aditya suggested.
>>>
>>> For now, as we are still experimenting, I am in favor of 1; simply
>>> because it is easier to review for now, when somebody applies it for
>>> testing. Option 2 is probably good once it works reliably and we have
>>> trust in the formatting setup.

Thanks
Aditya


More information about the Linux-kernel-mentees mailing list