[v2] scripts/tags.sh: choose which directories to exclude from being indexed

  It's common for drivers that share same physical components to also
duplicate source code (or at least portions of it). A good example is
both drivers/gpu/drm/amdgpu/* and drivers/gpu/drm/radeon/* have a header
file called atombios.h.

While their contents aren't the same, a lot of their structs have
the exact same names which makes navigating through the code base a bit
messy as cscope will show up 'references' across drivers which aren't
exactly correct.

This patch makes it possible for the devs to specify which folders
they don't want to include into database as part of the
find_other_sources func if a makefile variable IGNOREDIRS is present,
otherwise the original behaviour is kept.

Example:
        make ARCH=x86 IGNOREDIRS=drivers/gpu/drm/radeon,tools cscope

Signed-off-by: Paulo Miguel Almeida <paulo.miguel.almeida.rodenas@gmail.com>
---
Changelog:

- v2: change approach to include everything unless specified by the
  IGNOREDIRS variable: (Req: Vipin Sharma)
- v1: https://lore.kernel.org/lkml/Y5OKDvbGk4Kro6MK@mail.google.com/
---
 Documentation/kbuild/kbuild.rst |  7 +++++++
 scripts/tags.sh                 | 11 +++++++++--
 2 files changed, 16 insertions(+), 2 deletions(-)
  

On Sun, Dec 11, 2022 at 12:02:18PM +1300, Paulo Miguel Almeida wrote:
> This patch makes it possible for the devs to specify which folders
> they don't want to include into database as part of the
> find_other_sources func if a makefile variable IGNOREDIRS is present,
> otherwise the original behaviour is kept.

Better say "Add IGNOREDIRS variable, which specifies which directories
to be ignored from indexing."

> @@ -278,6 +278,13 @@ To get all available archs you can also specify all. E.g.::
>  
>      $ make ALLSOURCE_ARCHS=all tags
>  
> +IGNOREDIRS
> +---------------
> +For tags/TAGS/cscope targets, you can choose which directories won't
> +be included in the databases, separated by comma. E.g.:
> +
> +    $ make IGNOREDIRS=drivers/gpu/drm/radeon,tools cscope
> +

Use two-colon syntax (::) for code block above to be consistent with
other code blocks.

Thanks.
  

On Sun, Dec 11, 2022 at 11:21:33AM +0700, Bagas Sanjaya wrote:
> On Sun, Dec 11, 2022 at 12:02:18PM +1300, Paulo Miguel Almeida wrote:
> > This patch makes it possible for the devs to specify which folders
> > they don't want to include into database as part of the
> > find_other_sources func if a makefile variable IGNOREDIRS is present,
> > otherwise the original behaviour is kept.
> 
> Better say "Add IGNOREDIRS variable, which specifies which directories
> to be ignored from indexing."
> 
> > @@ -278,6 +278,13 @@ To get all available archs you can also specify all. E.g.::
> >  
> >      $ make ALLSOURCE_ARCHS=all tags
> >  
> > +IGNOREDIRS
> > +---------------
> > +For tags/TAGS/cscope targets, you can choose which directories won't
> > +be included in the databases, separated by comma. E.g.:
> > +
> > +    $ make IGNOREDIRS=drivers/gpu/drm/radeon,tools cscope
> > +
> 
> Use two-colon syntax (::) for code block above to be consistent with
> other code blocks.
> 
> Thanks. 
> 
> -- 
> An old man doll... just what I always wanted! - Clara

Hi Bagas,

thanks for taking the time to review this patch. I will make the changes
that you've pointed out.

thanks!

- Paulo A,
  

On Sat, Dec 10, 2022 at 3:02 PM Paulo Miguel Almeida
<paulo.miguel.almeida.rodenas@gmail.com> wrote:
>
> It's common for drivers that share same physical components to also
> duplicate source code (or at least portions of it). A good example is
> both drivers/gpu/drm/amdgpu/* and drivers/gpu/drm/radeon/* have a header
> file called atombios.h.
>
> While their contents aren't the same, a lot of their structs have
> the exact same names which makes navigating through the code base a bit
> messy as cscope will show up 'references' across drivers which aren't
> exactly correct.
>
> This patch makes it possible for the devs to specify which folders
> they don't want to include into database as part of the
> find_other_sources func if a makefile variable IGNOREDIRS is present,
> otherwise the original behaviour is kept.
>
> Example:
>         make ARCH=x86 IGNOREDIRS=drivers/gpu/drm/radeon,tools cscope
>
> Signed-off-by: Paulo Miguel Almeida <paulo.miguel.almeida.rodenas@gmail.com>
> ---
> Changelog:
>
> - v2: change approach to include everything unless specified by the
>   IGNOREDIRS variable: (Req: Vipin Sharma)
> - v1: https://lore.kernel.org/lkml/Y5OKDvbGk4Kro6MK@mail.google.com/
> ---
>  Documentation/kbuild/kbuild.rst |  7 +++++++
>  scripts/tags.sh                 | 11 +++++++++--
>  2 files changed, 16 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/kbuild/kbuild.rst b/Documentation/kbuild/kbuild.rst
> index 08f575e6236c..5f99f30e20d8 100644
> --- a/Documentation/kbuild/kbuild.rst
> +++ b/Documentation/kbuild/kbuild.rst
> @@ -278,6 +278,13 @@ To get all available archs you can also specify all. E.g.::
>
>      $ make ALLSOURCE_ARCHS=all tags
>
> +IGNOREDIRS
> +---------------
> +For tags/TAGS/cscope targets, you can choose which directories won't
> +be included in the databases, separated by comma. E.g.:
> +
> +    $ make IGNOREDIRS=drivers/gpu/drm/radeon,tools cscope
> +
>  KBUILD_BUILD_TIMESTAMP
>  ----------------------
>  Setting this to a date string overrides the timestamp used in the
> diff --git a/scripts/tags.sh b/scripts/tags.sh
> index e137cf15aae9..554721e9cad2 100755
> --- a/scripts/tags.sh
> +++ b/scripts/tags.sh
> @@ -59,10 +59,17 @@ find_include_sources()
>  }
>
>  # find sources in rest of tree
> -# we could benefit from a list of dirs to search in here
>  find_other_sources()
>  {
> -       find ${tree}* $ignore \
> +       local loc_ignore=${ignore}
> +       if [ -n "${IGNOREDIRS}" ]; then
> +               exp_ignored_dirs=$(sed 's/,/ /g' <<< ${IGNOREDIRS})
> +               for i in ${exp_ignored_dirs}; do
> +                       loc_ignore="${loc_ignore} ( -path $i ) -prune -o"
> +               done
> +       fi
> +

This should be global overwrite instead of just in this function.
Before find_other_sources() is executed, this script finds files in
arch directories. So, if you keep it local then those files cannot be
excluded which makes execution of the command incorrect:

make IGNOREDIRS=arch/x86 cscope

Above command will still index all of the code in arch/x86. Something
like this will be better.

--- a/scripts/tags.sh
+++ b/scripts/tags.sh
@@ -17,6 +17,13 @@ ignore="$(echo "$RCS_FIND_IGNORE" | sed 's|\\||g' )"
 # tags and cscope files should also ignore MODVERSION *.mod.c files
 ignore="$ignore ( -name *.mod.c ) -prune -o"

+if [ -n "${IGNOREDIRS}" ]; then
+       exp_ignored_dirs=$(sed 's/,/ /g' <<< ${IGNOREDIRS})
+       for i in ${exp_ignored_dirs}; do
+               ignore="${ignore} ( -path $i ) -prune -o"
+       done
+fi
+
 # Use make KBUILD_ABS_SRCTREE=1 {tags|cscope}
 # to force full paths for a non-O= build
 if [ "${srctree}" = "." -o -z "${srctree}" ]; then
@@ -62,9 +69,9 @@ find_include_sources()
 # we could benefit from a list of dirs to search in here
 find_other_sources()
 {
-       find ${tree}* $ignore \
-            \( -path ${tree}include -o -path ${tree}arch -o -name
'.tmp_*' \) -prune -o \
-              -name "$1" -not -type l -print;
+       find ${tree}* ${ignore} \
+               \( -path ${tree}include -o -path ${tree}arch -o -name
'.tmp_*' \) -prune -o \
+               -name "$1" -not -type l -print;
 }

We will still have to specify arch/x86 and arch/x86/include but this
works and keeps the definition of IGNOREDIRS relatively correct.


> +       find ${tree}* ${loc_ignore} \
>              \( -path ${tree}include -o -path ${tree}arch -o -name '.tmp_*' \) -prune -o \
>                -name "$1" -not -type l -print;
>  }
> --
> 2.38.1
>
  

On Mon, Dec 12, 2022 at 01:27:37PM -0800, Vipin Sharma wrote:
> On Sat, Dec 10, 2022 at 3:02 PM Paulo Miguel Almeida
> <paulo.miguel.almeida.rodenas@gmail.com> wrote:
> >  # find sources in rest of tree
> > -# we could benefit from a list of dirs to search in here
> >  find_other_sources()
> >  {
> > -       find ${tree}* $ignore \
> > +       local loc_ignore=${ignore}
> > +       if [ -n "${IGNOREDIRS}" ]; then
> > +               exp_ignored_dirs=$(sed 's/,/ /g' <<< ${IGNOREDIRS})
> > +               for i in ${exp_ignored_dirs}; do
> > +                       loc_ignore="${loc_ignore} ( -path $i ) -prune -o"
> > +               done
> > +       fi
> > +
> 
> This should be global overwrite instead of just in this function.
> Before find_other_sources() is executed, this script finds files in
> arch directories. So, if you keep it local then those files cannot be
> excluded which makes execution of the command incorrect:
> 
> make IGNOREDIRS=arch/x86 cscope
> 

Hi Vipin, thanks for taking the time to review this patch.

I see where you are coming from. I was aware of the 'loophole' that the
current approach could have but, to be honest, I thought that there
would be very little use in being able to exclude arch/.*?/ files.

The reason for that being that I thought the most common usage for this
feature would be to ignore folders within subsystems like drivers and
tools to ensure code navigation would be less 'messy'.

Additionally, if we go with the global IGNOREDIRS approach you just
described, we could have some conflicting options too such as:

make ALLSOURCE_ARCHS="x86 arm" IGNOREDIRS=arch/x86 cscope

My 2 cents is that ALLSOURCE_ARCHS is already the mechanism for
excluding archs so it's 'okay' to keep IGNOREDIRS as is.

Let me know your thoughts.

Thanks!

- Paulo A.

> Above command will still index all of the code in arch/x86. Something
> like this will be better.
> 
> --- a/scripts/tags.sh
> +++ b/scripts/tags.sh
> @@ -17,6 +17,13 @@ ignore="$(echo "$RCS_FIND_IGNORE" | sed 's|\\||g' )"
>  # tags and cscope files should also ignore MODVERSION *.mod.c files
>  ignore="$ignore ( -name *.mod.c ) -prune -o"
> 
> +if [ -n "${IGNOREDIRS}" ]; then
> +       exp_ignored_dirs=$(sed 's/,/ /g' <<< ${IGNOREDIRS})
> +       for i in ${exp_ignored_dirs}; do
> +               ignore="${ignore} ( -path $i ) -prune -o"
> +       done
> +fi
> +
>  # Use make KBUILD_ABS_SRCTREE=1 {tags|cscope}
>  # to force full paths for a non-O= build
>  if [ "${srctree}" = "." -o -z "${srctree}" ]; then
> @@ -62,9 +69,9 @@ find_include_sources()
>  # we could benefit from a list of dirs to search in here
>  find_other_sources()
>  {
> -       find ${tree}* $ignore \
> -            \( -path ${tree}include -o -path ${tree}arch -o -name
> '.tmp_*' \) -prune -o \
> -              -name "$1" -not -type l -print;
> +       find ${tree}* ${ignore} \
> +               \( -path ${tree}include -o -path ${tree}arch -o -name
> '.tmp_*' \) -prune -o \
> +               -name "$1" -not -type l -print;
>  }
> 
> We will still have to specify arch/x86 and arch/x86/include but this
> works and keeps the definition of IGNOREDIRS relatively correct.
> 
> 
> > +       find ${tree}* ${loc_ignore} \
> >              \( -path ${tree}include -o -path ${tree}arch -o -name '.tmp_*' \) -prune -o \
> >                -name "$1" -not -type l -print;
> >  }
> > --
> > 2.38.1
> >
  

On Mon, Dec 12, 2022 at 1:59 PM Paulo Miguel Almeida
<paulo.miguel.almeida.rodenas@gmail.com> wrote:
>
> On Mon, Dec 12, 2022 at 01:27:37PM -0800, Vipin Sharma wrote:
> > On Sat, Dec 10, 2022 at 3:02 PM Paulo Miguel Almeida
> > <paulo.miguel.almeida.rodenas@gmail.com> wrote:
> > >  # find sources in rest of tree
> > > -# we could benefit from a list of dirs to search in here
> > >  find_other_sources()
> > >  {
> > > -       find ${tree}* $ignore \
> > > +       local loc_ignore=${ignore}
> > > +       if [ -n "${IGNOREDIRS}" ]; then
> > > +               exp_ignored_dirs=$(sed 's/,/ /g' <<< ${IGNOREDIRS})
> > > +               for i in ${exp_ignored_dirs}; do
> > > +                       loc_ignore="${loc_ignore} ( -path $i ) -prune -o"
> > > +               done
> > > +       fi
> > > +
> >
> > This should be global overwrite instead of just in this function.
> > Before find_other_sources() is executed, this script finds files in
> > arch directories. So, if you keep it local then those files cannot be
> > excluded which makes execution of the command incorrect:
> >
> > make IGNOREDIRS=arch/x86 cscope
> >
>
> Hi Vipin, thanks for taking the time to review this patch.
>
> I see where you are coming from. I was aware of the 'loophole' that the
> current approach could have but, to be honest, I thought that there
> would be very little use in being able to exclude arch/.*?/ files.
>
> The reason for that being that I thought the most common usage for this
> feature would be to ignore folders within subsystems like drivers and
> tools to ensure code navigation would be less 'messy'.

Yes, the original intent was to make driver code browsing less messy
but if we are introducing an option we should adapt it for generic
cases and correct the semantics.

>
> Additionally, if we go with the global IGNOREDIRS approach you just
> described, we could have some conflicting options too such as:
>
> make ALLSOURCE_ARCHS="x86 arm" IGNOREDIRS=arch/x86 cscope
>

I don't think this is conflicting, to me it is more complementary.
Above line shows get all code for x86 and arm but don't get x86 source
code ("arch/x86/include" is fine). This can even be fine tuned to sub
directories.

I just now noticed after seeing your command, ALLSOURCE_ARCHS take
space separated values, whereas, IGNOREDIRS take comma separated
values. They both should be in the same format, since ALLSOURCE_ARCHS
was already there, it is better to change IGNOREDIRS.

Can you also change IGNOREDIRS to IGNORE_DIRS? It is much easier to
read this way. Sorry, I should have said this in the  beginning.

> My 2 cents is that ALLSOURCE_ARCHS is already the mechanism for
> excluding archs so it's 'okay' to keep IGNOREDIRS as is.
>
> Let me know your thoughts.
>
> Thanks!
>
> - Paulo A.
>
> > Above command will still index all of the code in arch/x86. Something
> > like this will be better.
> >
> > --- a/scripts/tags.sh
> > +++ b/scripts/tags.sh
> > @@ -17,6 +17,13 @@ ignore="$(echo "$RCS_FIND_IGNORE" | sed 's|\\||g' )"
> >  # tags and cscope files should also ignore MODVERSION *.mod.c files
> >  ignore="$ignore ( -name *.mod.c ) -prune -o"
> >
> > +if [ -n "${IGNOREDIRS}" ]; then
> > +       exp_ignored_dirs=$(sed 's/,/ /g' <<< ${IGNOREDIRS})
> > +       for i in ${exp_ignored_dirs}; do
> > +               ignore="${ignore} ( -path $i ) -prune -o"
> > +       done
> > +fi
> > +
> >  # Use make KBUILD_ABS_SRCTREE=1 {tags|cscope}
> >  # to force full paths for a non-O= build
> >  if [ "${srctree}" = "." -o -z "${srctree}" ]; then
> > @@ -62,9 +69,9 @@ find_include_sources()
> >  # we could benefit from a list of dirs to search in here
> >  find_other_sources()
> >  {
> > -       find ${tree}* $ignore \
> > -            \( -path ${tree}include -o -path ${tree}arch -o -name
> > '.tmp_*' \) -prune -o \
> > -              -name "$1" -not -type l -print;
> > +       find ${tree}* ${ignore} \
> > +               \( -path ${tree}include -o -path ${tree}arch -o -name
> > '.tmp_*' \) -prune -o \
> > +               -name "$1" -not -type l -print;
> >  }
> >
> > We will still have to specify arch/x86 and arch/x86/include but this
> > works and keeps the definition of IGNOREDIRS relatively correct.
> >
> >
> > > +       find ${tree}* ${loc_ignore} \
> > >              \( -path ${tree}include -o -path ${tree}arch -o -name '.tmp_*' \) -prune -o \
> > >                -name "$1" -not -type l -print;
> > >  }
> > > --
> > > 2.38.1
> > >
  

On Mon, Dec 12, 2022 at 02:32:41PM -0800, Vipin Sharma wrote:
> On Mon, Dec 12, 2022 at 1:59 PM Paulo Miguel Almeida
> <paulo.miguel.almeida.rodenas@gmail.com> wrote:
> >
> > On Mon, Dec 12, 2022 at 01:27:37PM -0800, Vipin Sharma wrote:
> > > On Sat, Dec 10, 2022 at 3:02 PM Paulo Miguel Almeida
> > > <paulo.miguel.almeida.rodenas@gmail.com> wrote:
> > > >  # find sources in rest of tree
> > > > -# we could benefit from a list of dirs to search in here
> > > >  find_other_sources()
> > > >  {
> > > > -       find ${tree}* $ignore \
> > > > +       local loc_ignore=${ignore}
> > > > +       if [ -n "${IGNOREDIRS}" ]; then
> > > > +               exp_ignored_dirs=$(sed 's/,/ /g' <<< ${IGNOREDIRS})
> > > > +               for i in ${exp_ignored_dirs}; do
> > > > +                       loc_ignore="${loc_ignore} ( -path $i ) -prune -o"
> > > > +               done
> > > > +       fi
> > > > +
> > >
> > > This should be global overwrite instead of just in this function.
> > > Before find_other_sources() is executed, this script finds files in
> > > arch directories. So, if you keep it local then those files cannot be
> > > excluded which makes execution of the command incorrect:
> > >
> > > make IGNOREDIRS=arch/x86 cscope
> > >
> >
> > Hi Vipin, thanks for taking the time to review this patch.
> >
> > I see where you are coming from. I was aware of the 'loophole' that the
> > current approach could have but, to be honest, I thought that there
> > would be very little use in being able to exclude arch/.*?/ files.
> >
> > The reason for that being that I thought the most common usage for this
> > feature would be to ignore folders within subsystems like drivers and
> > tools to ensure code navigation would be less 'messy'.
> 
> Yes, the original intent was to make driver code browsing less messy
> but if we are introducing an option we should adapt it for generic
> cases and correct the semantics.

Agreed.

> 
> >
> > Additionally, if we go with the global IGNOREDIRS approach you just
> > described, we could have some conflicting options too such as:
> >
> > make ALLSOURCE_ARCHS="x86 arm" IGNOREDIRS=arch/x86 cscope
> >
> 
> I don't think this is conflicting, to me it is more complementary.
> Above line shows get all code for x86 and arm but don't get x86 source
> code ("arch/x86/include" is fine). This can even be fine tuned to sub
> directories.
> 

That's a fair point. I had not thought about it that way. Thanks!

Will implement the changes when I get home.

> I just now noticed after seeing your command, ALLSOURCE_ARCHS take
> space separated values, whereas, IGNOREDIRS take comma separated
> values. They both should be in the same format, since ALLSOURCE_ARCHS
> was already there, it is better to change IGNOREDIRS.
> 
> Can you also change IGNOREDIRS to IGNORE_DIRS? It is much easier to
> read this way. Sorry, I should have said this in the  beginning.
> 

Yep, no problem! :-)

> > My 2 cents is that ALLSOURCE_ARCHS is already the mechanism for
> > excluding archs so it's 'okay' to keep IGNOREDIRS as is.
> >
> > Let me know your thoughts.
> >
> > Thanks!
> >
> > - Paulo A.
> >
> > > Above command will still index all of the code in arch/x86. Something
> > > like this will be better.
> > >
> > > --- a/scripts/tags.sh
> > > +++ b/scripts/tags.sh
> > > @@ -17,6 +17,13 @@ ignore="$(echo "$RCS_FIND_IGNORE" | sed 's|\\||g' )"
> > >  # tags and cscope files should also ignore MODVERSION *.mod.c files
> > >  ignore="$ignore ( -name *.mod.c ) -prune -o"
> > >
> > > +if [ -n "${IGNOREDIRS}" ]; then
> > > +       exp_ignored_dirs=$(sed 's/,/ /g' <<< ${IGNOREDIRS})
> > > +       for i in ${exp_ignored_dirs}; do
> > > +               ignore="${ignore} ( -path $i ) -prune -o"
> > > +       done
> > > +fi
> > > +
> > >  # Use make KBUILD_ABS_SRCTREE=1 {tags|cscope}
> > >  # to force full paths for a non-O= build
> > >  if [ "${srctree}" = "." -o -z "${srctree}" ]; then
> > > @@ -62,9 +69,9 @@ find_include_sources()
> > >  # we could benefit from a list of dirs to search in here
> > >  find_other_sources()
> > >  {
> > > -       find ${tree}* $ignore \
> > > -            \( -path ${tree}include -o -path ${tree}arch -o -name
> > > '.tmp_*' \) -prune -o \
> > > -              -name "$1" -not -type l -print;
> > > +       find ${tree}* ${ignore} \
> > > +               \( -path ${tree}include -o -path ${tree}arch -o -name
> > > '.tmp_*' \) -prune -o \
> > > +               -name "$1" -not -type l -print;
> > >  }
> > >
> > > We will still have to specify arch/x86 and arch/x86/include but this
> > > works and keeps the definition of IGNOREDIRS relatively correct.
> > >
> > >
> > > > +       find ${tree}* ${loc_ignore} \
> > > >              \( -path ${tree}include -o -path ${tree}arch -o -name '.tmp_*' \) -prune -o \
> > > >                -name "$1" -not -type l -print;
> > > >  }
> > > > --
> > > > 2.38.1
> > > >