Re: [PATCH i-g-t v3 5/5] runner/resultgen: Insert attachments list into results.json

[Gustavo Sousa <gustavo.sousa@intel.com>]

Zbigniew Kempczyński <zbigniew.kempczynski@intel.com> writes:

> On Wed, Apr 15, 2026 at 09:16:33AM -0300, Gustavo Sousa wrote:
>> Zbigniew Kempczyński <zbigniew.kempczynski@intel.com> writes:
>> 
>> > On Tue, Apr 14, 2026 at 06:39:28PM -0300, Gustavo Sousa wrote:
>> >> Zbigniew Kempczyński <zbigniew.kempczynski@intel.com> writes:
>> >> 
>> >> > Add list of filenames which were created by hooks in attachments directory
>> >> > (like GuC log copy) to results.json for allow presenting it in CI.
>> >> >
>> >> > Due to lack of userdata pointer in nftw() implementation main json
>> >> > object is passed via temporary static variable. It shouldn't be the
>> >> > problem because results.json is created in single thread. This change
>> >> > is not too elegant but allows to minimize the code change and is
>> >> > much easier to read comparing to recursive readdir().
>> >> >
>> >> > Signed-off-by: Zbigniew Kempczyński <zbigniew.kempczynski@intel.com>
>> >> > Cc: Kamil Konieczny <kamil.konieczny@linux.intel.com>
>> >> > Cc: Ryszard Knop <ryszard.knop@intel.com>
>> >> > Cc: Krzysztof Karas <krzysztof.karas@intel.com>
>> >> > ---
>> >> >  runner/resultgen.c | 63 ++++++++++++++++++++++++++++++++++++++++++++++
>> >> >  1 file changed, 63 insertions(+)
>> >> >
>> >> > diff --git a/runner/resultgen.c b/runner/resultgen.c
>> >> > index f8459385c3..d37f01a433 100644
>> >> > --- a/runner/resultgen.c
>> >> > +++ b/runner/resultgen.c
>> >> > @@ -1,11 +1,14 @@
>> >> >  #include <assert.h>
>> >> >  #include <ctype.h>
>> >> > +#include <dirent.h>
>> >> >  #include <fcntl.h>
>> >> > +#include <ftw.h>
>> >> >  #include <inttypes.h>
>> >> >  #include <stdio.h>
>> >> >  #include <string.h>
>> >> >  #include <sys/mman.h>
>> >> >  #include <sys/stat.h>
>> >> > +#include <sys/types.h>
>> >> >  #include <unistd.h>
>> >> >  
>> >> >  #include <jansson.h>
>> >> > @@ -1156,6 +1159,63 @@ static bool fill_from_dmesg(int fd,
>> >> >  	return true;
>> >> >  }
>> >> >  
>> >> > +struct json_t *tmp_tests;
>> >> > +static int ftw_attachments_list(const char *fpath, const struct stat *sb,
>> >> > +				int typeflag, struct FTW *ftwbuf)
>> >> > +{
>> >> > +	struct json_t *obj = NULL, *attobj = NULL;
>> >> > +	(void)sb;
>> >> > +	(void)ftwbuf;
>> >> > +
>> >> > +	if (typeflag == FTW_F) {
>> >> > +		char *p;
>> >> > +
>> >> > +		p = strstr(fpath + 2, "/");
>> >> > +		if (!p)
>> >> > +			return -1;
>> >> > +		*p = '\0'; /* temporary for acquiring the piglit name */
>> >> > +		obj = get_or_create_json_object(tmp_tests, fpath + 2);
>> >> 
>> >> For this to be trully recursive, I think we should create the JSON
>> >> object in the FTW_DP case.
>> >
>> > What for? Imo attachments array for test/subtest which contains
>> > relative paths from test/subtest is enough. I see no value by adding
>> > recursive arrays apart of increasing complexity to the code. Be aware CI has
>> > to present these attachments as well, so more we complicate results.json
>> > more work will be on CI side to alter javascript.
>> >
>> >> 
>> >> > +		attobj = get_or_create_json_array(obj, "attachments");
>> >> > +		*p = '/'; /* bring '/' back */
>> >> > +		json_array_append_new(attobj, escaped_json_stringn(fpath + 2, strlen(fpath + 2)));
>> >> > +	} else if (typeflag == FTW_DP) {
>> >> > +		;
>> >> > +	} else {
>> >> > +		return -1;
>> >> > +	}
>> >> > +
>> >> > +	return 0;
>> >> > +}
>> >> > +
>> >> > +static bool fill_from_attachments(int idirfd, struct json_t *tests)
>> >> > +{
>> >> > +	char attname[32];
>> >> > +	int attdirfd;
>> >> > +	DIR *currdir;
>> >> > +	int ret;
>> >> > +
>> >> > +	snprintf(attname, sizeof(attname), "%s", DIR_ATTACHMENTS);
>> >> > +	if ((attdirfd = openat(idirfd, attname, O_DIRECTORY | O_RDONLY | O_CLOEXEC)) < 0) {
>> >> > +		fprintf(stderr, "Error opening '%s' dir\n", DIR_ATTACHMENTS);
>> >> > +		return false;
>> >> > +	}
>> >> > +
>> >> > +	currdir = opendir(".");
>> >> > +	fchdir(attdirfd);
>> >> > +
>> >> > +	/*
>> >> > +	 * ftw doesn't support passing user data so *tests has to be
>> >> > +	 * set to some global for being visible in callback function.
>> >> > +	 * As results.json is not processed in multiple threads it is
>> >> > +	 * not a big problem.
>> >> > +	 */
>> >> > +	tmp_tests = tests;
>> >> > +	ret = nftw(".", ftw_attachments_list, 4, FTW_PHYS | FTW_DEPTH);
>> >> 
>> >> I think another option is to use fts(3)? Since it is a streaming API,
>> >> you can keep the state (i.e. the current json object) as a local
>> >> variable and update as you walk the tree.
>> >
>> > CI folks want minimize changes in results.json, so according to above
>> > I would keep everything in flat attachments array.
>> >
>> >> 
>> >> It seems it is not POSIX though.  Not sure if that's a real issue for
>> >> IGT, since stuff in IGT are supposed to run on Linux, right?
>> >
>> > And Android if I'm not wrong.
>> 
>> That's Linux as well, no?  Perhaps just need to have a sanity check to
>> see if those functions are also available there.
>> 
>> >
>> >> 
>> >> Thinking back, I guess it could fts(3) could even be used for the
>> >> recursive removal.
>> >
>> > Question - do we really need this? I can live with ugly static
>> > json at cost of simplicity especially this is only result generator
>> > which likely won't be touched by next months/years...
>> 
>> I would say: if the right tool is available for the job, why not using
>> it?
>> 
>> Not going to block on this though...
>> 
>> >
>> > For attachments case imo allowing recursive creation from hooks scripts
>> > is an overkill.
>> 
>> The hook mechanism is not something created for exclusive use by CI. It
>> is a tool for developers to use are they see fit.  That was my original
>> intention when I first developed the hooks infrastructure.
>> 
>> I don't like the fact that we are making assumptions of how the hooks
>> will behave here.
>> 
>> If CI wants a flat structure, that's fine: let CI provide hooks that
>> generate files in a flat hierarchy.  I just think that it is not right
>> to assume that all hooks will behave like that.
>
> Executing hooks out of igt_runner won't provide results.json, right?
> So all that recursive removal / etc doesn't makes sense when you execute
> tests standalone.

My comment above is not about running igt_runner vs standalone tests;
it is about the users having their own custom hook scripts, which could
be used as part of an igt_runner call.

By the way, did we reach an agreement about whether we should or not be
adding the attachments to the results.json?  I remember Ryszard had his
doubts.  I might have missed some conversation about it.

--
Gustavo Sousa

>
> --
> Zbigniew
>
>
>> 
>> --
>> Gustavo Sousa
>> 
>> >
>> > --
>> > Zbigniew
>> >
>> >> 
>> >> --
>> >> Gustavo Sousa
>> >> 
>> >> > +	fchdir(dirfd(currdir));
>> >> > +
>> >> > +	return ret ? false : true;
>> >> > +}
>> >> > +
>> >> >  static const char *result_from_exitcode(int exitcode)
>> >> >  {
>> >> >  	switch (exitcode) {
>> >> > @@ -2244,6 +2304,9 @@ static bool parse_test_directory(int dirfd,
>> >> >  		fprintf(stderr, "Error parsing output files (dmesg.txt)\n");
>> >> >  	}
>> >> >  
>> >> > +	if (!fill_from_attachments(dirfd, results->tests))
>> >> > +		fprintf(stderr, "Error parsing attachments directory\n");
>> >> > +
>> >> >  	override_results(entry->binary, &subtests, results->tests);
>> >> >  	prune_subtests(settings, entry, &subtests, results->tests);
>> >> >  
>> >> > -- 
>> >> > 2.43.0