Re: [PATCH 2/9] selftests: Add install target

[Shuah Khan <shuahkh@osg.samsung.com>]

On 03/10/2015 09:20 PM, Michael Ellerman wrote:
> On Tue, 2015-03-10 at 09:11 -0600, Shuah Khan wrote:
>> On 03/09/2015 04:29 PM, Shuah Khan wrote:
>>> On 03/09/2015 08:20 AM, Shuah Khan wrote:
>>>> On 03/05/2015 11:53 AM, Dave Jones wrote:
>>>>> On Tue, Mar 03, 2015 at 03:51:35PM +1100, Michael Ellerman wrote:
>>>>>  > This adds make install support to selftests. The basic usage is:
>>>>>  > 
>>>>>  > $ cd tools/testing/selftests
>>>>>  > $ make install
>>>>>  > 
>>>>>  > That installs into tools/testing/selftests/install, which can then be
>>>>>  > copied where ever necessary.
>>>>>  > 
>>>>>  > The install destination is also configurable using eg:
>>>>>  > 
>>>>>  > $ INSTALL_PATH=/mnt/selftests make install
>>>>>
>>>>>  ...
>>>>>
>>>>>  > +	@# Ask all targets to emit their test scripts
>>>>>  > +	echo "#!/bin/bash\n\n" > $(ALL_SCRIPT)
>>>>>
>>>>> $ ./all.sh 
>>>>> -bash: ./all.sh: /bin/bash\n\n: bad interpreter: No such file or directory
>>>>>
>>>>> Removing the \n\n fixes it.
>>>>>
>>>>>  > +		echo "cd \$$ROOT\n" >> $(ALL_SCRIPT); \
>>>>>
>>>>> ditto
>>>>>
>>>>> 	Dave
>>>>
>>>> Michael,
>>>>
>>>> Could you please fix these problems and send the patch.
>>>>
>>>
>>> Michael,
>>>
>>> Did you happen to run run_kselftest.sh from the install
>>> directory to make sure all the dependent executables
>>> are installed? You are missing a few required dependencies.
>>> efivars test for example.
>>>
>>> Please run kselftest_install.sh I sent out for review and
>>> compare the following:
>>>
>>> - contents of install directory created with your patch vs.
>>>   my kselftest_install.sh tool
>>> - Compare your run_kselftest.sh run with the one that gets
>>>   generated with my kselftest_install.sh tool
>>>
>>> General rule is all tests that get run when run_tests target
>>> is run should run from the install directory using the
>>> run_kselftest.sh generated during install.
>>>
>>
>> Couple more things. Please change the install directory name
>> to kselftest
>>
>> tools/testing/selftests/kselftest
>> instead of
>> tools/testing/selftests/install
> 
> I prefer install, that's what it is after all. I don't know why you're so
> obsessed with the "kselftest" name.

It is the overall user-interface. I want to be able to call
install wrapper script and have the right directory generated.
If user passes in /tmp for example as a main directory, install
is so generic and doesn't make sense. I see that you sent the
patch v4 still with install and please change it to kselftest
> 
>> Also please flatten the directory structure under the install
>> directory. I don't see any value in creating directory for each
>> test for install. Also it is makes it cumbersome for users
>> to navigate and work with after the install. This would mean cpu
>> and memory hot-plug scripts need unique names.
> 
> That's not a good idea. To start with different tests might the same name, as
> already happens with the memory & cpu hot-plug tests. They may also have data
> files that would clobber each other. We'd need to make sure all files used in
> all tests have different names, that would be a total mess.

No I want the just one container directory for the tests. Again I am
looking at the bigger picture user-interface angle. Please flatten it,
and resend it.

I responded to your patch v4. Please re-do the patch to address my
comments if you want your patch to be included.

thanks,
-- Shuah


-- 
Shuah Khan
Sr. Linux Kernel Developer
Open Source Innovation Group
Samsung Research America (Silicon Valley)
shuahkh@osg.samsung.com | (970) 217-8978