| Message ID | 20191020122452.3345-1-prabhakar.pkin@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | kselftest: Fix NULL INSTALL_PATH for TARGETS runlist | expand |
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); \ >
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); \ > > >
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); \ >>> >>
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); \
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(-)