[PATCH] kselftest: Fix NULL INSTALL_PATH for TARGETS runlist

[PATCH] kselftest: Fix NULL INSTALL_PATH for TARGETS runlist

[Prabhakar Kushwaha]

As per commit 131b30c94fbc ("kselftest: exclude failed TARGETS from
runlist") failed targets were excluded from the runlist. But value
$$INSTALL_PATH is always NULL. It should be $INSTALL_PATH instead
$$INSTALL_PATH.

So, fix Makefile to use $INSTALL_PATH. 

Signed-off-by: Prabhakar Kushwaha <pkushwaha@marvell.com>
Signed-off-by: Prabhakar Kushwaha <prabhakar.pkin@gmail.com>
CC: Cristian Marussi <cristian.marussi@arm.com>
---
 tools/testing/selftests/Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/testing/selftests/Makefile b/tools/testing/selftests/Makefile
index 4cdbae6f4e61..612f6757015d 100644
--- a/tools/testing/selftests/Makefile
+++ b/tools/testing/selftests/Makefile
@@ -213,7 +213,7 @@ ifdef INSTALL_PATH
 	@# included in the generated runlist.
 	for TARGET in $(TARGETS); do \
 		BUILD_TARGET=$$BUILD/$$TARGET;	\
-		[ ! -d $$INSTALL_PATH/$$TARGET ] && echo "Skipping non-existent dir: $$TARGET" && continue; \
+		[ ! -d $(INSTALL_PATH)/$$TARGET ] && echo "Skipping non-existent dir: $$TARGET" && continue; \
 		echo "[ -w /dev/kmsg ] && echo \"kselftest: Running tests in $$TARGET\" >> /dev/kmsg" >> $(ALL_SCRIPT); \
 		echo "cd $$TARGET" >> $(ALL_SCRIPT); \
 		echo -n "run_many" >> $(ALL_SCRIPT); \
-- 
2.17.0

Re: [PATCH] kselftest: Fix NULL INSTALL_PATH for TARGETS runlist

[Cristian Marussi]

Hi

On 20/10/2019 13:24, Prabhakar Kushwaha wrote:
> As per commit 131b30c94fbc ("kselftest: exclude failed TARGETS from
> runlist") failed targets were excluded from the runlist. But value
> $$INSTALL_PATH is always NULL. It should be $INSTALL_PATH instead
> $$INSTALL_PATH.
> 
> So, fix Makefile to use $INSTALL_PATH. 
> 

I was a bit puzzled at first since I never saw the NULLified value while testing
the original patch. Looking at it closely today, I realized that I used to test it
like:

$ rm -rf $HOME/KSFT_TEST && make -C tools/testing/selftests/ INSTALL_PATH=$HOME/KSFT_TEST install

which in fact causes INSTALL_PATH to be exported down to the subshell in the recipe, so that even
referring it as $$INSTALL_PATH from the recipe line make it work fine.

Instead, using the default Makefile provided value (unexported) by invoking like:

$ rm -rf $HOME/KSFT_TEST && make -C tools/testing/selftests/ install

exposes the error you mentioned, being INSTALL_PATH not accessible form the subshell and so NULL.
Moreover it's anyway certainly better to refer with $(INSTALL_PATH) being it a strict Makefile var.
So it's fine for me, thanks to have spotted this.

Reviewed-by: cristian.marussi@arm.com


Cristian


> Signed-off-by: Prabhakar Kushwaha <pkushwaha@marvell.com>
> Signed-off-by: Prabhakar Kushwaha <prabhakar.pkin@gmail.com>
> CC: Cristian Marussi <cristian.marussi@arm.com>
> ---
>  tools/testing/selftests/Makefile | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/testing/selftests/Makefile b/tools/testing/selftests/Makefile
> index 4cdbae6f4e61..612f6757015d 100644
> --- a/tools/testing/selftests/Makefile
> +++ b/tools/testing/selftests/Makefile
> @@ -213,7 +213,7 @@ ifdef INSTALL_PATH
>  	@# included in the generated runlist.
>  	for TARGET in $(TARGETS); do \
>  		BUILD_TARGET=$$BUILD/$$TARGET;	\
> -		[ ! -d $$INSTALL_PATH/$$TARGET ] && echo "Skipping non-existent dir: $$TARGET" && continue; \
> +		[ ! -d $(INSTALL_PATH)/$$TARGET ] && echo "Skipping non-existent dir: $$TARGET" && continue; \
>  		echo "[ -w /dev/kmsg ] && echo \"kselftest: Running tests in $$TARGET\" >> /dev/kmsg" >> $(ALL_SCRIPT); \
>  		echo "cd $$TARGET" >> $(ALL_SCRIPT); \
>  		echo -n "run_many" >> $(ALL_SCRIPT); \
> 

Re: [PATCH] kselftest: Fix NULL INSTALL_PATH for TARGETS runlist

[Prabhakar Kushwaha]

Dear Cristian,

On Mon, Oct 21, 2019 at 4:15 PM Cristian Marussi
<cristian.marussi@arm.com> wrote:
>
> Hi
>
> On 20/10/2019 13:24, Prabhakar Kushwaha wrote:
> > As per commit 131b30c94fbc ("kselftest: exclude failed TARGETS from
> > runlist") failed targets were excluded from the runlist. But value
> > $$INSTALL_PATH is always NULL. It should be $INSTALL_PATH instead
> > $$INSTALL_PATH.
> >
> > So, fix Makefile to use $INSTALL_PATH.
> >
>
> I was a bit puzzled at first since I never saw the NULLified value while testing
> the original patch. Looking at it closely today, I realized that I used to test it
> like:
>
> $ rm -rf $HOME/KSFT_TEST && make -C tools/testing/selftests/ INSTALL_PATH=$HOME/KSFT_TEST install
>
> which in fact causes INSTALL_PATH to be exported down to the subshell in the recipe, so that even
> referring it as $$INSTALL_PATH from the recipe line make it work fine.
>
> Instead, using the default Makefile provided value (unexported) by invoking like:
>
> $ rm -rf $HOME/KSFT_TEST && make -C tools/testing/selftests/ install
>
> exposes the error you mentioned, being INSTALL_PATH not accessible form the subshell and so NULL.
> Moreover it's anyway certainly better to refer with $(INSTALL_PATH) being it a strict Makefile var.
> So it's fine for me, thanks to have spotted this.
>
> Reviewed-by: cristian.marussi@arm.com
>

Thanks for Reviewing.

I have to send v2 patch with author mail id fix. I will keep your Reviewed-by.

--prabhakar (pk)


>
> > Signed-off-by: Prabhakar Kushwaha <pkushwaha@marvell.com>
> > Signed-off-by: Prabhakar Kushwaha <prabhakar.pkin@gmail.com>
> > CC: Cristian Marussi <cristian.marussi@arm.com>
> > ---
> >  tools/testing/selftests/Makefile | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/tools/testing/selftests/Makefile b/tools/testing/selftests/Makefile
> > index 4cdbae6f4e61..612f6757015d 100644
> > --- a/tools/testing/selftests/Makefile
> > +++ b/tools/testing/selftests/Makefile
> > @@ -213,7 +213,7 @@ ifdef INSTALL_PATH
> >       @# included in the generated runlist.
> >       for TARGET in $(TARGETS); do \
> >               BUILD_TARGET=$$BUILD/$$TARGET;  \
> > -             [ ! -d $$INSTALL_PATH/$$TARGET ] && echo "Skipping non-existent dir: $$TARGET" && continue; \
> > +             [ ! -d $(INSTALL_PATH)/$$TARGET ] && echo "Skipping non-existent dir: $$TARGET" && continue; \
> >               echo "[ -w /dev/kmsg ] && echo \"kselftest: Running tests in $$TARGET\" >> /dev/kmsg" >> $(ALL_SCRIPT); \
> >               echo "cd $$TARGET" >> $(ALL_SCRIPT); \
> >               echo -n "run_many" >> $(ALL_SCRIPT); \
> >
>

Re: [PATCH] kselftest: Fix NULL INSTALL_PATH for TARGETS runlist

[Cristian Marussi]

Hi

On 22/10/2019 04:52, Prabhakar Kushwaha wrote:
> Dear Cristian,
> 
> On Mon, Oct 21, 2019 at 4:15 PM Cristian Marussi
> <cristian.marussi@arm.com> wrote:
>>
>> Hi
>>
>> On 20/10/2019 13:24, Prabhakar Kushwaha wrote:
>>> As per commit 131b30c94fbc ("kselftest: exclude failed TARGETS from
>>> runlist") failed targets were excluded from the runlist. But value
>>> $$INSTALL_PATH is always NULL. It should be $INSTALL_PATH instead
>>> $$INSTALL_PATH.
>>>
>>> So, fix Makefile to use $INSTALL_PATH.
>>>
>>
>> I was a bit puzzled at first since I never saw the NULLified value while testing
>> the original patch. Looking at it closely today, I realized that I used to test it
>> like:
>>
>> $ rm -rf $HOME/KSFT_TEST && make -C tools/testing/selftests/ INSTALL_PATH=$HOME/KSFT_TEST install
>>
>> which in fact causes INSTALL_PATH to be exported down to the subshell in the recipe, so that even
>> referring it as $$INSTALL_PATH from the recipe line make it work fine.
>>
>> Instead, using the default Makefile provided value (unexported) by invoking like:
>>
>> $ rm -rf $HOME/KSFT_TEST && make -C tools/testing/selftests/ install
>>
>> exposes the error you mentioned, being INSTALL_PATH not accessible form the subshell and so NULL.
>> Moreover it's anyway certainly better to refer with $(INSTALL_PATH) being it a strict Makefile var.
>> So it's fine for me, thanks to have spotted this.
>>
>> Reviewed-by: cristian.marussi@arm.com
>>
> 
> Thanks for Reviewing.
> 
> I have to send v2 patch with author mail id fix. I will keep your Reviewed-by.

Thanks to you.

I forgot to say that maybe it's worth also adding a Fixes: tag too like:

Fixes: 131b30c94fbc ("kselftest: exclude failed TARGETS from runlist")

given that I've spotted the original patch being already picked up for
some stable queues like in:

https://lore.kernel.org/lkml/20191018220324.8165-22-sashal@kernel.org/

Thanks

Cristian

> 
> --prabhakar (pk)
> 
> 
>>
>>> Signed-off-by: Prabhakar Kushwaha <pkushwaha@marvell.com>
>>> Signed-off-by: Prabhakar Kushwaha <prabhakar.pkin@gmail.com>
>>> CC: Cristian Marussi <cristian.marussi@arm.com>
>>> ---
>>>  tools/testing/selftests/Makefile | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/tools/testing/selftests/Makefile b/tools/testing/selftests/Makefile
>>> index 4cdbae6f4e61..612f6757015d 100644
>>> --- a/tools/testing/selftests/Makefile
>>> +++ b/tools/testing/selftests/Makefile
>>> @@ -213,7 +213,7 @@ ifdef INSTALL_PATH
>>>       @# included in the generated runlist.
>>>       for TARGET in $(TARGETS); do \
>>>               BUILD_TARGET=$$BUILD/$$TARGET;  \
>>> -             [ ! -d $$INSTALL_PATH/$$TARGET ] && echo "Skipping non-existent dir: $$TARGET" && continue; \
>>> +             [ ! -d $(INSTALL_PATH)/$$TARGET ] && echo "Skipping non-existent dir: $$TARGET" && continue; \
>>>               echo "[ -w /dev/kmsg ] && echo \"kselftest: Running tests in $$TARGET\" >> /dev/kmsg" >> $(ALL_SCRIPT); \
>>>               echo "cd $$TARGET" >> $(ALL_SCRIPT); \
>>>               echo -n "run_many" >> $(ALL_SCRIPT); \
>>>
>>