[PATCH i-g-t v4 3/5] runner/settings: Add function to set IGT_DATA_PATH environment variable

[Naladala, Ramanaidu]

Hi Kamil,

On 4/4/2025 8:51 PM, Kamil Konieczny wrote:
> Hi Naladala,
> On 2025-03-26 at 02:31:44 +0530, Naladala Ramanaidu wrote:
>> Add environment variable to obtain absolute path for PNG files
>> at runtime.
>>
>> v2: Fix review comments. (Kamil)
>> v3: Fix review comments. (kamil)
>> V4: Fix review comments. (kamil)
>>
>> Signed-off-by: Naladala Ramanaidu<ramanaidu.naladala at intel.com>
>> ---
>>   runner/settings.c | 22 ++++++++++++++++++++++
>>   runner/settings.h |  1 +
>>   2 files changed, 23 insertions(+)
>>
>> diff --git a/runner/settings.c b/runner/settings.c
>> index 1ccf345d9..94c95f26e 100644
>> --- a/runner/settings.c
>> +++ b/runner/settings.c
>> @@ -612,6 +612,21 @@ char *absolute_path(const char *path)
>>   	return result;
>>   }
>>   
>> +int set_tests_datadir(void)
>> +{
>> +	const char *datapath = "../share/igt-gpu-tools";
> Hmm, maybe try also a build folder for local runs?
>
> ../build/data
> ../../build/data
for local runs image paths are handled/exported by meson. In meson 
imgdir will create the path for image directory.
imho, igt_runner change not required for build/data directory.
>> +	char *abpath;
>> +
>> +	abpath = absolute_path(datapath);
> Make sure abpath is not NULL before using it.
Sure i will change and float next rev.
>
>> +
>> +	if (getenv("IGT_DATA_PATH") == NULL)
> Please make it simpler:
> 	if (getenv("IGT_DATA_PATH"))
> 		return 0;
>
> And here add checks and settings.
>
>> +		return setenv("IGT_DATA_PATH", abpath, 1);
> Btw no need for "1" - you already checked it is unset.
i will fix this in next rev.
>
>> +
>> +	return 0;
>> +}
>> +
>> +
>> +
>>   static char *bin_path(char *fname)
>>   {
>>   	char *path, *p;
>> @@ -654,6 +669,10 @@ void init_settings(struct settings *settings)
>>   {
>>   	memset(settings, 0, sizeof(*settings));
>>   	IGT_INIT_LIST_HEAD(&settings->env_vars);
>> +
>> +	if (set_tests_datadir())
>> +		fprintf(stderr, "Data dir path not set\n");
> Could you also add printing of data path here?
>
> 	else
> 		printf("IGT data/ path: %s/\n", setting->data_path);
>
>> +
>>   	igt_vec_init(&settings->hook_strs, sizeof(char *));
>>   }
>>   
>> @@ -671,6 +690,9 @@ void clear_settings(struct settings *settings)
>>   	free_hook_strs(&settings->hook_strs);
>>   	free_array_deep((void **)settings->cmdline.argv, settings->cmdline.argc);
>>   
>> +	if (getenv("IGT_DATA_PATH") != NULL)
>> +		unsetenv("IGT_DATA_PATH");
>> +
> Please remember that is was set by runner so only unset this
> if runner was setting this env var.
IGT_DATA_PATH is added only for igt_runner use case. For local runs it 
is not required. This env variable is set and clear by igt_runner.
>
> Overall looks good,
>
> Regards,
> Kamil
>
>>   	init_settings(settings);
>>   }
>>   
>> diff --git a/runner/settings.h b/runner/settings.h
>> index 30743fc40..cbcf624b0 100644
>> --- a/runner/settings.h
>> +++ b/runner/settings.h
>> @@ -156,5 +156,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_tests_datadir(void);
>>   
>>   #endif
>> -- 
>> 2.43.0
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/igt-dev/attachments/20250508/7e53d0eb/attachment.htm>