Honor the non-default data prefix when looking for the hicolor theme

[arrowd]

This is a follow-up to #132

[ximion]

This looks incorrect - this code is supposed to find the built-in copy of the theme. If this doesn't work, then something might be wrong with your installation of asgen.

What bug is this fixing?

[arrowd]

I'm not sure what "built-in" means in this context. My understanding is that it is the theme that is installed in the system that runs asgen. In this case it is located under /usr/local/ on FreeBSD, so I need this change for asgen to find it.

[ximion]

No, this code is searching for an embedded copy that ships with asgen itself, and if you configured asgen properly at compile-time, then Utils::getDataPath will take care of finding that copy. It is only used if the hicolor theme could not be found in the analyzed repository itself, which may be the case for EPEL stuff or PPAs on Ubuntu.

Erroring out when that file wasn't found, like the original code does, is the right behavior here - because if Utils::getDataPath didn't find that piece of data, it means something is wrong with the asgen installation and there's probably more stuff that would break.

[arrowd]

Utils::getDataPath will take care of finding that copy.

It falls back to /usr/share at the very end, which made me think that it also should check the extra prefix.

Anyways, why bundle that file as part of the asgen package rather than relying on the system package management to provide it?

[ximion]

It falls back to /usr/share at the very end, which made me think that it also should check the extra prefix.

It has checked DATADIR prior, which should point to /usr/local/share/appstream if the prefix is /usr/local.

Anyways, why bundle that file as part of the asgen package rather than relying on the system package management to provide it?

Because we don't want to depend on a full icon theme and spotting this issue is pretty hard if you forgot the dependency. But also for sandboxing reasons where we don't want to access things from the host at all when the tool runs in a sandbox. Providing the file outright as a vetted fallback was simply the most robust solution.

Remember, this theme definition will only ever be used if the repository didn't contain the hicolor icon theme already - if it does, we prefer the copy from the analyzed data.

[arrowd]

All right, thanks for explanations, this all makes sense!

The last unclear piece for me, however, is

Remember, this theme definition will only ever be used if the repository didn't contain the hicolor icon theme already - if it does, we prefer the copy from the analyzed data.

But the theme is provided by one of thousands of packages in the repository. Does asgen first scan the whole repo looking for a theme package and then proceeds to the main processing? Or this theme data only required at the end of the scan?

[ximion]

But the theme is provided by one of thousands of packages in the repository. Does asgen first scan the whole repo looking for a theme package and then proceeds to the main processing?

Yes, that is exactly what happens ;-)

There is two steps to this, first the tool builds an index of the entire repository of stuff that it could use at a later run, and then in a second step it processes the data and actually extracts the information.

All of this is cached though, so on subsequent runs it will only process new packages. It will also prune old data from the index once a package leaves the repository.

[arrowd]

Great, thanks for clearing this up. I'll get back to you with FreeBSD backend changes once I try out the new version.

[ximion]

This is also the reason why processing files directly without a larger repo scan is not ideal - asgen needs a view of what's in the repository (depending on packaging, the impact of this may vary though).

[arrowd]

Yep, I also thought about this. But I'm still going to try to make it work by acting on an unpacked packages during the repository building. It feels so wasteful to unpack archives after the repo is build that I can't accept it without trying to improve the situation.

[ximion]

Well, if you keep the caches, subsequent runs will be really, really cheap - it also permits running the generator on a sandboxed external machine. You have to keep the caches though, otherwise this will indeed be wasteful (there was a case of someone deleting the workspace db directory between runs in the past, and then wondering why every run took so long...).

That said, if you find something that works well for you, of course try it! (just keep in mind that even processed files will be cached and must have correct package-ids like they would have in the archive).