Re: [PATCH i-g-t v2 3/5] runner/settings: Add function to set IGT_RUNNER_DATA environment variable

["Naladala, Ramanaidu" <Ramanaidu.naladala@intel.com>]

[-- Attachment #1: Type: text/plain, Size: 3635 bytes --]


On 1/16/2025 6:06 PM, Kamil Konieczny wrote:
> Hi Naladala,
> On 2025-01-08 at 01:36:12 +0530, Naladala Ramanaidu wrote:
>> Introduced set_runner_datadir() to set the IGT_RUNNER_DATA environment
>> variable. This function uses absolute_path() to convert the relative
>> path "../share/igt-gpu-tools" to an absolute path.
>> Update the
>> runner/runner code to call set_runner_datadir() and handle errors
>> appropriately.
> Please do not translate code into words, is it there in a patch itself.
> imho here the most important part is description why we need it and
> what problem you try to resolve.
Sure Kamil. Thanks for the review. I will update the commit message.
>
>> v2: Fix review comments. (Kamil)
>>
>> Signed-off-by: Naladala Ramanaidu<ramanaidu.naladala@intel.com>
>> ---
>>   runner/runner.c   | 8 ++++++++
>>   runner/settings.c | 9 +++++++++
>>   runner/settings.h | 1 +
>>   3 files changed, 18 insertions(+)
>>
>> diff --git a/runner/runner.c b/runner/runner.c
>> index 4855ad641..950eb5662 100644
>> --- a/runner/runner.c
>> +++ b/runner/runner.c
>> @@ -12,8 +12,13 @@ int main(int argc, char **argv)
>>   	struct job_list job_list;
>>   	struct execute_state state;
>>   	int exitcode = 0;
>> +	int data_flag = 0;
>>   
>>   	init_settings(&settings);
>> +	data_flag = set_runner_datadir();
>> +	if (data_flag)
>> +		fprintf(stderr, "Data dir path not set\n");
> Why an error? You could check if './data' path is present
> but this imho should be done in init_settings(), not here.

Sure. I will move this to init_setting(). Error message is required to 
understand the test failure.

env variable is not permanent it will be cleared before stop igt_runner 
execution.

>
>> +
>>   	init_job_list(&job_list);
>>   
>>   	if (!parse_options(argc, argv, &settings)) {
>> @@ -49,6 +54,9 @@ int main(int argc, char **argv)
>>   		exitcode = 1;
>>   	}
>>   
>> +	if (!data_flag)
>> +		unsetenv("IGT_RUNNER_DATA");
>> +
>>   	printf("Done.\n");
>>   	return exitcode;
>>   }
>> diff --git a/runner/settings.c b/runner/settings.c
>> index bea0c3059..c8220be32 100644
>> --- a/runner/settings.c
>> +++ b/runner/settings.c
>> @@ -589,6 +589,15 @@ char *absolute_path(const char *path)
>>   	return result;
>>   }
>>   
>> +int set_runner_datadir(void)
> The only user of this function is in runner.c
> so why do you need it here?
>
>> +{
>> +	const char *datapath = "../share/igt-gpu-tools";
>> +	char *abpath;
>> +
>> +	abpath = absolute_path(datapath);
>> +	return setenv("IGT_RUNNER_DATA", abpath, 1);
> Why are you setting this var here? It could be set by a user
> so why are you overwriting it in that case?
>
> imho you do not need this function but maybe I am missing something?
>
> Regards,
> Kamil
Hi Kamil,

This env variable is not set by the user. In previously, image files 
path is hard coded

and when we ran test from different locations causing test fail. This 
env variable is

set by the runner and useful to get the absolute path of the image 
directory on

runtime. This env variable will set by igt_runner and cleared env 
variable before

runner complete the test execution.

>
>> +}
>> +
>>   static char *bin_path(char *fname)
>>   {
>>   	char *path, *p;
>> diff --git a/runner/settings.h b/runner/settings.h
>> index 7e6cd11e2..5926817b6 100644
>> --- a/runner/settings.h
>> +++ b/runner/settings.h
>> @@ -150,5 +150,6 @@ bool serialize_settings(struct settings *settings);
>>   
>>   bool read_settings_from_file(struct settings *settings, FILE* f);
>>   bool read_settings_from_dir(struct settings *settings, int dirfd);
>> +int set_runner_datadir(void);
>>   
>>   #endif
>> -- 
>> 2.43.0
>>

[-- Attachment #2: Type: text/html, Size: 5425 bytes --]