[PATCH] xen/build: Fix `make cscope` rune

[PATCH] xen/build: Fix `make cscope` rune

[Andrew Cooper]

There are two problems, both in the all_sources definition.

First, everything in arch/*/include gets double hits with cscope queries,
because they end up getting listed twice in cscope.files.

Drop the first `find` rune of the three, because it's redundant with the third
rune following c/s 725381a5eab3 ("xen: move include/asm-* to
arch/*/include/asm").

Second, and this way for a long time:

  $ make cscope
  ( find arch/x86/include -name '*.h' -print; find include -name '*.h' -print;
  find xsm arch/x86 common drivers lib test -name '*.[chS]' -print ) >
  cscope.files
  cscope -k -b -q
  cscope: cannot find file arch/x86/efi/efi.h
  cscope: cannot find file arch/x86/efi/ebmalloc.c
  cscope: cannot find file arch/x86/efi/compat.c
  cscope: cannot find file arch/x86/efi/pe.c
  cscope: cannot find file arch/x86/efi/boot.c
  cscope: cannot find file arch/x86/efi/runtime.c

This is caused by these being symlinks to common/efi.  Restrict all find runes
to `-type f` to skip symlinks, because common/efi/*.c are already listed.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Anthony PERARD <anthony.perard@citrix.com>
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Julien Grall <julien@xen.org>
CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
CC: Bertrand Marquis <bertrand.marquis@arm.com>

Anthony: I looked through the remainder of your build series and I cant spot
any edits to all_sources.  Apologies if I missed it.
---
 xen/Makefile | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/xen/Makefile b/xen/Makefile
index 2ad7da7ad67b..dc6bdc44c7a2 100644
--- a/xen/Makefile
+++ b/xen/Makefile
@@ -468,9 +468,8 @@ arch/$(TARGET_ARCH)/include/asm/asm-offsets.h: asm-offsets.s
 
 SUBDIRS = xsm arch/$(TARGET_ARCH) common drivers lib test
 define all_sources
-    ( find arch/$(TARGET_ARCH)/include -name '*.h' -print; \
-      find include -name '*.h' -print; \
-      find $(SUBDIRS) -name '*.[chS]' -print )
+    ( find include -type f -name '*.h' -print; \
+      find $(SUBDIRS) -type f -name '*.[chS]' -print )
 endef
 
 define set_exuberant_flags
-- 
2.11.0

Re: [PATCH] xen/build: Fix `make cscope` rune

[Jan Beulich]

On 16.12.2021 10:20, Andrew Cooper wrote:
> There are two problems, both in the all_sources definition.
> 
> First, everything in arch/*/include gets double hits with cscope queries,
> because they end up getting listed twice in cscope.files.
> 
> Drop the first `find` rune of the three, because it's redundant with the third
> rune following c/s 725381a5eab3 ("xen: move include/asm-* to
> arch/*/include/asm").

I'm certainly fine with this part.

> Second, and this way for a long time:
> 
>   $ make cscope
>   ( find arch/x86/include -name '*.h' -print; find include -name '*.h' -print;
>   find xsm arch/x86 common drivers lib test -name '*.[chS]' -print ) >
>   cscope.files
>   cscope -k -b -q
>   cscope: cannot find file arch/x86/efi/efi.h
>   cscope: cannot find file arch/x86/efi/ebmalloc.c
>   cscope: cannot find file arch/x86/efi/compat.c
>   cscope: cannot find file arch/x86/efi/pe.c
>   cscope: cannot find file arch/x86/efi/boot.c
>   cscope: cannot find file arch/x86/efi/runtime.c
> 
> This is caused by these being symlinks to common/efi.  Restrict all find runes
> to `-type f` to skip symlinks, because common/efi/*.c are already listed.

I have reservations here, albeit of theoretical nature as long as only
the csope target is affected (simply because I don't use it): Make
rules should really be independent of a dir entry being a real file or
a symlink. I did run into problems with that already years ago when
the shim was introduced. My arrangements heavily use symlinking, and
any assumption on files being "real" ones will break this. At the very
least symlink checks should be restricted to cover only relative ones;
ideally one would distinguish ones staying within the tree vs ones
reaching to the "outside".

> --- a/xen/Makefile
> +++ b/xen/Makefile
> @@ -468,9 +468,8 @@ arch/$(TARGET_ARCH)/include/asm/asm-offsets.h: asm-offsets.s
>  
>  SUBDIRS = xsm arch/$(TARGET_ARCH) common drivers lib test
>  define all_sources
> -    ( find arch/$(TARGET_ARCH)/include -name '*.h' -print; \
> -      find include -name '*.h' -print; \
> -      find $(SUBDIRS) -name '*.[chS]' -print )
> +    ( find include -type f -name '*.h' -print; \
> +      find $(SUBDIRS) -type f -name '*.[chS]' -print )
>  endef

I further wonder how use of $(TARGET_ARCH) can be correct here. Why
would the enumeration of items here be limited to a particular arch?
When you edit files in the source tree, everything should be covered.
Restriction to a particular arch only makes sense in a build tree.

Jan

Re: [PATCH] xen/build: Fix `make cscope` rune

[Andrew Cooper]

On 16/12/2021 14:00, Jan Beulich wrote:
> On 16.12.2021 10:20, Andrew Cooper wrote:
>> Second, and this way for a long time:
>>
>>   $ make cscope
>>   ( find arch/x86/include -name '*.h' -print; find include -name '*.h' -print;
>>   find xsm arch/x86 common drivers lib test -name '*.[chS]' -print ) >
>>   cscope.files
>>   cscope -k -b -q
>>   cscope: cannot find file arch/x86/efi/efi.h
>>   cscope: cannot find file arch/x86/efi/ebmalloc.c
>>   cscope: cannot find file arch/x86/efi/compat.c
>>   cscope: cannot find file arch/x86/efi/pe.c
>>   cscope: cannot find file arch/x86/efi/boot.c
>>   cscope: cannot find file arch/x86/efi/runtime.c
>>
>> This is caused by these being symlinks to common/efi.  Restrict all find runes
>> to `-type f` to skip symlinks, because common/efi/*.c are already listed.
> I have reservations here, albeit of theoretical nature as long as only
> the csope target is affected (simply because I don't use it): Make
> rules should really be independent of a dir entry being a real file or
> a symlink. I did run into problems with that already years ago when
> the shim was introduced. My arrangements heavily use symlinking, and
> any assumption on files being "real" ones will break this. At the very
> least symlink checks should be restricted to cover only relative ones;
> ideally one would distinguish ones staying within the tree vs ones
> reaching to the "outside".

all_sources is used exclusively for "tags" purposes; the
TAGS/tags/gtags/cscope targets.

Personally, I'd prefer there to not be symlinks in the first place.  The
EFI logic is unnecessarily complicated to navigate.

But the reality is that inter-xen/ symlinks for source files are also a
duplication as far as these `find` runes are concerned.

Apparently tags et al will follow symlinks, while there's no obviously
help online about cscope, other than "resolve your symlinks first".

In either case, you don't want to end up with both the regular path, and
the symlink, ending up in the file list.


I don't anticipate the usecase for all_source changing, nor the way we
symlink things, so I think sticking with `-type f` is the appropriate
action.

Furthermore, you really don't want a directory (e.g. include/foo.d )
getting into the file list either.

>> --- a/xen/Makefile
>> +++ b/xen/Makefile
>> @@ -468,9 +468,8 @@ arch/$(TARGET_ARCH)/include/asm/asm-offsets.h: asm-offsets.s
>>  
>>  SUBDIRS = xsm arch/$(TARGET_ARCH) common drivers lib test
>>  define all_sources
>> -    ( find arch/$(TARGET_ARCH)/include -name '*.h' -print; \
>> -      find include -name '*.h' -print; \
>> -      find $(SUBDIRS) -name '*.[chS]' -print )
>> +    ( find include -type f -name '*.h' -print; \
>> +      find $(SUBDIRS) -type f -name '*.[chS]' -print )
>>  endef
> I further wonder how use of $(TARGET_ARCH) can be correct here. Why
> would the enumeration of items here be limited to a particular arch?
> When you edit files in the source tree, everything should be covered.
> Restriction to a particular arch only makes sense in a build tree.

I'd say that's a matter of opinion.  There are times when I'd quite like
it to be all arch's, and there are other times when I'm very glad it's not.

Either way, I don't intend to address that rats nest in this patch.

~Andrew

Re: [PATCH] xen/build: Fix `make cscope` rune

[Jan Beulich]

On 16.12.2021 15:58, Andrew Cooper wrote:
> On 16/12/2021 14:00, Jan Beulich wrote:
>> On 16.12.2021 10:20, Andrew Cooper wrote:
>>> Second, and this way for a long time:
>>>
>>>   $ make cscope
>>>   ( find arch/x86/include -name '*.h' -print; find include -name '*.h' -print;
>>>   find xsm arch/x86 common drivers lib test -name '*.[chS]' -print ) >
>>>   cscope.files
>>>   cscope -k -b -q
>>>   cscope: cannot find file arch/x86/efi/efi.h
>>>   cscope: cannot find file arch/x86/efi/ebmalloc.c
>>>   cscope: cannot find file arch/x86/efi/compat.c
>>>   cscope: cannot find file arch/x86/efi/pe.c
>>>   cscope: cannot find file arch/x86/efi/boot.c
>>>   cscope: cannot find file arch/x86/efi/runtime.c
>>>
>>> This is caused by these being symlinks to common/efi.  Restrict all find runes
>>> to `-type f` to skip symlinks, because common/efi/*.c are already listed.
>> I have reservations here, albeit of theoretical nature as long as only
>> the csope target is affected (simply because I don't use it): Make
>> rules should really be independent of a dir entry being a real file or
>> a symlink. I did run into problems with that already years ago when
>> the shim was introduced. My arrangements heavily use symlinking, and
>> any assumption on files being "real" ones will break this. At the very
>> least symlink checks should be restricted to cover only relative ones;
>> ideally one would distinguish ones staying within the tree vs ones
>> reaching to the "outside".
> 
> all_sources is used exclusively for "tags" purposes; the
> TAGS/tags/gtags/cscope targets.
> 
> Personally, I'd prefer there to not be symlinks in the first place.  The
> EFI logic is unnecessarily complicated to navigate.
> 
> But the reality is that inter-xen/ symlinks for source files are also a
> duplication as far as these `find` runes are concerned.

DYM intra-xen/ symlinks? Else I'm afraid I'm not following.

> Apparently tags et al will follow symlinks, while there's no obviously
> help online about cscope, other than "resolve your symlinks first".
> 
> In either case, you don't want to end up with both the regular path, and
> the symlink, ending up in the file list.
> 
> 
> I don't anticipate the usecase for all_source changing, nor the way we
> symlink things, so I think sticking with `-type f` is the appropriate
> action.

Well, as said - I disagree, but as long as only targets I don't care
about are affected, I guess I'll let you do what you want done.

> Furthermore, you really don't want a directory (e.g. include/foo.d )
> getting into the file list either.

I certainly agree with you here.

Jan

Re: [PATCH] xen/build: Fix `make cscope` rune

[Andrew Cooper]

On 16/12/2021 15:15, Jan Beulich wrote:
> On 16.12.2021 15:58, Andrew Cooper wrote:
>> On 16/12/2021 14:00, Jan Beulich wrote:
>>> On 16.12.2021 10:20, Andrew Cooper wrote:
>>>> Second, and this way for a long time:
>>>>
>>>>   $ make cscope
>>>>   ( find arch/x86/include -name '*.h' -print; find include -name '*.h' -print;
>>>>   find xsm arch/x86 common drivers lib test -name '*.[chS]' -print ) >
>>>>   cscope.files
>>>>   cscope -k -b -q
>>>>   cscope: cannot find file arch/x86/efi/efi.h
>>>>   cscope: cannot find file arch/x86/efi/ebmalloc.c
>>>>   cscope: cannot find file arch/x86/efi/compat.c
>>>>   cscope: cannot find file arch/x86/efi/pe.c
>>>>   cscope: cannot find file arch/x86/efi/boot.c
>>>>   cscope: cannot find file arch/x86/efi/runtime.c
>>>>
>>>> This is caused by these being symlinks to common/efi.  Restrict all find runes
>>>> to `-type f` to skip symlinks, because common/efi/*.c are already listed.
>>> I have reservations here, albeit of theoretical nature as long as only
>>> the csope target is affected (simply because I don't use it): Make
>>> rules should really be independent of a dir entry being a real file or
>>> a symlink. I did run into problems with that already years ago when
>>> the shim was introduced. My arrangements heavily use symlinking, and
>>> any assumption on files being "real" ones will break this. At the very
>>> least symlink checks should be restricted to cover only relative ones;
>>> ideally one would distinguish ones staying within the tree vs ones
>>> reaching to the "outside".
>> all_sources is used exclusively for "tags" purposes; the
>> TAGS/tags/gtags/cscope targets.
>>
>> Personally, I'd prefer there to not be symlinks in the first place.  The
>> EFI logic is unnecessarily complicated to navigate.
>>
>> But the reality is that inter-xen/ symlinks for source files are also a
>> duplication as far as these `find` runes are concerned.
> DYM intra-xen/ symlinks? Else I'm afraid I'm not following.

I do, yes.  I'll find some more coffee.

>
>> Apparently tags et al will follow symlinks, while there's no obviously
>> help online about cscope, other than "resolve your symlinks first".
>>
>> In either case, you don't want to end up with both the regular path, and
>> the symlink, ending up in the file list.
>>
>>
>> I don't anticipate the usecase for all_source changing, nor the way we
>> symlink things, so I think sticking with `-type f` is the appropriate
>> action.
> Well, as said - I disagree, but as long as only targets I don't care
> about are affected, I guess I'll let you do what you want done.

Thanks.

>
>> Furthermore, you really don't want a directory (e.g. include/foo.d )
>> getting into the file list either.
> I certainly agree with you here.

Reading back, I actually meant foo.h/ as a dir, but the intent wasn't lost.

~Andrew

Re: [PATCH] xen/build: Fix `make cscope` rune

[Anthony PERARD]

On Thu, Dec 16, 2021 at 09:20:14AM +0000, Andrew Cooper wrote:
> There are two problems, both in the all_sources definition.
> 
> First, everything in arch/*/include gets double hits with cscope queries,
> because they end up getting listed twice in cscope.files.
> 
> Drop the first `find` rune of the three, because it's redundant with the third
> rune following c/s 725381a5eab3 ("xen: move include/asm-* to
> arch/*/include/asm").
> 
> Second, and this way for a long time:
> 
>   $ make cscope
>   ( find arch/x86/include -name '*.h' -print; find include -name '*.h' -print;
>   find xsm arch/x86 common drivers lib test -name '*.[chS]' -print ) >
>   cscope.files
>   cscope -k -b -q
>   cscope: cannot find file arch/x86/efi/efi.h
>   cscope: cannot find file arch/x86/efi/ebmalloc.c
>   cscope: cannot find file arch/x86/efi/compat.c
>   cscope: cannot find file arch/x86/efi/pe.c
>   cscope: cannot find file arch/x86/efi/boot.c
>   cscope: cannot find file arch/x86/efi/runtime.c

It's kind of weird that cscope can't read symlinks, but I guess that the
way it is.

> This is caused by these being symlinks to common/efi.  Restrict all find runes
> to `-type f` to skip symlinks, because common/efi/*.c are already listed.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Anthony PERARD <anthony.perard@citrix.com>

> Anthony: I looked through the remainder of your build series and I cant spot
> any edits to all_sources.  Apologies if I missed it.

I don't think I've made further edit of this.

Thanks,

-- 
Anthony PERARD

Re: [PATCH] xen/build: Fix `make cscope` rune

[Volodymyr Babchuk]

Hi Andrew,

Andrew Cooper <andrew.cooper3@citrix.com> writes:

> There are two problems, both in the all_sources definition.

As a cscope user I want to thank you for the fix.

>
> First, everything in arch/*/include gets double hits with cscope queries,
> because they end up getting listed twice in cscope.files.
>
> Drop the first `find` rune of the three, because it's redundant with the third
> rune following c/s 725381a5eab3 ("xen: move include/asm-* to
> arch/*/include/asm").
>
> Second, and this way for a long time:
>
>   $ make cscope
>   ( find arch/x86/include -name '*.h' -print; find include -name '*.h' -print;
>   find xsm arch/x86 common drivers lib test -name '*.[chS]' -print ) >
>   cscope.files
>   cscope -k -b -q
>   cscope: cannot find file arch/x86/efi/efi.h
>   cscope: cannot find file arch/x86/efi/ebmalloc.c
>   cscope: cannot find file arch/x86/efi/compat.c
>   cscope: cannot find file arch/x86/efi/pe.c
>   cscope: cannot find file arch/x86/efi/boot.c
>   cscope: cannot find file arch/x86/efi/runtime.c
>
> This is caused by these being symlinks to common/efi.  Restrict all find runes
> to `-type f` to skip symlinks, because common/efi/*.c are already listed.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>

> ---
> CC: Anthony PERARD <anthony.perard@citrix.com>
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Wei Liu <wl@xen.org>
> CC: Stefano Stabellini <sstabellini@kernel.org>
> CC: Julien Grall <julien@xen.org>
> CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
> CC: Bertrand Marquis <bertrand.marquis@arm.com>
>
> Anthony: I looked through the remainder of your build series and I cant spot
> any edits to all_sources.  Apologies if I missed it.
> ---
>  xen/Makefile | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/xen/Makefile b/xen/Makefile
> index 2ad7da7ad67b..dc6bdc44c7a2 100644
> --- a/xen/Makefile
> +++ b/xen/Makefile
> @@ -468,9 +468,8 @@ arch/$(TARGET_ARCH)/include/asm/asm-offsets.h: asm-offsets.s
>  
>  SUBDIRS = xsm arch/$(TARGET_ARCH) common drivers lib test
>  define all_sources
> -    ( find arch/$(TARGET_ARCH)/include -name '*.h' -print; \
> -      find include -name '*.h' -print; \
> -      find $(SUBDIRS) -name '*.[chS]' -print )
> +    ( find include -type f -name '*.h' -print; \
> +      find $(SUBDIRS) -type f -name '*.[chS]' -print )

In my tooling I'm using -printf "\"%p\"\n" because generally there might
be files with funny names.

>  endef
>  
>  define set_exuberant_flags


-- 
Volodymyr Babchuk at EPAM