50appstream: Only run as root

[iainlane]

'appstreamcli refresh' only works when you are root. If you are running
'apt update' as a normal user (for example some sandboxed tools do
this), then the failure of this Post-Invoke-Success hook causes the
update to fail.

E: Problem executing scripts APT::Update::Post-Invoke-Success 'if /usr/bin/test -e /usr/bin/appstreamcli; then appstreamcli refresh > /dev/null; fi'
E: Sub-process returned an error code

You could do this in some other ways too - adding || true to allow the refresh to fail or making appstreamcli exit 0 in this case.

[ximion]

Interesting - can you give an example of which tools do that? (I'm just very curious)
I wonder if in that case we should maybe replace the check of whether we are root with a check for the target location being writable...

[ximion]

Heh, just found https://bugs.launchpad.net/ubuntu/+source/appstream/+bug/1547428 in my inbox.
Do you think checking if we can write to the cache is better? Actually, there is no other reason why we need root permissions for cache updates, so this looks like a better idea.
In that case though, appstreamcli would fail if the cache dir isn't writable (/var/cache/app-info, and on Debian /var/lib/app-info (although that's gonna change, I have ideas for a much saner implementation)), and if that is generally the case in those sanboxed scenarios, checking for us to be root is probably better (as in your PR).
Ignoring all failures would hide some issues we would like to see, e.g. valid errors and crashes, so I would be reluctant to do that (could make debugging issues harder).

[iainlane]

Someone just reported it with apport, and I have a script which makes a "fake" apt environment to test transitions (you can override the apt system directories by doing apt -oDir=... -oDir::state=...). I also saw it with something else... struggling to remember what that is though.

laney@nightingale> appstreamcli refresh
You need to run this command with superuser permissions!

That's why I decided to do a check for root - if you want we could do as you say and drop this check in favour of checking the target and then also add a corresponding -w /var/lib/app-info into the apt hook instead of $(id -u) -eq 0.

What is your feeling?

[ximion]

I would go for the write-test - having the data might be useful in those test and sandbox scenarios, and also we shouldn't request root permissions when we don't actually need it...
I could implement the write-checking tomorrow (busy day today), but if you want to do it, please go ahead!
Thank you for spotting this and creating a patch - I wasn't even aware that this usecase existed.

[iainlane]

Superseded by #29