| Message ID | 20211216092014.707-1-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xen/build: Fix `make cscope` rune | expand |
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
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,
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
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
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
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
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
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(-)