* [Linux-kernel-mentees] POC script to apply patches into a test-branch and run clang-format @ 2020-12-05 11:10 Dwaipayan Ray 2020-12-05 15:27 ` Aditya 0 siblings, 1 reply; 16+ messages in thread From: Dwaipayan Ray @ 2020-12-05 11:10 UTC (permalink / raw) To: Lukas Bulwahn, linux-kernel-mentees, Dwaipayan Ray, Aditya Srivastava Hi, In continuation to our previous discussion about ways to make checkpatch apply patches into a test brach, and probably run clang-format to generate a diff, I wrote a small script which does the same. It can be found here: https://gist.github.com/raydwaipayan/6b8d10470a163f7ae501c703bc27a83f The script is still in it's infancy and more configuration options and support for patch series is pending. Lukas, it would be great to have your review on this. Thank you, Dwaipayan. _______________________________________________ Linux-kernel-mentees mailing list Linux-kernel-mentees@lists.linuxfoundation.org https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Linux-kernel-mentees] POC script to apply patches into a test-branch and run clang-format 2020-12-05 11:10 [Linux-kernel-mentees] POC script to apply patches into a test-branch and run clang-format Dwaipayan Ray @ 2020-12-05 15:27 ` Aditya 2020-12-05 15:54 ` Dwaipayan Ray 0 siblings, 1 reply; 16+ messages in thread From: Aditya @ 2020-12-05 15:27 UTC (permalink / raw) To: Dwaipayan Ray, Lukas Bulwahn, linux-kernel-mentees On 5/12/20 4:40 pm, Dwaipayan Ray wrote: > Hi, > In continuation to our previous discussion about ways to make checkpatch > apply patches into a test brach, and probably run clang-format to generate > a diff, I wrote a small script which does the same. > > It can be found here: > https://gist.github.com/raydwaipayan/6b8d10470a163f7ae501c703bc27a83f > > The script is still in it's infancy and more configuration options and support > for patch series is pending. > > Lukas, it would be great to have your review on this. > > Thank you, > Dwaipayan. > Hi Dwaipayan Nice work! I was able to run the script, with few changes to the script. I'd add my suggestions about it: - The check with: "(! -e $git_clang_format_file)" doesn't work correctly for me. Although the command runs properly, after I comment out this if-section. Probably if we just check for a certain version of git which added "git clang-format" for the first time, in users system, it might do the trick. - Also "git branch --show-current" is compatible only with Git 2.22 or above (For eg, mine is 2.17.1). Maybe replace it with "git rev-parse --abbrev-ref HEAD"? Also, I created a patch for testing the script. Here it is: From 842e18ecc9500fbd2e5d08c4a2b3e7e3c275b6bc Mon Sep 17 00:00:00 2001 From: Aditya Srivastava <yashsri421@gmail.com> Date: Sat, 5 Dec 2020 19:38:03 +0530 Subject: [PATCH] temp --- block/sed-opal.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/block/sed-opal.c b/block/sed-opal.c index daafadbb88ca..f7929eb67f37 100644 --- a/block/sed-opal.c +++ b/block/sed-opal.c @@ -2322,7 +2322,7 @@ static int opal_add_user_to_lr(struct opal_dev *dev, mutex_lock(&dev->dev_lock); setup_opal_dev(dev); ret = execute_steps(dev, steps, ARRAY_SIZE(steps)); - mutex_unlock(&dev->dev_lock); +mutex_unlock(&dev->dev_lock); return ret; } -- 2.17.1 Now this patch gets applied to a new "test-xyz" branch, but it is not formatted, ie the added line remains un-indented. Is this expected behaviour from the script? I have also tried out something similar (using clang-format-diff though). I'll try to add it in your script and send. Thanks Aditya _______________________________________________ Linux-kernel-mentees mailing list Linux-kernel-mentees@lists.linuxfoundation.org https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [Linux-kernel-mentees] POC script to apply patches into a test-branch and run clang-format 2020-12-05 15:27 ` Aditya @ 2020-12-05 15:54 ` Dwaipayan Ray 2020-12-06 10:48 ` Aditya 0 siblings, 1 reply; 16+ messages in thread From: Dwaipayan Ray @ 2020-12-05 15:54 UTC (permalink / raw) To: Aditya; +Cc: linux-kernel-mentees Hey Aditya, > - The check with: "(! -e $git_clang_format_file)" doesn't work > correctly for me. Although the command runs properly, after I comment > out this if-section. Probably if we just check for a certain version > of git which added "git clang-format" for the first time, in users > system, it might do the trick. > True, we need to track that version and terminate the script. Although I am curious why it didn't work in your case. Is that file non existent on your system? > - Also "git branch --show-current" is compatible only with Git 2.22 or > above (For eg, mine is 2.17.1). Maybe replace it with "git rev-parse > --abbrev-ref HEAD"? Yes, that version compatibility might be a problem. The other option is nice, but again it causes a certain conflict when the HEAD itself is detached. In a detached HEAD state, "git rev-parse --abbrev-ref HEAD" just returns "HEAD". > diff --git a/block/sed-opal.c b/block/sed-opal.c > index daafadbb88ca..f7929eb67f37 100644 > --- a/block/sed-opal.c > +++ b/block/sed-opal.c > @@ -2322,7 +2322,7 @@ static int opal_add_user_to_lr(struct opal_dev *dev, > mutex_lock(&dev->dev_lock); > setup_opal_dev(dev); > ret = execute_steps(dev, steps, ARRAY_SIZE(steps)); > - mutex_unlock(&dev->dev_lock); > +mutex_unlock(&dev->dev_lock); > > return ret; > } > -- > 2.17.1 > > > Now this patch gets applied to a new "test-xyz" branch, but it is not > formatted, ie the added line remains un-indented. Is this expected > behaviour from the script? > Currently it should only output the diff. The --clang-format flag is to be used though. Maybe the diff could be written into a clang-format-fixes.patch file and apply it later on. That way the user has more control over what gets changed. > I have also tried out something similar (using clang-format-diff > though). I'll try to add it in your script and send. > Yep sure enough. The problem was somehow I didn't have it on my system. So I couldn't use that. But it should be easy to swap in git-clang-format for clang-format-diff. But anyways thanks for testing it out! A lot more to be done but let's see where it goes :) Thank you, Dwaipayan. _______________________________________________ Linux-kernel-mentees mailing list Linux-kernel-mentees@lists.linuxfoundation.org https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Linux-kernel-mentees] POC script to apply patches into a test-branch and run clang-format 2020-12-05 15:54 ` Dwaipayan Ray @ 2020-12-06 10:48 ` Aditya 2020-12-11 6:17 ` Lukas Bulwahn 0 siblings, 1 reply; 16+ messages in thread From: Aditya @ 2020-12-06 10:48 UTC (permalink / raw) To: Dwaipayan Ray; +Cc: linux-kernel-mentees On 5/12/20 9:24 pm, Dwaipayan Ray wrote: > Hey Aditya, > >> - The check with: "(! -e $git_clang_format_file)" doesn't work >> correctly for me. Although the command runs properly, after I comment >> out this if-section. Probably if we just check for a certain version >> of git which added "git clang-format" for the first time, in users >> system, it might do the trick. >> > > True, we need to track that version and terminate the script. Although I > am curious why it didn't work in your case. Is that file non existent on your > system? > >> - Also "git branch --show-current" is compatible only with Git 2.22 or >> above (For eg, mine is 2.17.1). Maybe replace it with "git rev-parse >> --abbrev-ref HEAD"? > > Yes, that version compatibility might be a problem. > The other option is nice, but again it causes a certain conflict when the > HEAD itself is detached. > > In a detached HEAD state, "git rev-parse --abbrev-ref HEAD" just returns > "HEAD". > >> diff --git a/block/sed-opal.c b/block/sed-opal.c >> index daafadbb88ca..f7929eb67f37 100644 >> --- a/block/sed-opal.c >> +++ b/block/sed-opal.c >> @@ -2322,7 +2322,7 @@ static int opal_add_user_to_lr(struct opal_dev *dev, >> mutex_lock(&dev->dev_lock); >> setup_opal_dev(dev); >> ret = execute_steps(dev, steps, ARRAY_SIZE(steps)); >> - mutex_unlock(&dev->dev_lock); >> +mutex_unlock(&dev->dev_lock); >> >> return ret; >> } >> -- >> 2.17.1 >> >> >> Now this patch gets applied to a new "test-xyz" branch, but it is not >> formatted, ie the added line remains un-indented. Is this expected >> behaviour from the script? >> > > Currently it should only output the diff. The --clang-format flag is to be used > though. Maybe the diff could be written into a clang-format-fixes.patch file and > apply it later on. > That way the user has more control over what gets changed. > Hi Dwaipayan, I have modified your script with clang-format-diff here: https://gist.github.com/AdityaSrivast/0fc57fbd49db8c05c7b1154d4289174a As we wanted, it generates a modified patch which is clang formatted, with suffix ".clang-format.EXPERIMENTAL". It also modifies the files with clang-format in test-branch, which was desired with our "--clang-format" flag to this script. I think the code probably will need to be refactored once we are sure that we are on the right track and the script does the desired job. Lukas, what do you think? Thanks Aditya >> I have also tried out something similar (using clang-format-diff >> though). I'll try to add it in your script and send. >> > > Yep sure enough. The problem was somehow I didn't have it on > my system. So I couldn't use that. But it should be easy to swap > in git-clang-format for clang-format-diff. > > But anyways thanks for testing it out! > A lot more to be done but let's see where it goes :) _______________________________________________ Linux-kernel-mentees mailing list Linux-kernel-mentees@lists.linuxfoundation.org https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Linux-kernel-mentees] POC script to apply patches into a test-branch and run clang-format 2020-12-06 10:48 ` Aditya @ 2020-12-11 6:17 ` Lukas Bulwahn 2020-12-11 6:24 ` Dwaipayan Ray 0 siblings, 1 reply; 16+ messages in thread From: Lukas Bulwahn @ 2020-12-11 6:17 UTC (permalink / raw) To: Aditya; +Cc: Dwaipayan Ray, linux-kernel-mentees On Sun, Dec 6, 2020 at 11:49 AM Aditya <yashsri421@gmail.com> wrote: > > On 5/12/20 9:24 pm, Dwaipayan Ray wrote: > > Hey Aditya, > > > >> - The check with: "(! -e $git_clang_format_file)" doesn't work > >> correctly for me. Although the command runs properly, after I comment > >> out this if-section. Probably if we just check for a certain version > >> of git which added "git clang-format" for the first time, in users > >> system, it might do the trick. > >> > > > > True, we need to track that version and terminate the script. Although I > > am curious why it didn't work in your case. Is that file non existent on your > > system? > > > >> - Also "git branch --show-current" is compatible only with Git 2.22 or > >> above (For eg, mine is 2.17.1). Maybe replace it with "git rev-parse > >> --abbrev-ref HEAD"? > > > > Yes, that version compatibility might be a problem. > > The other option is nice, but again it causes a certain conflict when the > > HEAD itself is detached. > > > > In a detached HEAD state, "git rev-parse --abbrev-ref HEAD" just returns > > "HEAD". > > > >> diff --git a/block/sed-opal.c b/block/sed-opal.c > >> index daafadbb88ca..f7929eb67f37 100644 > >> --- a/block/sed-opal.c > >> +++ b/block/sed-opal.c > >> @@ -2322,7 +2322,7 @@ static int opal_add_user_to_lr(struct opal_dev *dev, > >> mutex_lock(&dev->dev_lock); > >> setup_opal_dev(dev); > >> ret = execute_steps(dev, steps, ARRAY_SIZE(steps)); > >> - mutex_unlock(&dev->dev_lock); > >> +mutex_unlock(&dev->dev_lock); > >> > >> return ret; > >> } > >> -- > >> 2.17.1 > >> > >> > >> Now this patch gets applied to a new "test-xyz" branch, but it is not > >> formatted, ie the added line remains un-indented. Is this expected > >> behaviour from the script? > >> > > > > Currently it should only output the diff. The --clang-format flag is to be used > > though. Maybe the diff could be written into a clang-format-fixes.patch file and > > apply it later on. > > That way the user has more control over what gets changed. > > > Hi Dwaipayan, > I have modified your script with clang-format-diff here: > https://gist.github.com/AdityaSrivast/0fc57fbd49db8c05c7b1154d4289174a > > As we wanted, it generates a modified patch which is clang formatted, > with suffix ".clang-format.EXPERIMENTAL". It also modifies the files > with clang-format in test-branch, which was desired with our > "--clang-format" flag to this script. > > I think the code probably will need to be refactored once we are sure > that we are on the right track and the script does the desired job. > > Lukas, what do you think? > Aditya, Dwaipayan, can you set up a git repository that you use together to work on this script? And provide a short description of how to use the script with an example in that repository? Then, I am happy to try it out... and provide you more feedback. Lukas _______________________________________________ Linux-kernel-mentees mailing list Linux-kernel-mentees@lists.linuxfoundation.org https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Linux-kernel-mentees] POC script to apply patches into a test-branch and run clang-format 2020-12-11 6:17 ` Lukas Bulwahn @ 2020-12-11 6:24 ` Dwaipayan Ray 2020-12-11 6:27 ` Lukas Bulwahn 0 siblings, 1 reply; 16+ messages in thread From: Dwaipayan Ray @ 2020-12-11 6:24 UTC (permalink / raw) To: Lukas Bulwahn; +Cc: linux-kernel-mentees, Aditya > > Aditya, Dwaipayan, can you set up a git repository that you use > together to work on this script? > > And provide a short description of how to use the script with an > example in that repository? > > Then, I am happy to try it out... and provide you more feedback. > > Lukas Hi Lukas, Sure enough, if Aditya doesn't mind, I will put up a repo for the same. Thank you, Dwaipayan. _______________________________________________ Linux-kernel-mentees mailing list Linux-kernel-mentees@lists.linuxfoundation.org https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Linux-kernel-mentees] POC script to apply patches into a test-branch and run clang-format 2020-12-11 6:24 ` Dwaipayan Ray @ 2020-12-11 6:27 ` Lukas Bulwahn 2020-12-11 9:16 ` Aditya 0 siblings, 1 reply; 16+ messages in thread From: Lukas Bulwahn @ 2020-12-11 6:27 UTC (permalink / raw) To: Dwaipayan Ray; +Cc: linux-kernel-mentees, Aditya On Fri, Dec 11, 2020 at 7:24 AM Dwaipayan Ray <dwaipayanray1@gmail.com> wrote: > > > > > Aditya, Dwaipayan, can you set up a git repository that you use > > together to work on this script? > > > > And provide a short description of how to use the script with an > > example in that repository? > > > > Then, I am happy to try it out... and provide you more feedback. > > > > Lukas > > Hi Lukas, > Sure enough, if Aditya doesn't mind, I will put up a repo for the same. > Dwaipayan, just set up a repository with your latest state that you are currently happy with, then Aditya can fork that and put changes on top as he might see fit and open a pull request, so you integrate them back. > Thank you, > Dwaipayan. _______________________________________________ Linux-kernel-mentees mailing list Linux-kernel-mentees@lists.linuxfoundation.org https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Linux-kernel-mentees] POC script to apply patches into a test-branch and run clang-format 2020-12-11 6:27 ` Lukas Bulwahn @ 2020-12-11 9:16 ` Aditya 2020-12-15 6:46 ` Lukas Bulwahn 0 siblings, 1 reply; 16+ messages in thread From: Aditya @ 2020-12-11 9:16 UTC (permalink / raw) To: Lukas Bulwahn, Dwaipayan Ray; +Cc: linux-kernel-mentees On 11/12/20 11:57 am, Lukas Bulwahn wrote: > On Fri, Dec 11, 2020 at 7:24 AM Dwaipayan Ray <dwaipayanray1@gmail.com> wrote: >> >>> >>> Aditya, Dwaipayan, can you set up a git repository that you use >>> together to work on this script? >>> >>> And provide a short description of how to use the script with an >>> example in that repository? >>> >>> Then, I am happy to try it out... and provide you more feedback. >>> >>> Lukas >> >> Hi Lukas, >> Sure enough, if Aditya doesn't mind, I will put up a repo for the same. >> > > Dwaipayan, just set up a repository with your latest state that you > are currently happy with, then Aditya can fork that and put changes on > top as he might see fit and open a pull request, so you integrate them > back. > Hi I have added my changes in the repo and also provided steps to run the script in readme. Repo Link: https://github.com/raydwaipayan/apply-patch You can use this patch for testing. It applies on next-20201204. From c631c1b9c4c7e3cdbf0e41b21b8fa1d34ba20aef Mon Sep 17 00:00:00 2001 From: Aditya Srivastava <yashsri421@gmail.com> Date: Fri, 11 Dec 2020 14:19:52 +0530 Subject: [PATCH] patch for testing This patch has been created for testing clang-format on c-file. --- block/bfq-cgroup.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/block/bfq-cgroup.c b/block/bfq-cgroup.c index b791e2041e49..8409ee064204 100644 --- a/block/bfq-cgroup.c +++ b/block/bfq-cgroup.c @@ -184,7 +184,8 @@ void bfqg_stats_set_start_empty_time(struct bfq_group *bfqg) return; stats->start_empty_time = ktime_get_ns(); - bfqg_stats_mark_empty(stats); + +bfqg_stats_mark_empty(stats); } void bfqg_stats_update_idle_time(struct bfq_group *bfqg) -- 2.17.1 Thanks Aditya _______________________________________________ Linux-kernel-mentees mailing list Linux-kernel-mentees@lists.linuxfoundation.org https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [Linux-kernel-mentees] POC script to apply patches into a test-branch and run clang-format 2020-12-11 9:16 ` Aditya @ 2020-12-15 6:46 ` Lukas Bulwahn 2020-12-21 19:47 ` Dwaipayan Ray 0 siblings, 1 reply; 16+ messages in thread From: Lukas Bulwahn @ 2020-12-15 6:46 UTC (permalink / raw) To: Aditya; +Cc: Dwaipayan Ray, linux-kernel-mentees On Fri, Dec 11, 2020 at 10:16 AM Aditya <yashsri421@gmail.com> wrote: > > On 11/12/20 11:57 am, Lukas Bulwahn wrote: > > On Fri, Dec 11, 2020 at 7:24 AM Dwaipayan Ray <dwaipayanray1@gmail.com> wrote: > >> > >>> > >>> Aditya, Dwaipayan, can you set up a git repository that you use > >>> together to work on this script? > >>> > >>> And provide a short description of how to use the script with an > >>> example in that repository? > >>> > >>> Then, I am happy to try it out... and provide you more feedback. > >>> > >>> Lukas > >> > >> Hi Lukas, > >> Sure enough, if Aditya doesn't mind, I will put up a repo for the same. > >> > > > > Dwaipayan, just set up a repository with your latest state that you > > are currently happy with, then Aditya can fork that and put changes on > > top as he might see fit and open a pull request, so you integrate them > > back. > > > > Hi > I have added my changes in the repo and also provided steps to run the > script in readme. > > Repo Link: https://github.com/raydwaipayan/apply-patch > > You can use this patch for testing. It applies on next-20201204. > 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 > From c631c1b9c4c7e3cdbf0e41b21b8fa1d34ba20aef Mon Sep 17 00:00:00 2001 > From: Aditya Srivastava <yashsri421@gmail.com> > Date: Fri, 11 Dec 2020 14:19:52 +0530 > Subject: [PATCH] patch for testing > > This patch has been created for testing clang-format on c-file. > --- > block/bfq-cgroup.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/block/bfq-cgroup.c b/block/bfq-cgroup.c > index b791e2041e49..8409ee064204 100644 > --- a/block/bfq-cgroup.c > +++ b/block/bfq-cgroup.c > @@ -184,7 +184,8 @@ void bfqg_stats_set_start_empty_time(struct > bfq_group *bfqg) > return; > > stats->start_empty_time = ktime_get_ns(); > - bfqg_stats_mark_empty(stats); > + > +bfqg_stats_mark_empty(stats); > } > > void bfqg_stats_update_idle_time(struct bfq_group *bfqg) > -- > 2.17.1 > > > Thanks > Aditya > _______________________________________________ Linux-kernel-mentees mailing list Linux-kernel-mentees@lists.linuxfoundation.org https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Linux-kernel-mentees] POC script to apply patches into a test-branch and run clang-format 2020-12-15 6:46 ` Lukas Bulwahn @ 2020-12-21 19:47 ` Dwaipayan Ray 2020-12-22 10:25 ` Aditya 0 siblings, 1 reply; 16+ messages in thread From: Dwaipayan Ray @ 2020-12-21 19:47 UTC (permalink / raw) To: Lukas Bulwahn; +Cc: linux-kernel-mentees, Aditya > 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? Thank you, Dwaipayan. _______________________________________________ Linux-kernel-mentees mailing list Linux-kernel-mentees@lists.linuxfoundation.org https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Linux-kernel-mentees] POC script to apply patches into a test-branch and run clang-format 2020-12-21 19:47 ` Dwaipayan Ray @ 2020-12-22 10:25 ` Aditya 2020-12-22 10:34 ` Dwaipayan Ray 0 siblings, 1 reply; 16+ messages in thread From: Aditya @ 2020-12-22 10:25 UTC (permalink / raw) To: Dwaipayan Ray, Lukas Bulwahn; +Cc: linux-kernel-mentees 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". 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. 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. 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. I'll try to make these improvements and send a PR or a patch to you, where we can discuss any changes. Thanks Aditya _______________________________________________ Linux-kernel-mentees mailing list Linux-kernel-mentees@lists.linuxfoundation.org https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Linux-kernel-mentees] POC script to apply patches into a test-branch and run clang-format 2020-12-22 10:25 ` Aditya @ 2020-12-22 10:34 ` Dwaipayan Ray 2020-12-22 10:53 ` Lukas Bulwahn 0 siblings, 1 reply; 16+ messages in thread From: Dwaipayan Ray @ 2020-12-22 10:34 UTC (permalink / raw) To: Aditya; +Cc: linux-kernel-mentees 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? > 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. > I'll try to make these improvements and send a PR or a patch to you, > where we can discuss any changes. > Thanks Aditya, please do. Regards, Dwaipayan. _______________________________________________ Linux-kernel-mentees mailing list Linux-kernel-mentees@lists.linuxfoundation.org https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Linux-kernel-mentees] POC script to apply patches into a test-branch and run clang-format 2020-12-22 10:34 ` Dwaipayan Ray @ 2020-12-22 10:53 ` Lukas Bulwahn 2020-12-23 13:16 ` Aditya 0 siblings, 1 reply; 16+ messages in thread From: Lukas Bulwahn @ 2020-12-22 10:53 UTC (permalink / raw) To: Dwaipayan Ray; +Cc: linux-kernel-mentees, Aditya 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. Lukas _______________________________________________ Linux-kernel-mentees mailing list Linux-kernel-mentees@lists.linuxfoundation.org https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Linux-kernel-mentees] POC script to apply patches into a test-branch and run clang-format 2020-12-22 10:53 ` Lukas Bulwahn @ 2020-12-23 13:16 ` Aditya 2021-02-05 19:56 ` Aditya 0 siblings, 1 reply; 16+ messages in thread From: Aditya @ 2020-12-23 13:16 UTC (permalink / raw) To: Lukas Bulwahn, Dwaipayan Ray; +Cc: linux-kernel-mentees 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 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 _______________________________________________ Linux-kernel-mentees mailing list Linux-kernel-mentees@lists.linuxfoundation.org https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Linux-kernel-mentees] POC script to apply patches into a test-branch and run clang-format 2020-12-23 13:16 ` Aditya @ 2021-02-05 19:56 ` Aditya 2021-02-06 9:21 ` Aditya 0 siblings, 1 reply; 16+ messages in thread From: Aditya @ 2021-02-05 19:56 UTC (permalink / raw) To: Lukas Bulwahn; +Cc: Dwaipayan Ray, linux-kernel-mentees 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Linux-kernel-mentees] POC script to apply patches into a test-branch and run clang-format 2021-02-05 19:56 ` Aditya @ 2021-02-06 9:21 ` Aditya 0 siblings, 0 replies; 16+ messages in thread From: Aditya @ 2021-02-06 9:21 UTC (permalink / raw) To: Lukas Bulwahn; +Cc: Dwaipayan Ray, linux-kernel-mentees 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@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. 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 _______________________________________________ Linux-kernel-mentees mailing list Linux-kernel-mentees@lists.linuxfoundation.org https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees ^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2021-02-06 9:21 UTC | newest] Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-12-05 11:10 [Linux-kernel-mentees] POC script to apply patches into a test-branch and run clang-format Dwaipayan Ray 2020-12-05 15:27 ` Aditya 2020-12-05 15:54 ` Dwaipayan Ray 2020-12-06 10:48 ` Aditya 2020-12-11 6:17 ` Lukas Bulwahn 2020-12-11 6:24 ` Dwaipayan Ray 2020-12-11 6:27 ` Lukas Bulwahn 2020-12-11 9:16 ` Aditya 2020-12-15 6:46 ` Lukas Bulwahn 2020-12-21 19:47 ` Dwaipayan Ray 2020-12-22 10:25 ` Aditya 2020-12-22 10:34 ` Dwaipayan Ray 2020-12-22 10:53 ` Lukas Bulwahn 2020-12-23 13:16 ` Aditya 2021-02-05 19:56 ` Aditya 2021-02-06 9:21 ` Aditya
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.