| Message ID | jpgh9eybjs8.fsf@linux.bootlegged.copy (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Apr 18, 2016 at 07:04:55PM -0400, Bandan Das wrote: > > The cscope.files that we generate doesn't include all > source files that are potentially interesting. We should > include all $(ARCH)es and not the just the one configure > finds. Moreover, $(ARCH) expands to x86_64 which is not the > correct path for x86 sources. Generate cscope.files by searching > for all files starting from root. No thanks :-) I'd rather not get hits for x86 and powerpc when jumping around in arm code. It's true that $(ARCH) expands to x86_64, but $(TEST_DIR) expands to x86, so the current find does find all x86 source. > > While we are there, remove the unnecessary sed substitution > and modify find to include a few other file name extensions. I can't remember why the sed was necessary, so it could maybe go away. Does it hurt anything though? Looks like you've added .cc, .sh, and .bash. I don't want to mix scripts with C source. As for api/*.{cc|hh}, we could change the API configure variable to a path ('./api') instead of a boolean, and then use it in the current cscope line, along with adding both .cc and .hh to the name extension list. (On a side note, we should probably create ./api its own makefile and look into getting it to work with other architectures at some point). Thanks, drew > > Signed-off-by: Bandan Das <bsd@redhat.com> > --- > Makefile | 9 +++------ > configure | 2 ++ > 2 files changed, 5 insertions(+), 6 deletions(-) > > diff --git a/Makefile b/Makefile > index 5d7506e..458d0f0 100644 > --- a/Makefile > +++ b/Makefile > @@ -92,11 +92,8 @@ distclean: clean libfdt_clean > $(RM) lib/asm config.mak $(TEST_DIR)-run test.log msr.out cscope.* > $(RM) -r tests > > -cscope: cscope_dirs = lib lib/libfdt lib/linux > -cscope: cscope_dirs += lib/$(ARCH)/asm lib/$(TEST_DIR)/asm lib/asm-generic > -cscope: cscope_dirs += $(TEST_DIR) lib/$(TEST_DIR) lib/$(ARCH) > cscope: > - $(RM) ./cscope.* > - find -L $(cscope_dirs) -maxdepth 1 \ > - -name '*.[chsS]' -print | sed 's,^\./,,' | sort -u > ./cscope.files > + $(RM) $(SRCDIR)/cscope.* > + find -L $(SRCDIR) -maxdepth 3 \ > + -regex '.*\.\(c\|h\|cc\|S\|sh\|bash\|s\)$$' -print | sort -u > ./cscope.files > cscope -bk > diff --git a/configure b/configure > index ba6c55b..3153fb9 100755 > --- a/configure > +++ b/configure > @@ -12,6 +12,7 @@ host=$arch > cross_prefix= > endian="" > pretty_print_stacks=yes > +srcdir=`pwd` > > usage() { > cat <<-EOF > @@ -159,4 +160,5 @@ TEST_DIR=$testdir > FIRMWARE=$firmware > ENDIAN=$endian > PRETTY_PRINT_STACKS=$pretty_print_stacks > +SRCDIR=$srcdir > EOF > -- > 2.5.5 > > -- > To unsubscribe from this list: send the line "unsubscribe kvm" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Andrew Jones <drjones@redhat.com> writes: > On Mon, Apr 18, 2016 at 07:04:55PM -0400, Bandan Das wrote: >> >> The cscope.files that we generate doesn't include all >> source files that are potentially interesting. We should >> include all $(ARCH)es and not the just the one configure >> finds. Moreover, $(ARCH) expands to x86_64 which is not the >> correct path for x86 sources. Generate cscope.files by searching >> for all files starting from root. > > No thanks :-) I'd rather not get hits for x86 and powerpc when I don't think that's a good idea. Just because you don't like to see x86 bits when jumping around in arm code doesn't mean you should hide all references to a given function. And if the arm code is segregated enough that there are no common functions, you won't see them anyway. > jumping around in arm code. It's true that $(ARCH) expands to > x86_64, but $(TEST_DIR) expands to x86, so the current find > does find all x86 source. Yep, you are right. It still prints a warning message about non-existent dirs though which needs to be fixed. >> >> While we are there, remove the unnecessary sed substitution >> and modify find to include a few other file name extensions. > > I can't remember why the sed was necessary, so it could maybe go > away. Does it hurt anything though? No, it doesn't hurt anything. Looks like it was meant to replace "./" with "". So, I really don't know what it was meant to do because that substitution doesn't seem right either. > Looks like you've added .cc, .sh, and .bash. I don't want to mix > scripts with C source. As for api/*.{cc|hh}, we could change the > API configure variable to a path ('./api') instead of a boolean, > and then use it in the current cscope line, along with adding > both .cc and .hh to the name extension list. (On a side note, we > should probably create ./api its own makefile and look into > getting it to work with other architectures at some point). Ok, this I agree with. We can separate the scripts from the c sources and probably have a variable that sets adding them. But please let's not make it difficult to write arm/powerpc tests just because someone's on x86 :) > Thanks, > drew > >> >> Signed-off-by: Bandan Das <bsd@redhat.com> >> --- >> Makefile | 9 +++------ >> configure | 2 ++ >> 2 files changed, 5 insertions(+), 6 deletions(-) >> >> diff --git a/Makefile b/Makefile >> index 5d7506e..458d0f0 100644 >> --- a/Makefile >> +++ b/Makefile >> @@ -92,11 +92,8 @@ distclean: clean libfdt_clean >> $(RM) lib/asm config.mak $(TEST_DIR)-run test.log msr.out cscope.* >> $(RM) -r tests >> >> -cscope: cscope_dirs = lib lib/libfdt lib/linux >> -cscope: cscope_dirs += lib/$(ARCH)/asm lib/$(TEST_DIR)/asm lib/asm-generic >> -cscope: cscope_dirs += $(TEST_DIR) lib/$(TEST_DIR) lib/$(ARCH) >> cscope: >> - $(RM) ./cscope.* >> - find -L $(cscope_dirs) -maxdepth 1 \ >> - -name '*.[chsS]' -print | sed 's,^\./,,' | sort -u > ./cscope.files >> + $(RM) $(SRCDIR)/cscope.* >> + find -L $(SRCDIR) -maxdepth 3 \ >> + -regex '.*\.\(c\|h\|cc\|S\|sh\|bash\|s\)$$' -print | sort -u > ./cscope.files >> cscope -bk >> diff --git a/configure b/configure >> index ba6c55b..3153fb9 100755 >> --- a/configure >> +++ b/configure >> @@ -12,6 +12,7 @@ host=$arch >> cross_prefix= >> endian="" >> pretty_print_stacks=yes >> +srcdir=`pwd` >> >> usage() { >> cat <<-EOF >> @@ -159,4 +160,5 @@ TEST_DIR=$testdir >> FIRMWARE=$firmware >> ENDIAN=$endian >> PRETTY_PRINT_STACKS=$pretty_print_stacks >> +SRCDIR=$srcdir >> EOF >> -- >> 2.5.5 >> >> -- >> To unsubscribe from this list: send the line "unsubscribe kvm" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html > -- > To unsubscribe from this list: send the line "unsubscribe kvm" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
2016-04-19 13:08-0400, Bandan Das: > Andrew Jones <drjones@redhat.com> writes: >> On Mon, Apr 18, 2016 at 07:04:55PM -0400, Bandan Das wrote: >>> >>> The cscope.files that we generate doesn't include all >>> source files that are potentially interesting. We should >>> include all $(ARCH)es and not the just the one configure >>> finds. Moreover, $(ARCH) expands to x86_64 which is not the >>> correct path for x86 sources. Generate cscope.files by searching >>> for all files starting from root. >> >> No thanks :-) I'd rather not get hits for x86 and powerpc when > > I don't think that's a good idea. Just because you don't like to > see x86 bits when jumping around in arm code doesn't mean you should > hide all references to a given function. And if the arm code is > segregated enough that there are no common functions, you won't > see them anyway. > [...] But please let's > not make it difficult to write arm/powerpc tests just because someone's on > x86 :) In the kernel, environment can override configuration, so you'd do % ARCH=arm make cscope and it doesn't matter what arch is being compiled. I think this would be a better solution for kvm-unit-tests too. I agree with Drew that other arches would mostly clutter searches if they were included by default, but we can also add a rule in case indexing all arches is desired (like when changing the prototype of some generic function?). -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Radim Kr?má? <rkrcmar@redhat.com> writes: > 2016-04-19 13:08-0400, Bandan Das: >> Andrew Jones <drjones@redhat.com> writes: >>> On Mon, Apr 18, 2016 at 07:04:55PM -0400, Bandan Das wrote: >>>> >>>> The cscope.files that we generate doesn't include all >>>> source files that are potentially interesting. We should >>>> include all $(ARCH)es and not the just the one configure >>>> finds. Moreover, $(ARCH) expands to x86_64 which is not the >>>> correct path for x86 sources. Generate cscope.files by searching >>>> for all files starting from root. >>> >>> No thanks :-) I'd rather not get hits for x86 and powerpc when >> >> I don't think that's a good idea. Just because you don't like to >> see x86 bits when jumping around in arm code doesn't mean you should >> hide all references to a given function. And if the arm code is >> segregated enough that there are no common functions, you won't >> see them anyway. >> [...] But please let's >> not make it difficult to write arm/powerpc tests just because someone's on >> x86 :) > > In the kernel, environment can override configuration, so you'd do > > % ARCH=arm make cscope > > and it doesn't matter what arch is being compiled. I think this would > be a better solution for kvm-unit-tests too. > > I agree with Drew that other arches would mostly clutter searches if > they were included by default, but we can also add a rule in case Since you mentioned the kernel, it looks like both the kernel/qemu include all by default which makes sense to me. As I mentioned above, you definitely don't want to modify a common library function only to realize that you forgot to modify all sites. Looking at all sites adds visibility, not introduce clutter. > indexing all arches is desired (like when changing the prototype of some > generic function?). kvm-unit-tests is small enough that this can be the default. That said, this is a trivial change and I am happy to live with it in my local branch. (Unless ofcourse both Linux and Qemu decide to make this default behavior too!) -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
2016-04-19 14:58-0400, Bandan Das: > Radim Kr?má? <rkrcmar@redhat.com> writes: >> 2016-04-19 13:08-0400, Bandan Das: >>> Andrew Jones <drjones@redhat.com> writes: >>>> On Mon, Apr 18, 2016 at 07:04:55PM -0400, Bandan Das wrote: >>>>> >>>>> The cscope.files that we generate doesn't include all >>>>> source files that are potentially interesting. We should >>>>> include all $(ARCH)es and not the just the one configure >>>>> finds. Moreover, $(ARCH) expands to x86_64 which is not the >>>>> correct path for x86 sources. Generate cscope.files by searching >>>>> for all files starting from root. >>>> >>>> No thanks :-) I'd rather not get hits for x86 and powerpc when >>> >>> I don't think that's a good idea. Just because you don't like to >>> see x86 bits when jumping around in arm code doesn't mean you should >>> hide all references to a given function. And if the arm code is >>> segregated enough that there are no common functions, you won't >>> see them anyway. >>> [...] But please let's >>> not make it difficult to write arm/powerpc tests just because someone's on >>> x86 :) >> >> In the kernel, environment can override configuration, so you'd do >> >> % ARCH=arm make cscope >> >> and it doesn't matter what arch is being compiled. I think this would >> be a better solution for kvm-unit-tests too. >> >> I agree with Drew that other arches would mostly clutter searches if >> they were included by default, but we can also add a rule in case > > Since you mentioned the kernel, it looks like both the kernel/qemu > include all by default which makes sense to me. qemu does, kernel doesn't (e.g. search for sys_sigreturn). > As I mentioned above, > you definitely don't want to modify a common library function only to > realize that you forgot to modify all sites. Looking at all sites adds > visibility, not introduce clutter. > >> indexing all arches is desired (like when changing the prototype of some >> generic function?). > > kvm-unit-tests is small enough that this can be the default. I don't see a reason to add results that we'd then have to mentally filter regardless of their amount. > That said, this > is a trivial change and I am happy to live with it in my local branch. (Unless > ofcourse both Linux and Qemu decide to make this default behavior too!) I think we'll agree on something that can be merged -- the present situation is bad. For me, "include all" is a matter of `cscope -bk **/*.[chS]` and getting only arch-specific cscope is harder, so having an option to generate cscope just for target arch would be nice. (I don't care about the default much, `ARCH=$sth make cscope` is easy to type if the default included all.) -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Radim Kr?má? <rkrcmar@redhat.com> writes: > 2016-04-19 14:58-0400, Bandan Das: >> Radim Kr?má? <rkrcmar@redhat.com> writes: >>> 2016-04-19 13:08-0400, Bandan Das: >>>> Andrew Jones <drjones@redhat.com> writes: >>>>> On Mon, Apr 18, 2016 at 07:04:55PM -0400, Bandan Das wrote: >>>>>> >>>>>> The cscope.files that we generate doesn't include all >>>>>> source files that are potentially interesting. We should >>>>>> include all $(ARCH)es and not the just the one configure >>>>>> finds. Moreover, $(ARCH) expands to x86_64 which is not the >>>>>> correct path for x86 sources. Generate cscope.files by searching >>>>>> for all files starting from root. >>>>> >>>>> No thanks :-) I'd rather not get hits for x86 and powerpc when >>>> >>>> I don't think that's a good idea. Just because you don't like to >>>> see x86 bits when jumping around in arm code doesn't mean you should >>>> hide all references to a given function. And if the arm code is >>>> segregated enough that there are no common functions, you won't >>>> see them anyway. >>>> [...] But please let's >>>> not make it difficult to write arm/powerpc tests just because someone's on >>>> x86 :) >>> >>> In the kernel, environment can override configuration, so you'd do >>> >>> % ARCH=arm make cscope >>> >>> and it doesn't matter what arch is being compiled. I think this would >>> be a better solution for kvm-unit-tests too. >>> >>> I agree with Drew that other arches would mostly clutter searches if >>> they were included by default, but we can also add a rule in case >> >> Since you mentioned the kernel, it looks like both the kernel/qemu >> include all by default which makes sense to me. > > qemu does, kernel doesn't (e.g. search for sys_sigreturn). Not exactly, arch specific(arch/*) files are excluded but there are others such as sound/* include/* that are not all x86 specific. The arch segregation is helpful since there would be functions with similar names (and similar functionality). That's not the case for every file in x86/ and arm/ of kvm-unit-tests. > >> As I mentioned above, >> you definitely don't want to modify a common library function only to >> realize that you forgot to modify all sites. Looking at all sites adds >> visibility, not introduce clutter. >> >>> indexing all arches is desired (like when changing the prototype of some >>> generic function?). >> >> kvm-unit-tests is small enough that this can be the default. > > I don't see a reason to add results that we'd then have to mentally > filter regardless of their amount. > >> That said, this >> is a trivial change and I am happy to live with it in my local branch. (Unless >> ofcourse both Linux and Qemu decide to make this default behavior too!) > > I think we'll agree on something that can be merged -- the present > situation is bad. It's not that bad, actually. We are simply arguing over personal preferences here. Changing my workflow a bit is not hard. :) > For me, "include all" is a matter of `cscope -bk **/*.[chS]` and getting > only arch-specific cscope is harder, so having an option to generate > cscope just for target arch would be nice. Yes, the option is definitely nice but making it the default, not so much. > (I don't care about the default much, `ARCH=$sth make cscope` is easy > to type if the default included all.) > -- > To unsubscribe from this list: send the line "unsubscribe kvm" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Apr 19, 2016 at 03:44:50PM -0400, Bandan Das wrote: > Radim Kr?má? <rkrcmar@redhat.com> writes: > > > 2016-04-19 14:58-0400, Bandan Das: > >> Radim Kr?má? <rkrcmar@redhat.com> writes: > >>> 2016-04-19 13:08-0400, Bandan Das: > >>>> Andrew Jones <drjones@redhat.com> writes: > >>>>> On Mon, Apr 18, 2016 at 07:04:55PM -0400, Bandan Das wrote: > >>>>>> > >>>>>> The cscope.files that we generate doesn't include all > >>>>>> source files that are potentially interesting. We should > >>>>>> include all $(ARCH)es and not the just the one configure > >>>>>> finds. Moreover, $(ARCH) expands to x86_64 which is not the > >>>>>> correct path for x86 sources. Generate cscope.files by searching > >>>>>> for all files starting from root. > >>>>> > >>>>> No thanks :-) I'd rather not get hits for x86 and powerpc when > >>>> > >>>> I don't think that's a good idea. Just because you don't like to > >>>> see x86 bits when jumping around in arm code doesn't mean you should > >>>> hide all references to a given function. And if the arm code is > >>>> segregated enough that there are no common functions, you won't > >>>> see them anyway. > >>>> [...] But please let's > >>>> not make it difficult to write arm/powerpc tests just because someone's on > >>>> x86 :) How does it make it more difficult to write arm/powerpc tests on x86? I do it all the time. Of course you need to run ./configure --arch=<ARCH> first, but you need to do that to compile anyway. > >>> > >>> In the kernel, environment can override configuration, so you'd do > >>> > >>> % ARCH=arm make cscope > >>> > >>> and it doesn't matter what arch is being compiled. I think this would > >>> be a better solution for kvm-unit-tests too. This is what kvm-unit-tests does, except it takes ARCH from config.mak, instead of making you specify it, and there is no 'everything' option, default or otherwise right now. > >>> > >>> I agree with Drew that other arches would mostly clutter searches if > >>> they were included by default, but we can also add a rule in case > >> > >> Since you mentioned the kernel, it looks like both the kernel/qemu > >> include all by default which makes sense to me. > > > > qemu does, kernel doesn't (e.g. search for sys_sigreturn). > > Not exactly, arch specific(arch/*) files are excluded but there are > others such as sound/* include/* that are not all x86 specific. The > arch segregation is helpful since there would be functions with similar > names (and similar functionality). That's not the case for every file > in x86/ and arm/ of kvm-unit-tests. kvm-unit-tests' cscope find collects all common files and all arch specific files, just as you've described about the kernel. Please give an example of where you couldn't find something you needed with this find, so that I know what the problem is. Also, if there is a problem, we can likely extend the find just a bit, rather than changing it to a wholesale search. > > > > >> As I mentioned above, > >> you definitely don't want to modify a common library function only to > >> realize that you forgot to modify all sites. Looking at all sites adds > >> visibility, not introduce clutter. Steps to modifying a common function's call sites 1) git grep <function> 2) compile test [at least] all architectures > >> > >>> indexing all arches is desired (like when changing the prototype of some > >>> generic function?). > >> > >> kvm-unit-tests is small enough that this can be the default. > > > > I don't see a reason to add results that we'd then have to mentally > > filter regardless of their amount. > > > >> That said, this > >> is a trivial change and I am happy to live with it in my local branch. (Unless > >> ofcourse both Linux and Qemu decide to make this default behavior too!) > > > > I think we'll agree on something that can be merged -- the present > > situation is bad. > > It's not that bad, actually. We are simply arguing over personal > preferences here. Changing my workflow a bit is not hard. :) > > > For me, "include all" is a matter of `cscope -bk **/*.[chS]` and getting > > only arch-specific cscope is harder, so having an option to generate > > cscope just for target arch would be nice. Yes, generating cscope just for the target arch is nice, and that's kvm-unit-tests default, as it makes the most sense for the common unit test developer, i.e. somebody not extending the framework, but rather just writing an arch-specific unit test. > > Yes, the option is definitely nice but making it the default, not so much. As should be clear by now, IMHO, the default is not only nice, but correct. Feel free to add a cscope-all target though. > > > (I don't care about the default much, `ARCH=$sth make cscope` is easy > > to type if the default included all.) I can live with that too, but I do care about defaults, and defaults should target the common cases. I haven't yet seen enough justification as to why seeing everything (whether you want to or not) is the best common case for kvm-unit-tests' cscope generation. Thanks, drew -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Andrew Jones <drjones@redhat.com> writes: > On Tue, Apr 19, 2016 at 03:44:50PM -0400, Bandan Das wrote: >> Radim Kr?má? <rkrcmar@redhat.com> writes: >> >> > 2016-04-19 14:58-0400, Bandan Das: >> >> Radim Kr?má? <rkrcmar@redhat.com> writes: >> >>> 2016-04-19 13:08-0400, Bandan Das: >> >>>> Andrew Jones <drjones@redhat.com> writes: >> >>>>> On Mon, Apr 18, 2016 at 07:04:55PM -0400, Bandan Das wrote: >> >>>>>> >> >>>>>> The cscope.files that we generate doesn't include all >> >>>>>> source files that are potentially interesting. We should >> >>>>>> include all $(ARCH)es and not the just the one configure >> >>>>>> finds. Moreover, $(ARCH) expands to x86_64 which is not the >> >>>>>> correct path for x86 sources. Generate cscope.files by searching >> >>>>>> for all files starting from root. >> >>>>> >> >>>>> No thanks :-) I'd rather not get hits for x86 and powerpc when >> >>>> >> >>>> I don't think that's a good idea. Just because you don't like to >> >>>> see x86 bits when jumping around in arm code doesn't mean you should >> >>>> hide all references to a given function. And if the arm code is >> >>>> segregated enough that there are no common functions, you won't >> >>>> see them anyway. >> >>>> [...] But please let's >> >>>> not make it difficult to write arm/powerpc tests just because someone's on >> >>>> x86 :) > > How does it make it more difficult to write arm/powerpc tests on x86? > I do it all the time. Of course you need to run ./configure --arch=<ARCH> > first, but you need to do that to compile anyway. > >> >>> >> >>> In the kernel, environment can override configuration, so you'd do >> >>> >> >>> % ARCH=arm make cscope >> >>> >> >>> and it doesn't matter what arch is being compiled. I think this would >> >>> be a better solution for kvm-unit-tests too. > > This is what kvm-unit-tests does, except it takes ARCH from config.mak, > instead of making you specify it, and there is no 'everything' option, > default or otherwise right now. > >> >>> >> >>> I agree with Drew that other arches would mostly clutter searches if >> >>> they were included by default, but we can also add a rule in case >> >> >> >> Since you mentioned the kernel, it looks like both the kernel/qemu >> >> include all by default which makes sense to me. >> > >> > qemu does, kernel doesn't (e.g. search for sys_sigreturn). >> >> Not exactly, arch specific(arch/*) files are excluded but there are >> others such as sound/* include/* that are not all x86 specific. The >> arch segregation is helpful since there would be functions with similar >> names (and similar functionality). That's not the case for every file >> in x86/ and arm/ of kvm-unit-tests. > > kvm-unit-tests' cscope find collects all common files and all arch > specific files, just as you've described about the kernel. Please give > an example of where you couldn't find something you needed with this > find, so that I know what the problem is. Also, if there is a problem, Sorry, you are right. Looks like I was looking at stale files (iotable.c/h) which don't exist anymore. So, I think the current setup works for me too since I mostly work on x86. For other times, when I do have to certain things in other arches, I will just run cscope by hand. > we can likely extend the find just a bit, rather than changing it to a > wholesale search. > >> ... >> >> Yes, the option is definitely nice but making it the default, not so much. > > As should be clear by now, IMHO, the default is not only nice, but correct. > Feel free to add a cscope-all target though. Yeah, I can live with this too. IMHO, the default is a matter of preference, there's no question of correctness. >> >> > (I don't care about the default much, `ARCH=$sth make cscope` is easy >> > to type if the default included all.) > > I can live with that too, but I do care about defaults, and defaults > should target the common cases. I haven't yet seen enough justification > as to why seeing everything (whether you want to or not) is the best > common case for kvm-unit-tests' cscope generation. Me neither, I am not convinced yet how including all files obstructs your workflow but I am certain living with the defaults won't obstruct mine. > Thanks, > drew -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Apr 20, 2016 at 10:31:24AM -0400, Bandan Das wrote: > Andrew Jones <drjones@redhat.com> writes: > > > On Tue, Apr 19, 2016 at 03:44:50PM -0400, Bandan Das wrote: > >> Radim Kr?má? <rkrcmar@redhat.com> writes: > >> > >> > 2016-04-19 14:58-0400, Bandan Das: > >> >> Radim Kr?má? <rkrcmar@redhat.com> writes: > >> >>> 2016-04-19 13:08-0400, Bandan Das: > >> >>>> Andrew Jones <drjones@redhat.com> writes: > >> >>>>> On Mon, Apr 18, 2016 at 07:04:55PM -0400, Bandan Das wrote: > >> >>>>>> > >> >>>>>> The cscope.files that we generate doesn't include all > >> >>>>>> source files that are potentially interesting. We should > >> >>>>>> include all $(ARCH)es and not the just the one configure > >> >>>>>> finds. Moreover, $(ARCH) expands to x86_64 which is not the > >> >>>>>> correct path for x86 sources. Generate cscope.files by searching > >> >>>>>> for all files starting from root. > >> >>>>> > >> >>>>> No thanks :-) I'd rather not get hits for x86 and powerpc when > >> >>>> > >> >>>> I don't think that's a good idea. Just because you don't like to > >> >>>> see x86 bits when jumping around in arm code doesn't mean you should > >> >>>> hide all references to a given function. And if the arm code is > >> >>>> segregated enough that there are no common functions, you won't > >> >>>> see them anyway. > >> >>>> [...] But please let's > >> >>>> not make it difficult to write arm/powerpc tests just because someone's on > >> >>>> x86 :) > > > > How does it make it more difficult to write arm/powerpc tests on x86? > > I do it all the time. Of course you need to run ./configure --arch=<ARCH> > > first, but you need to do that to compile anyway. > > > >> >>> > >> >>> In the kernel, environment can override configuration, so you'd do > >> >>> > >> >>> % ARCH=arm make cscope > >> >>> > >> >>> and it doesn't matter what arch is being compiled. I think this would > >> >>> be a better solution for kvm-unit-tests too. > > > > This is what kvm-unit-tests does, except it takes ARCH from config.mak, > > instead of making you specify it, and there is no 'everything' option, > > default or otherwise right now. > > > >> >>> > >> >>> I agree with Drew that other arches would mostly clutter searches if > >> >>> they were included by default, but we can also add a rule in case > >> >> > >> >> Since you mentioned the kernel, it looks like both the kernel/qemu > >> >> include all by default which makes sense to me. > >> > > >> > qemu does, kernel doesn't (e.g. search for sys_sigreturn). > >> > >> Not exactly, arch specific(arch/*) files are excluded but there are > >> others such as sound/* include/* that are not all x86 specific. The > >> arch segregation is helpful since there would be functions with similar > >> names (and similar functionality). That's not the case for every file > >> in x86/ and arm/ of kvm-unit-tests. > > > > kvm-unit-tests' cscope find collects all common files and all arch > > specific files, just as you've described about the kernel. Please give > > an example of where you couldn't find something you needed with this > > find, so that I know what the problem is. Also, if there is a problem, > > Sorry, you are right. Looks like I was looking at stale files (iotable.c/h) > which don't exist anymore. So, I think the current setup works for me > too since I mostly work on x86. For other times, when I do have to certain > things in other arches, I will just run cscope by hand. Why run it by hand? Configure the arch you want to focus on, and then do 'make cscope'. Or, if you really need to view all, then submit a patch adding a new makefile target, and then run that. > > > we can likely extend the find just a bit, rather than changing it to a > > wholesale search. > > > >> > ... > >> > >> Yes, the option is definitely nice but making it the default, not so much. > > > > As should be clear by now, IMHO, the default is not only nice, but correct. > > Feel free to add a cscope-all target though. > > Yeah, I can live with this too. IMHO, the default is a matter of preference, there's > no question of correctness. Selection of defaults is hard. Somebody always feels it wrong. However if you look at what works best for the largest audience, then I believe that selection is the _correct_ default. I could be wrong about this one, but I don't think so ;-) > > >> > >> > (I don't care about the default much, `ARCH=$sth make cscope` is easy > >> > to type if the default included all.) > > > > I can live with that too, but I do care about defaults, and defaults > > should target the common cases. I haven't yet seen enough justification > > as to why seeing everything (whether you want to or not) is the best > > common case for kvm-unit-tests' cscope generation. > > Me neither, I am not convinced yet how including all files obstructs your workflow > but I am certain living with the defaults won't obstruct mine. It doesn't obstruct it. It adds useless overhead. More mental filtering, as Radim described it. drew -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Yeah, kvm-unit-tests flamewars! Testing has really taken off. Paolo On 19/04/2016 01:04, Bandan Das wrote: > > The cscope.files that we generate doesn't include all > source files that are potentially interesting. We should > include all $(ARCH)es and not the just the one configure > finds. Moreover, $(ARCH) expands to x86_64 which is not the > correct path for x86 sources. Generate cscope.files by searching > for all files starting from root. > > While we are there, remove the unnecessary sed substitution > and modify find to include a few other file name extensions. > > Signed-off-by: Bandan Das <bsd@redhat.com> > --- > Makefile | 9 +++------ > configure | 2 ++ > 2 files changed, 5 insertions(+), 6 deletions(-) > > diff --git a/Makefile b/Makefile > index 5d7506e..458d0f0 100644 > --- a/Makefile > +++ b/Makefile > @@ -92,11 +92,8 @@ distclean: clean libfdt_clean > $(RM) lib/asm config.mak $(TEST_DIR)-run test.log msr.out cscope.* > $(RM) -r tests > > -cscope: cscope_dirs = lib lib/libfdt lib/linux > -cscope: cscope_dirs += lib/$(ARCH)/asm lib/$(TEST_DIR)/asm lib/asm-generic > -cscope: cscope_dirs += $(TEST_DIR) lib/$(TEST_DIR) lib/$(ARCH) > cscope: > - $(RM) ./cscope.* > - find -L $(cscope_dirs) -maxdepth 1 \ > - -name '*.[chsS]' -print | sed 's,^\./,,' | sort -u > ./cscope.files > + $(RM) $(SRCDIR)/cscope.* > + find -L $(SRCDIR) -maxdepth 3 \ > + -regex '.*\.\(c\|h\|cc\|S\|sh\|bash\|s\)$$' -print | sort -u > ./cscope.files > cscope -bk > diff --git a/configure b/configure > index ba6c55b..3153fb9 100755 > --- a/configure > +++ b/configure > @@ -12,6 +12,7 @@ host=$arch > cross_prefix= > endian="" > pretty_print_stacks=yes > +srcdir=`pwd` > > usage() { > cat <<-EOF > @@ -159,4 +160,5 @@ TEST_DIR=$testdir > FIRMWARE=$firmware > ENDIAN=$endian > PRETTY_PRINT_STACKS=$pretty_print_stacks > +SRCDIR=$srcdir > EOF > -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/Makefile b/Makefile index 5d7506e..458d0f0 100644 --- a/Makefile +++ b/Makefile @@ -92,11 +92,8 @@ distclean: clean libfdt_clean $(RM) lib/asm config.mak $(TEST_DIR)-run test.log msr.out cscope.* $(RM) -r tests -cscope: cscope_dirs = lib lib/libfdt lib/linux -cscope: cscope_dirs += lib/$(ARCH)/asm lib/$(TEST_DIR)/asm lib/asm-generic -cscope: cscope_dirs += $(TEST_DIR) lib/$(TEST_DIR) lib/$(ARCH) cscope: - $(RM) ./cscope.* - find -L $(cscope_dirs) -maxdepth 1 \ - -name '*.[chsS]' -print | sed 's,^\./,,' | sort -u > ./cscope.files + $(RM) $(SRCDIR)/cscope.* + find -L $(SRCDIR) -maxdepth 3 \ + -regex '.*\.\(c\|h\|cc\|S\|sh\|bash\|s\)$$' -print | sort -u > ./cscope.files cscope -bk diff --git a/configure b/configure index ba6c55b..3153fb9 100755 --- a/configure +++ b/configure @@ -12,6 +12,7 @@ host=$arch cross_prefix= endian="" pretty_print_stacks=yes +srcdir=`pwd` usage() { cat <<-EOF @@ -159,4 +160,5 @@ TEST_DIR=$testdir FIRMWARE=$firmware ENDIAN=$endian PRETTY_PRINT_STACKS=$pretty_print_stacks +SRCDIR=$srcdir EOF
The cscope.files that we generate doesn't include all source files that are potentially interesting. We should include all $(ARCH)es and not the just the one configure finds. Moreover, $(ARCH) expands to x86_64 which is not the correct path for x86 sources. Generate cscope.files by searching for all files starting from root. While we are there, remove the unnecessary sed substitution and modify find to include a few other file name extensions. Signed-off-by: Bandan Das <bsd@redhat.com> --- Makefile | 9 +++------ configure | 2 ++ 2 files changed, 5 insertions(+), 6 deletions(-)