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

Aditya yashsri421 at gmail.com
Wed Dec 23 13:16:38 UTC 2020


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 Dwaipayan,
I have made a PR to the repository.
These are the changes that I have made:

- check if the patch applies before applying it. If so, don't apply
the patch.
- remove the user's need to enter file path for clang-format-diff
- fix python version dependency with respect to clang-format version.
Detect required python version on the basis of clang-version.
- Do not run clang-format if patch application fails
- Update Readme accordingly.

A big improvement we have made is that we no longer require
clang-format-8 specifically. It can be any version and we'll invoke it
accordingly with corresponding python version dependency.

In addition, I feel we have overcome the flavor dependency for
clang-format-diff. This is based on my assumption that the file will
either be present as "clang-format-diff.py" or "clang-format-diff".

I haven't been able to test this though for different flavors, but I
believe this will be true as I haven't seen "clang-format-diff" being
used in any other form apart from "clang-format-diff" or
"clang-format-diff.py" in any doc online.

What do you think?

Thanks
Aditya


More information about the Linux-kernel-mentees mailing list