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

Lukas Bulwahn lukas.bulwahn at gmail.com
Tue Dec 22 10:53:49 UTC 2020


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.

Lukas


More information about the Linux-kernel-mentees mailing list