* [DIM PATCH 1/3] dim: cleanup checkpatch_commit
@ 2018-03-13 11:30 Jani Nikula
2018-03-13 11:30 ` [DIM PATCH 2/3] dim: add checkpatch profiles to allow different checkpatch options Jani Nikula
2018-03-13 11:30 ` [DIM PATCH 3/3] dim: loosen some drm-intel checkpatch rules Jani Nikula
0 siblings, 2 replies; 9+ messages in thread
From: Jani Nikula @ 2018-03-13 11:30 UTC (permalink / raw)
To: intel-gfx, dim-tools; +Cc: jani.nikula, Rodrigo Vivi
Remove some old cruft. Pass checkpatch parameters via a variable. No
functional changes.
Cc: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
dim | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/dim b/dim
index 45aec403a44e..81e2bc1511ac 100755
--- a/dim
+++ b/dim
@@ -1360,13 +1360,14 @@ function check_maintainer
# $1 is the git sha1 to check
function checkpatch_commit
{
- local commit cmd bug_lines non_i915_files rv
+ local commit rv checkpatch_options
commit=$1
- cmd="git show --pretty=email $commit"
+ checkpatch_options="-q --emacs --strict --show-types -"
git --no-pager log --oneline -1 $commit
- if ! $cmd | scripts/checkpatch.pl -q --emacs --strict --show-types -; then
+ if ! git show --pretty=email $commit |\
+ scripts/checkpatch.pl $checkpatch_options; then
rv=1
fi
--
2.11.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [DIM PATCH 2/3] dim: add checkpatch profiles to allow different checkpatch options
2018-03-13 11:30 [DIM PATCH 1/3] dim: cleanup checkpatch_commit Jani Nikula
@ 2018-03-13 11:30 ` Jani Nikula
2018-03-13 11:48 ` Daniel Vetter
2018-03-13 11:30 ` [DIM PATCH 3/3] dim: loosen some drm-intel checkpatch rules Jani Nikula
1 sibling, 1 reply; 9+ messages in thread
From: Jani Nikula @ 2018-03-13 11:30 UTC (permalink / raw)
To: intel-gfx, dim-tools; +Cc: jani.nikula, Rodrigo Vivi
To reduce noise on CI checkpatch reports, we want to silence some
checkpatch warnings. Different branches may end up having different
rules, and users may want to get unfiltered results, so introduce
checkpatch profiles. Add some placeholder profiles to be filled later
on.
The idea is that CI would run 'dim checkpatch HEAD^ drm-intel' (or some
other commit range-ish as the case may be).
Cc: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
dim | 46 +++++++++++++++++++++++++++++++++++++++++-----
dim.rst | 9 +++++++--
2 files changed, 48 insertions(+), 7 deletions(-)
diff --git a/dim b/dim
index 81e2bc1511ac..4ba1c7ff490a 100755
--- a/dim
+++ b/dim
@@ -836,7 +836,7 @@ function apply_patch #patch_file
rv=1
fi
- if ! checkpatch_commit HEAD; then
+ if ! checkpatch_commit HEAD branch; then
rv=1
fi
if ! check_maintainer $branch HEAD; then
@@ -1358,12 +1358,47 @@ function check_maintainer
}
# $1 is the git sha1 to check
+# $2 is the checkpatch profile
function checkpatch_commit
{
- local commit rv checkpatch_options
+ local commit rv checkpatch_options profile profile_options
commit=$1
- checkpatch_options="-q --emacs --strict --show-types -"
+ profile=${2:-default}
+
+ # special branch profile maps branches to profiles
+ if [[ "$profile" = "branch" ]]; then
+ case "$(git_current_branch)" in
+ drm-intel-next-queued|drm-intel-next-fixes|drm-intel-fixes)
+ profile=drm-intel
+ ;;
+ drm-misc-next|drm-misc-next-fixes|drm-misc-fixes)
+ profile=drm-misc
+ ;;
+ *)
+ profile=default
+ ;;
+ esac
+ fi
+
+ # map profiles to checkpatch options
+ case "$profile" in
+ default)
+ profile_options=""
+ ;;
+ drm-misc)
+ profile_options=""
+ ;;
+ drm-intel)
+ profile_options=""
+ ;;
+ *)
+ echoerr "Unknown checkpatch profile $profile"
+ profile_options=""
+ ;;
+ esac
+
+ checkpatch_options="-q --emacs --strict --show-types $profile_options -"
git --no-pager log --oneline -1 $commit
if ! git show --pretty=email $commit |\
@@ -1430,12 +1465,13 @@ function dim_extract_next_fixes
dim_alias_cp=checkpatch
function dim_checkpatch
{
- local range rv
+ local range profile rv
range=$(rangeish "${1:-}")
+ profile=${2:-}
for commit in $(git rev-list --reverse $range); do
- if ! checkpatch_commit $commit; then
+ if ! checkpatch_commit $commit $profile; then
rv=1
fi
done
diff --git a/dim.rst b/dim.rst
index e2c5fd6e6d0a..cc930959e497 100644
--- a/dim.rst
+++ b/dim.rst
@@ -130,13 +130,18 @@ fixes *commit-ish*
Print the Fixes: and Cc: lines for the supplied *commit-ish* in the linux kernel
CodingStyle approved format.
-checkpatch [*commit-ish* [.. *commit-ish*]]
--------------------------------------------
+checkpatch [*commit-ish* [.. *commit-ish*]] [*profile*]
+-------------------------------------------------------
Runs the given commit range commit-ish..commit-ish through the check tools.
If no commit-ish is passed, defaults to HEAD^..HEAD. If one commit-ish is passed
instead of a range, the range commit-ish..HEAD is used.
+If profile is given, uses specific options for checkpatch error
+filtering. Current profiles are "default", "branch", "drm-intel", and
+"drm-misc". The "branch" profile maps the current git branch to the appropriate
+profile, or if the branch is not known, to "default".
+
sparse [*commit-ish* [.. *commit-ish*]]
---------------------------------------
Run sparse on the files changed by the given commit range.
--
2.11.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [DIM PATCH 3/3] dim: loosen some drm-intel checkpatch rules
2018-03-13 11:30 [DIM PATCH 1/3] dim: cleanup checkpatch_commit Jani Nikula
2018-03-13 11:30 ` [DIM PATCH 2/3] dim: add checkpatch profiles to allow different checkpatch options Jani Nikula
@ 2018-03-13 11:30 ` Jani Nikula
2018-03-13 11:55 ` Daniel Vetter
1 sibling, 1 reply; 9+ messages in thread
From: Jani Nikula @ 2018-03-13 11:30 UTC (permalink / raw)
To: intel-gfx, dim-tools; +Cc: jani.nikula, Rodrigo Vivi
Set max line length to 100. I don't want to silence the LONG_LINE
warning altogether, and I'd still prefer to keep lines under 80
characters, but I also don't want to see all the noise, and nor do I
want to see silly code trying to arbitrarily squeeze under 80 when it
doesn't make sense. 100 is a nice arbitrary round number... I hope
review catches silly stuff regardless. Fingers crossed.
BIT_MACRO. We have (1 << N) all over the place. I hope to switch to
BIT() macro eventually, but this documents current use.
PREFER_KERNEL_TYPES. We also have uint(8|16|32|64)_t all over the
place. I also hope to move towards kernel types, but this documents
current use.
SPLIT_STRING, LONG_LINE_STRING. Don't nag about strings split to many
lines, but also don't nag about strings not split.
There's plenty more that could be tweaked, but let's start with
something to improve the S/N ratio of the automated CI checkpatch
reports. Now that we have --show-types included in the output, we can
more easily discuss the ignores on a case-by-case basis.
Cc: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
dim | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/dim b/dim
index 4ba1c7ff490a..9fa6d9cd855b 100755
--- a/dim
+++ b/dim
@@ -1390,7 +1390,7 @@ function checkpatch_commit
profile_options=""
;;
drm-intel)
- profile_options=""
+ profile_options="--max-line-length=100 --ignore=BIT_MACRO,PREFER_KERNEL_TYPES,SPLIT_STRING,LONG_LINE_STRING"
;;
*)
echoerr "Unknown checkpatch profile $profile"
--
2.11.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [DIM PATCH 2/3] dim: add checkpatch profiles to allow different checkpatch options
2018-03-13 11:30 ` [DIM PATCH 2/3] dim: add checkpatch profiles to allow different checkpatch options Jani Nikula
@ 2018-03-13 11:48 ` Daniel Vetter
2018-03-13 15:07 ` Jani Nikula
0 siblings, 1 reply; 9+ messages in thread
From: Daniel Vetter @ 2018-03-13 11:48 UTC (permalink / raw)
To: Jani Nikula; +Cc: intel-gfx, dim-tools, Rodrigo Vivi
On Tue, Mar 13, 2018 at 01:30:09PM +0200, Jani Nikula wrote:
> To reduce noise on CI checkpatch reports, we want to silence some
> checkpatch warnings. Different branches may end up having different
> rules, and users may want to get unfiltered results, so introduce
> checkpatch profiles. Add some placeholder profiles to be filled later
> on.
>
> The idea is that CI would run 'dim checkpatch HEAD^ drm-intel' (or some
> other commit range-ish as the case may be).
>
> Cc: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
> dim | 46 +++++++++++++++++++++++++++++++++++++++++-----
> dim.rst | 9 +++++++--
> 2 files changed, 48 insertions(+), 7 deletions(-)
>
> diff --git a/dim b/dim
> index 81e2bc1511ac..4ba1c7ff490a 100755
> --- a/dim
> +++ b/dim
> @@ -836,7 +836,7 @@ function apply_patch #patch_file
> rv=1
> fi
>
> - if ! checkpatch_commit HEAD; then
> + if ! checkpatch_commit HEAD branch; then
> rv=1
> fi
> if ! check_maintainer $branch HEAD; then
> @@ -1358,12 +1358,47 @@ function check_maintainer
> }
>
> # $1 is the git sha1 to check
> +# $2 is the checkpatch profile
> function checkpatch_commit
> {
> - local commit rv checkpatch_options
> + local commit rv checkpatch_options profile profile_options
>
> commit=$1
> - checkpatch_options="-q --emacs --strict --show-types -"
> + profile=${2:-default}
> +
> + # special branch profile maps branches to profiles
> + if [[ "$profile" = "branch" ]]; then
> + case "$(git_current_branch)" in
> + drm-intel-next-queued|drm-intel-next-fixes|drm-intel-fixes)
> + profile=drm-intel
> + ;;
> + drm-misc-next|drm-misc-next-fixes|drm-misc-fixes)
> + profile=drm-misc
> + ;;
Use branch_to_repo instead, if that doesn't come up with anything, then
default?
With that little bit of polished applied, on patches 1&2:
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> + *)
> + profile=default
> + ;;
> + esac
> + fi
> +
> + # map profiles to checkpatch options
> + case "$profile" in
> + default)
> + profile_options=""
> + ;;
> + drm-misc)
> + profile_options=""
> + ;;
> + drm-intel)
> + profile_options=""
> + ;;
> + *)
> + echoerr "Unknown checkpatch profile $profile"
> + profile_options=""
> + ;;
> + esac
> +
> + checkpatch_options="-q --emacs --strict --show-types $profile_options -"
>
> git --no-pager log --oneline -1 $commit
> if ! git show --pretty=email $commit |\
> @@ -1430,12 +1465,13 @@ function dim_extract_next_fixes
> dim_alias_cp=checkpatch
> function dim_checkpatch
> {
> - local range rv
> + local range profile rv
>
> range=$(rangeish "${1:-}")
> + profile=${2:-}
>
> for commit in $(git rev-list --reverse $range); do
> - if ! checkpatch_commit $commit; then
> + if ! checkpatch_commit $commit $profile; then
> rv=1
> fi
> done
> diff --git a/dim.rst b/dim.rst
> index e2c5fd6e6d0a..cc930959e497 100644
> --- a/dim.rst
> +++ b/dim.rst
> @@ -130,13 +130,18 @@ fixes *commit-ish*
> Print the Fixes: and Cc: lines for the supplied *commit-ish* in the linux kernel
> CodingStyle approved format.
>
> -checkpatch [*commit-ish* [.. *commit-ish*]]
> --------------------------------------------
> +checkpatch [*commit-ish* [.. *commit-ish*]] [*profile*]
> +-------------------------------------------------------
> Runs the given commit range commit-ish..commit-ish through the check tools.
>
> If no commit-ish is passed, defaults to HEAD^..HEAD. If one commit-ish is passed
> instead of a range, the range commit-ish..HEAD is used.
>
> +If profile is given, uses specific options for checkpatch error
> +filtering. Current profiles are "default", "branch", "drm-intel", and
> +"drm-misc". The "branch" profile maps the current git branch to the appropriate
> +profile, or if the branch is not known, to "default".
> +
> sparse [*commit-ish* [.. *commit-ish*]]
> ---------------------------------------
> Run sparse on the files changed by the given commit range.
> --
> 2.11.0
>
> _______________________________________________
> dim-tools mailing list
> dim-tools@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dim-tools
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [DIM PATCH 3/3] dim: loosen some drm-intel checkpatch rules
2018-03-13 11:30 ` [DIM PATCH 3/3] dim: loosen some drm-intel checkpatch rules Jani Nikula
@ 2018-03-13 11:55 ` Daniel Vetter
2018-03-14 9:03 ` Jani Nikula
0 siblings, 1 reply; 9+ messages in thread
From: Daniel Vetter @ 2018-03-13 11:55 UTC (permalink / raw)
To: Jani Nikula; +Cc: intel-gfx, dim-tools, Rodrigo Vivi
On Tue, Mar 13, 2018 at 01:30:10PM +0200, Jani Nikula wrote:
> Set max line length to 100. I don't want to silence the LONG_LINE
> warning altogether, and I'd still prefer to keep lines under 80
> characters, but I also don't want to see all the noise, and nor do I
> want to see silly code trying to arbitrarily squeeze under 80 when it
> doesn't make sense. 100 is a nice arbitrary round number... I hope
> review catches silly stuff regardless. Fingers crossed.
>
> BIT_MACRO. We have (1 << N) all over the place. I hope to switch to
> BIT() macro eventually, but this documents current use.
>
> PREFER_KERNEL_TYPES. We also have uint(8|16|32|64)_t all over the
> place. I also hope to move towards kernel types, but this documents
> current use.
>
> SPLIT_STRING, LONG_LINE_STRING. Don't nag about strings split to many
> lines, but also don't nag about strings not split.
>
> There's plenty more that could be tweaked, but let's start with
> something to improve the S/N ratio of the automated CI checkpatch
> reports. Now that we have --show-types included in the output, we can
> more easily discuss the ignores on a case-by-case basis.
>
> Cc: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
> dim | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/dim b/dim
> index 4ba1c7ff490a..9fa6d9cd855b 100755
> --- a/dim
> +++ b/dim
> @@ -1390,7 +1390,7 @@ function checkpatch_commit
> profile_options=""
> ;;
> drm-intel)
> - profile_options=""
> + profile_options="--max-line-length=100 --ignore=BIT_MACRO,PREFER_KERNEL_TYPES,SPLIT_STRING,LONG_LINE_STRING"
I've scrolled a bit through checkpatch complaines with this, and I think
it looks a lot more reasonable. There's also a huge pile of stuff that we
should probably have fixed when the patches landed, so CI'ing this looks
good.
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Aside: Should we encourage checkpatch patches to clean this up, with the
note that they must use the drm-intel profile and the caveat that we might
want to add more stuff to our ignore list instead of taking the patches?
-Daniel
> ;;
> *)
> echoerr "Unknown checkpatch profile $profile"
> --
> 2.11.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [DIM PATCH 2/3] dim: add checkpatch profiles to allow different checkpatch options
2018-03-13 11:48 ` Daniel Vetter
@ 2018-03-13 15:07 ` Jani Nikula
2018-03-14 6:29 ` Daniel Vetter
0 siblings, 1 reply; 9+ messages in thread
From: Jani Nikula @ 2018-03-13 15:07 UTC (permalink / raw)
To: Daniel Vetter; +Cc: intel-gfx, dim-tools, Rodrigo Vivi
On Tue, 13 Mar 2018, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Tue, Mar 13, 2018 at 01:30:09PM +0200, Jani Nikula wrote:
>> To reduce noise on CI checkpatch reports, we want to silence some
>> checkpatch warnings. Different branches may end up having different
>> rules, and users may want to get unfiltered results, so introduce
>> checkpatch profiles. Add some placeholder profiles to be filled later
>> on.
>>
>> The idea is that CI would run 'dim checkpatch HEAD^ drm-intel' (or some
>> other commit range-ish as the case may be).
>>
>> Cc: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
>> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>> ---
>> dim | 46 +++++++++++++++++++++++++++++++++++++++++-----
>> dim.rst | 9 +++++++--
>> 2 files changed, 48 insertions(+), 7 deletions(-)
>>
>> diff --git a/dim b/dim
>> index 81e2bc1511ac..4ba1c7ff490a 100755
>> --- a/dim
>> +++ b/dim
>> @@ -836,7 +836,7 @@ function apply_patch #patch_file
>> rv=1
>> fi
>>
>> - if ! checkpatch_commit HEAD; then
>> + if ! checkpatch_commit HEAD branch; then
>> rv=1
>> fi
>> if ! check_maintainer $branch HEAD; then
>> @@ -1358,12 +1358,47 @@ function check_maintainer
>> }
>>
>> # $1 is the git sha1 to check
>> +# $2 is the checkpatch profile
>> function checkpatch_commit
>> {
>> - local commit rv checkpatch_options
>> + local commit rv checkpatch_options profile profile_options
>>
>> commit=$1
>> - checkpatch_options="-q --emacs --strict --show-types -"
>> + profile=${2:-default}
>> +
>> + # special branch profile maps branches to profiles
>> + if [[ "$profile" = "branch" ]]; then
>> + case "$(git_current_branch)" in
>> + drm-intel-next-queued|drm-intel-next-fixes|drm-intel-fixes)
>> + profile=drm-intel
>> + ;;
>> + drm-misc-next|drm-misc-next-fixes|drm-misc-fixes)
>> + profile=drm-misc
>> + ;;
>
> Use branch_to_repo instead, if that doesn't come up with anything, then
> default?
This command is supposed to work without configuration (a developer
command)...
BR,
Jani.
>
> With that little bit of polished applied, on patches 1&2:
>
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>> + *)
>> + profile=default
>> + ;;
>> + esac
>> + fi
>> +
>> + # map profiles to checkpatch options
>> + case "$profile" in
>> + default)
>> + profile_options=""
>> + ;;
>> + drm-misc)
>> + profile_options=""
>> + ;;
>> + drm-intel)
>> + profile_options=""
>> + ;;
>> + *)
>> + echoerr "Unknown checkpatch profile $profile"
>> + profile_options=""
>> + ;;
>> + esac
>> +
>> + checkpatch_options="-q --emacs --strict --show-types $profile_options -"
>>
>> git --no-pager log --oneline -1 $commit
>> if ! git show --pretty=email $commit |\
>> @@ -1430,12 +1465,13 @@ function dim_extract_next_fixes
>> dim_alias_cp=checkpatch
>> function dim_checkpatch
>> {
>> - local range rv
>> + local range profile rv
>>
>> range=$(rangeish "${1:-}")
>> + profile=${2:-}
>>
>> for commit in $(git rev-list --reverse $range); do
>> - if ! checkpatch_commit $commit; then
>> + if ! checkpatch_commit $commit $profile; then
>> rv=1
>> fi
>> done
>> diff --git a/dim.rst b/dim.rst
>> index e2c5fd6e6d0a..cc930959e497 100644
>> --- a/dim.rst
>> +++ b/dim.rst
>> @@ -130,13 +130,18 @@ fixes *commit-ish*
>> Print the Fixes: and Cc: lines for the supplied *commit-ish* in the linux kernel
>> CodingStyle approved format.
>>
>> -checkpatch [*commit-ish* [.. *commit-ish*]]
>> --------------------------------------------
>> +checkpatch [*commit-ish* [.. *commit-ish*]] [*profile*]
>> +-------------------------------------------------------
>> Runs the given commit range commit-ish..commit-ish through the check tools.
>>
>> If no commit-ish is passed, defaults to HEAD^..HEAD. If one commit-ish is passed
>> instead of a range, the range commit-ish..HEAD is used.
>>
>> +If profile is given, uses specific options for checkpatch error
>> +filtering. Current profiles are "default", "branch", "drm-intel", and
>> +"drm-misc". The "branch" profile maps the current git branch to the appropriate
>> +profile, or if the branch is not known, to "default".
>> +
>> sparse [*commit-ish* [.. *commit-ish*]]
>> ---------------------------------------
>> Run sparse on the files changed by the given commit range.
>> --
>> 2.11.0
>>
>> _______________________________________________
>> dim-tools mailing list
>> dim-tools@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dim-tools
--
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [DIM PATCH 2/3] dim: add checkpatch profiles to allow different checkpatch options
2018-03-13 15:07 ` Jani Nikula
@ 2018-03-14 6:29 ` Daniel Vetter
2018-03-14 8:57 ` Jani Nikula
0 siblings, 1 reply; 9+ messages in thread
From: Daniel Vetter @ 2018-03-14 6:29 UTC (permalink / raw)
To: Jani Nikula
Cc: intel-gfx, DRM maintainer tools announcements, discussion,
and development, Rodrigo Vivi
On Tue, Mar 13, 2018 at 4:07 PM, Jani Nikula <jani.nikula@intel.com> wrote:
> On Tue, 13 Mar 2018, Daniel Vetter <daniel@ffwll.ch> wrote:
>> On Tue, Mar 13, 2018 at 01:30:09PM +0200, Jani Nikula wrote:
>>> To reduce noise on CI checkpatch reports, we want to silence some
>>> checkpatch warnings. Different branches may end up having different
>>> rules, and users may want to get unfiltered results, so introduce
>>> checkpatch profiles. Add some placeholder profiles to be filled later
>>> on.
>>>
>>> The idea is that CI would run 'dim checkpatch HEAD^ drm-intel' (or some
>>> other commit range-ish as the case may be).
>>>
>>> Cc: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
>>> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>>> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
>>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>>> ---
>>> dim | 46 +++++++++++++++++++++++++++++++++++++++++-----
>>> dim.rst | 9 +++++++--
>>> 2 files changed, 48 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/dim b/dim
>>> index 81e2bc1511ac..4ba1c7ff490a 100755
>>> --- a/dim
>>> +++ b/dim
>>> @@ -836,7 +836,7 @@ function apply_patch #patch_file
>>> rv=1
>>> fi
>>>
>>> - if ! checkpatch_commit HEAD; then
>>> + if ! checkpatch_commit HEAD branch; then
>>> rv=1
>>> fi
>>> if ! check_maintainer $branch HEAD; then
>>> @@ -1358,12 +1358,47 @@ function check_maintainer
>>> }
>>>
>>> # $1 is the git sha1 to check
>>> +# $2 is the checkpatch profile
>>> function checkpatch_commit
>>> {
>>> - local commit rv checkpatch_options
>>> + local commit rv checkpatch_options profile profile_options
>>>
>>> commit=$1
>>> - checkpatch_options="-q --emacs --strict --show-types -"
>>> + profile=${2:-default}
>>> +
>>> + # special branch profile maps branches to profiles
>>> + if [[ "$profile" = "branch" ]]; then
>>> + case "$(git_current_branch)" in
>>> + drm-intel-next-queued|drm-intel-next-fixes|drm-intel-fixes)
>>> + profile=drm-intel
>>> + ;;
>>> + drm-misc-next|drm-misc-next-fixes|drm-misc-fixes)
>>> + profile=drm-misc
>>> + ;;
>>
>> Use branch_to_repo instead, if that doesn't come up with anything, then
>> default?
>
> This command is supposed to work without configuration (a developer
> command)...
Duh :-/
I guess I'm ok as-is, but feels a bit silly.
-Daniel
>
> BR,
> Jani.
>
>
>>
>> With that little bit of polished applied, on patches 1&2:
>>
>> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>>> + *)
>>> + profile=default
>>> + ;;
>>> + esac
>>> + fi
>>> +
>>> + # map profiles to checkpatch options
>>> + case "$profile" in
>>> + default)
>>> + profile_options=""
>>> + ;;
>>> + drm-misc)
>>> + profile_options=""
>>> + ;;
>>> + drm-intel)
>>> + profile_options=""
>>> + ;;
>>> + *)
>>> + echoerr "Unknown checkpatch profile $profile"
>>> + profile_options=""
>>> + ;;
>>> + esac
>>> +
>>> + checkpatch_options="-q --emacs --strict --show-types $profile_options -"
>>>
>>> git --no-pager log --oneline -1 $commit
>>> if ! git show --pretty=email $commit |\
>>> @@ -1430,12 +1465,13 @@ function dim_extract_next_fixes
>>> dim_alias_cp=checkpatch
>>> function dim_checkpatch
>>> {
>>> - local range rv
>>> + local range profile rv
>>>
>>> range=$(rangeish "${1:-}")
>>> + profile=${2:-}
>>>
>>> for commit in $(git rev-list --reverse $range); do
>>> - if ! checkpatch_commit $commit; then
>>> + if ! checkpatch_commit $commit $profile; then
>>> rv=1
>>> fi
>>> done
>>> diff --git a/dim.rst b/dim.rst
>>> index e2c5fd6e6d0a..cc930959e497 100644
>>> --- a/dim.rst
>>> +++ b/dim.rst
>>> @@ -130,13 +130,18 @@ fixes *commit-ish*
>>> Print the Fixes: and Cc: lines for the supplied *commit-ish* in the linux kernel
>>> CodingStyle approved format.
>>>
>>> -checkpatch [*commit-ish* [.. *commit-ish*]]
>>> --------------------------------------------
>>> +checkpatch [*commit-ish* [.. *commit-ish*]] [*profile*]
>>> +-------------------------------------------------------
>>> Runs the given commit range commit-ish..commit-ish through the check tools.
>>>
>>> If no commit-ish is passed, defaults to HEAD^..HEAD. If one commit-ish is passed
>>> instead of a range, the range commit-ish..HEAD is used.
>>>
>>> +If profile is given, uses specific options for checkpatch error
>>> +filtering. Current profiles are "default", "branch", "drm-intel", and
>>> +"drm-misc". The "branch" profile maps the current git branch to the appropriate
>>> +profile, or if the branch is not known, to "default".
>>> +
>>> sparse [*commit-ish* [.. *commit-ish*]]
>>> ---------------------------------------
>>> Run sparse on the files changed by the given commit range.
>>> --
>>> 2.11.0
>>>
>>> _______________________________________________
>>> dim-tools mailing list
>>> dim-tools@lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/dim-tools
>
> --
> Jani Nikula, Intel Open Source Technology Center
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [DIM PATCH 2/3] dim: add checkpatch profiles to allow different checkpatch options
2018-03-14 6:29 ` Daniel Vetter
@ 2018-03-14 8:57 ` Jani Nikula
0 siblings, 0 replies; 9+ messages in thread
From: Jani Nikula @ 2018-03-14 8:57 UTC (permalink / raw)
To: Daniel Vetter
Cc: intel-gfx, DRM maintainer tools announcements, discussion,
and development, Rodrigo Vivi
On Wed, 14 Mar 2018, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Tue, Mar 13, 2018 at 4:07 PM, Jani Nikula <jani.nikula@intel.com> wrote:
>> On Tue, 13 Mar 2018, Daniel Vetter <daniel@ffwll.ch> wrote:
>>> On Tue, Mar 13, 2018 at 01:30:09PM +0200, Jani Nikula wrote:
>>>> To reduce noise on CI checkpatch reports, we want to silence some
>>>> checkpatch warnings. Different branches may end up having different
>>>> rules, and users may want to get unfiltered results, so introduce
>>>> checkpatch profiles. Add some placeholder profiles to be filled later
>>>> on.
>>>>
>>>> The idea is that CI would run 'dim checkpatch HEAD^ drm-intel' (or some
>>>> other commit range-ish as the case may be).
>>>>
>>>> Cc: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
>>>> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>>>> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
>>>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>>>> ---
>>>> dim | 46 +++++++++++++++++++++++++++++++++++++++++-----
>>>> dim.rst | 9 +++++++--
>>>> 2 files changed, 48 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/dim b/dim
>>>> index 81e2bc1511ac..4ba1c7ff490a 100755
>>>> --- a/dim
>>>> +++ b/dim
>>>> @@ -836,7 +836,7 @@ function apply_patch #patch_file
>>>> rv=1
>>>> fi
>>>>
>>>> - if ! checkpatch_commit HEAD; then
>>>> + if ! checkpatch_commit HEAD branch; then
>>>> rv=1
>>>> fi
>>>> if ! check_maintainer $branch HEAD; then
>>>> @@ -1358,12 +1358,47 @@ function check_maintainer
>>>> }
>>>>
>>>> # $1 is the git sha1 to check
>>>> +# $2 is the checkpatch profile
>>>> function checkpatch_commit
>>>> {
>>>> - local commit rv checkpatch_options
>>>> + local commit rv checkpatch_options profile profile_options
>>>>
>>>> commit=$1
>>>> - checkpatch_options="-q --emacs --strict --show-types -"
>>>> + profile=${2:-default}
>>>> +
>>>> + # special branch profile maps branches to profiles
>>>> + if [[ "$profile" = "branch" ]]; then
>>>> + case "$(git_current_branch)" in
>>>> + drm-intel-next-queued|drm-intel-next-fixes|drm-intel-fixes)
>>>> + profile=drm-intel
>>>> + ;;
>>>> + drm-misc-next|drm-misc-next-fixes|drm-misc-fixes)
>>>> + profile=drm-misc
>>>> + ;;
>>>
>>> Use branch_to_repo instead, if that doesn't come up with anything, then
>>> default?
>>
>> This command is supposed to work without configuration (a developer
>> command)...
>
> Duh :-/
>
> I guess I'm ok as-is, but feels a bit silly.
Thanks, pushed all three. I don't deny it feels a bit silly, but it's
nothing that can't be fixed later.
BR,
Jani.
> -Daniel
>
>>
>> BR,
>> Jani.
>>
>>
>>>
>>> With that little bit of polished applied, on patches 1&2:
>>>
>>> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>>>> + *)
>>>> + profile=default
>>>> + ;;
>>>> + esac
>>>> + fi
>>>> +
>>>> + # map profiles to checkpatch options
>>>> + case "$profile" in
>>>> + default)
>>>> + profile_options=""
>>>> + ;;
>>>> + drm-misc)
>>>> + profile_options=""
>>>> + ;;
>>>> + drm-intel)
>>>> + profile_options=""
>>>> + ;;
>>>> + *)
>>>> + echoerr "Unknown checkpatch profile $profile"
>>>> + profile_options=""
>>>> + ;;
>>>> + esac
>>>> +
>>>> + checkpatch_options="-q --emacs --strict --show-types $profile_options -"
>>>>
>>>> git --no-pager log --oneline -1 $commit
>>>> if ! git show --pretty=email $commit |\
>>>> @@ -1430,12 +1465,13 @@ function dim_extract_next_fixes
>>>> dim_alias_cp=checkpatch
>>>> function dim_checkpatch
>>>> {
>>>> - local range rv
>>>> + local range profile rv
>>>>
>>>> range=$(rangeish "${1:-}")
>>>> + profile=${2:-}
>>>>
>>>> for commit in $(git rev-list --reverse $range); do
>>>> - if ! checkpatch_commit $commit; then
>>>> + if ! checkpatch_commit $commit $profile; then
>>>> rv=1
>>>> fi
>>>> done
>>>> diff --git a/dim.rst b/dim.rst
>>>> index e2c5fd6e6d0a..cc930959e497 100644
>>>> --- a/dim.rst
>>>> +++ b/dim.rst
>>>> @@ -130,13 +130,18 @@ fixes *commit-ish*
>>>> Print the Fixes: and Cc: lines for the supplied *commit-ish* in the linux kernel
>>>> CodingStyle approved format.
>>>>
>>>> -checkpatch [*commit-ish* [.. *commit-ish*]]
>>>> --------------------------------------------
>>>> +checkpatch [*commit-ish* [.. *commit-ish*]] [*profile*]
>>>> +-------------------------------------------------------
>>>> Runs the given commit range commit-ish..commit-ish through the check tools.
>>>>
>>>> If no commit-ish is passed, defaults to HEAD^..HEAD. If one commit-ish is passed
>>>> instead of a range, the range commit-ish..HEAD is used.
>>>>
>>>> +If profile is given, uses specific options for checkpatch error
>>>> +filtering. Current profiles are "default", "branch", "drm-intel", and
>>>> +"drm-misc". The "branch" profile maps the current git branch to the appropriate
>>>> +profile, or if the branch is not known, to "default".
>>>> +
>>>> sparse [*commit-ish* [.. *commit-ish*]]
>>>> ---------------------------------------
>>>> Run sparse on the files changed by the given commit range.
>>>> --
>>>> 2.11.0
>>>>
>>>> _______________________________________________
>>>> dim-tools mailing list
>>>> dim-tools@lists.freedesktop.org
>>>> https://lists.freedesktop.org/mailman/listinfo/dim-tools
>>
>> --
>> Jani Nikula, Intel Open Source Technology Center
--
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [DIM PATCH 3/3] dim: loosen some drm-intel checkpatch rules
2018-03-13 11:55 ` Daniel Vetter
@ 2018-03-14 9:03 ` Jani Nikula
0 siblings, 0 replies; 9+ messages in thread
From: Jani Nikula @ 2018-03-14 9:03 UTC (permalink / raw)
To: Daniel Vetter; +Cc: intel-gfx, dim-tools, Rodrigo Vivi
On Tue, 13 Mar 2018, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Tue, Mar 13, 2018 at 01:30:10PM +0200, Jani Nikula wrote:
>> Set max line length to 100. I don't want to silence the LONG_LINE
>> warning altogether, and I'd still prefer to keep lines under 80
>> characters, but I also don't want to see all the noise, and nor do I
>> want to see silly code trying to arbitrarily squeeze under 80 when it
>> doesn't make sense. 100 is a nice arbitrary round number... I hope
>> review catches silly stuff regardless. Fingers crossed.
>>
>> BIT_MACRO. We have (1 << N) all over the place. I hope to switch to
>> BIT() macro eventually, but this documents current use.
>>
>> PREFER_KERNEL_TYPES. We also have uint(8|16|32|64)_t all over the
>> place. I also hope to move towards kernel types, but this documents
>> current use.
>>
>> SPLIT_STRING, LONG_LINE_STRING. Don't nag about strings split to many
>> lines, but also don't nag about strings not split.
>>
>> There's plenty more that could be tweaked, but let's start with
>> something to improve the S/N ratio of the automated CI checkpatch
>> reports. Now that we have --show-types included in the output, we can
>> more easily discuss the ignores on a case-by-case basis.
>>
>> Cc: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
>> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>> ---
>> dim | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/dim b/dim
>> index 4ba1c7ff490a..9fa6d9cd855b 100755
>> --- a/dim
>> +++ b/dim
>> @@ -1390,7 +1390,7 @@ function checkpatch_commit
>> profile_options=""
>> ;;
>> drm-intel)
>> - profile_options=""
>> + profile_options="--max-line-length=100 --ignore=BIT_MACRO,PREFER_KERNEL_TYPES,SPLIT_STRING,LONG_LINE_STRING"
>
> I've scrolled a bit through checkpatch complaines with this, and I think
> it looks a lot more reasonable. There's also a huge pile of stuff that we
> should probably have fixed when the patches landed, so CI'ing this looks
> good.
>
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>
> Aside: Should we encourage checkpatch patches to clean this up, with the
> note that they must use the drm-intel profile and the caveat that we might
> want to add more stuff to our ignore list instead of taking the patches?
Overall I just think the checkpatch patches crop up enough without
encouragement. IMO let's see how this rolls in CI first and proceed from
there.
The two warnings that I do think would most benefit from mass change are
BIT_MACRO and PREFER_KERNEL_TYPES. People tend to look around them and
cargo cult. A little bit of git grep on the C99 types seems to back this
up; their use multiplies and prospers in some files, while is neglible
in others.
BR,
Jani.
--
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2018-03-14 9:03 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-03-13 11:30 [DIM PATCH 1/3] dim: cleanup checkpatch_commit Jani Nikula
2018-03-13 11:30 ` [DIM PATCH 2/3] dim: add checkpatch profiles to allow different checkpatch options Jani Nikula
2018-03-13 11:48 ` Daniel Vetter
2018-03-13 15:07 ` Jani Nikula
2018-03-14 6:29 ` Daniel Vetter
2018-03-14 8:57 ` Jani Nikula
2018-03-13 11:30 ` [DIM PATCH 3/3] dim: loosen some drm-intel checkpatch rules Jani Nikula
2018-03-13 11:55 ` Daniel Vetter
2018-03-14 9:03 ` Jani Nikula
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.