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

[Aditya <yashsri421@gmail.com>]

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@gmail.com> wrote:
>>>
>>> On Tue, Dec 22, 2020 at 3:55 PM Aditya <yashsri421@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.

Thanks
Aditya
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees