Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Kodi postinst script fails if polkit-1 dir does not exist #153

Open
MichaIng opened this issue Oct 16, 2019 · 13 comments
Open

Kodi postinst script fails if polkit-1 dir does not exist #153

MichaIng opened this issue Oct 16, 2019 · 13 comments

Comments

@MichaIng
Copy link

MichaIng commented Oct 16, 2019

Sorry if this is to wrong place to report this, but I got confused and couldn't find out which "xmbc" fork finally serves as source the the RPi repo package.

However, the package postinst scripts contains this:

if [ ! -f "/etc/polkit-1/localauthority/50-local.d/kodi.pkla" ]; then
tee -a /etc/polkit-1/localauthority/50-local.d/kodi.pkla > /dev/null 2>&1 <<_EOF_
...

Since policykit1 is not a dependency of the kodi package, the required /etc/polkit-1/localauthority/50-local.d dir does not always exist, hence the install fails.

Three possible solutions:

  • Check for dir, at least /etc/polkit-1, before creating the file. mkdir -p /etc/polkit-1/localauthority/50-local.d required.
  • If this rule should be assured as well for later policykit1 installs, mkdir -p /etc/polkit-1/localauthority/50-local.d and skip above check.
  • If for some reason those rules should be effective, policykit1 must be added as dependency. But I would leave this for user choice, probably add to suggestions or recommends instead.

Reference: MichaIng/DietPi#3031 (comment)

@derdershat
Copy link

I think the deepest source is https://github.com/popcornmix/xbmc
so an @popcornmix could help

@XECDesign
Copy link
Member

XECDesign commented Oct 16, 2019

The package is kindly provided by Rascas of pipplware - I've already left a message here https://www.raspberrypi.org/forums/viewtopic.php?f=66&t=251645&start=125#p1552570

@diogomsantos
Copy link

Hi, I am rascas, the Kodi packages maintainer.

The Kodi 18 Leia source code for the RPi 0/1/2/3 is available here:
https://github.com/PIPplware/xbmc/tree/leia_backports
And Kodi 18 Leia source code for the RPi 4 is available here:
https://github.com/PIPplware/xbmc/tree/leia_pi4

More info here:
https://www.raspberrypi.org/forums/viewtopic.php?f=66&t=251645

So correct me if I am wrong: DietPi for the RPi is based on Raspbian but not on the pre made oficial images. I am asking this because policykit-1 is installed on the pre made images.

Anyway, what the polkit rule does is to allow the user who will run Kodi, to be able to reboot and shutdown the Pi and to mount/unmount devices inside Kodi. Maybe the best is to just add policykit-1 as a dependency, but I can do whatever you think is best,

@MichaIng
Copy link
Author

@diogomsantos
Jep DietPi RPi images are based on Raspbian, but come with very few pre-installed packages, instead installs them ondemand via software install or config scripts.

We solve Kodi permissions via udev rules (generally for all devices, not just RPi), however policykit-1 of course is another nice solution, if installed. But I would keep dependencies as low as possible.

Wouldn't it be easiest to make /etc/polkit-1/localauthority/50-local.d/kodi.pkla a regular deb package config file, so the dir and file are created automatically by dpkg, if not yet existent, else user is asked by dpkg to keep old file, if it has changed, or use maintainer updated one? Otherwise I personally would prefer simply mkdir -p /etc/polkit-1/localauthority/50-local.d before the cat. In both ways the policy becomes effective as fast as policykit-1 is installed, but the choice is left to user.

@XECDesign
Copy link
Member

If you go with the mkdir route, the nice thing to do in postrm purge would be to remove the pkla file and all created directories if they're empty.

@MichaIng
Copy link
Author

E.g.

[[ ! -f '/etc/polkit-1/localauthority/50-local.d/kodi.pkla' ]] || rm /etc/polkit-1/localauthority/50-local.d/kodi.pkla
rmdir -p --ignore-fail-on-non-empty /etc/polkit-1/localauthority/50-local.d
  • But as well here the regular deb package config file automates much, keeps those when doing apt remove kodi and purges them when doing apt purge kodi. So IMO nice to go with this native implementation and keep the postinst/postrm scripts simple?

@XECDesign
Copy link
Member

XECDesign commented Oct 16, 2019

If I was doing this I'd probably ship it as a normal file in /var/lib/polkit-1/localauthority/50-local.d

@diogomsantos
Copy link

Putting the rule on a conffile was my first idea, but I use cmake cpack deb for a while now, as it is easier and supported in upstream Kodi, and adding a conffile there didn't look easy, so I ended up using postinst/postrm.

@PanderMusubi
Copy link

Are there any updates for this issue?

@MichaIng
Copy link
Author

MichaIng commented May 13, 2021

To re-activate this. Isn't it possible to simply add a conffiles file here: https://github.com/PIPplware/xbmc/tree/leia_pi4/cmake/cpack/deb
With content:

/etc/polkit-1/localauthority/50-local.d/kodi.pkla

Now I don't see where the file itself would need to be added, but since postinst and postrm seem to be used by cmake cpack like that, conffiles should be used the same way.

As alternative I'd actually second @XECDesign's idea to make it a regular file. I lost track what the reason was to not hardcode it into the package, if there was any? Shipping it statically as /var/lib/polkit-1/localauthority/50-local.d/kodi.pkla still allows admins to override it, when wanted, by creating an own /etc/polkit-1/localauthority/50-local.d/kodi.pkla. It has the benefit that the original file stays intact, custom changes can be easily reverted (by removing the custom file from /etc) and also maintainer-wise new rules/defaults can be shipped without requiring the admin to confirm overwriting an old config file on upgrade (when using the above conffiles method).

@diogomsantos
Copy link

If you tell me how to do that with cmake cpak deb, I will gladly do it.

@MichaIng
Copy link
Author

MichaIng commented May 13, 2021

I see you added policykit-1 as dependency already: PIPplware/xbmc@1320da2
So the issue should be fixed by this (although this has not yet been uploaded into the repo). Changing the way how these files are added would still have some benefits, e.g. also making the udev rules a static file /lib/udev/rules.d/99-kodi.rules or /usr/lib/udev/rules.d/99-kodi.rules to leave /etc/udev/rules.d for local admin additions/overrides only, as intended.

Out of interest, I was digging a little further. Simply placing the conffiles file into https://github.com/PIPplware/xbmc/tree/leia_pi4/cmake/cpack/deb is not enough, but it needs to be added here as well: https://github.com/PIPplware/xbmc/blob/leia_pi4/cmake/cpack/CPackConfigDEB.cmake#L314-L326
This can be used to add further files to the package, but since DESTINATION share/ is used, it seems to place everything below /usr/ and hence I don't know how to add one below /lib/ or /var/ with this install function. Cleanest seems to be to extend the variable CPACK_INSTALLED_DIRECTORIES by a new directory structure to bundle additional files:

When I find some more spare time and joy to tinker with this, I'll open a new issue and PR at your repo as a testing and discussion basis. But lacking cpack experience, it would be trial and failure and currently IMO not worth the effort. It's a new issue anyway, not to fix the OP, so I mark this as closed 🙂.

@MichaIng
Copy link
Author

MichaIng commented Oct 22, 2023

The issue re-appeared with Bookworm, since the policykit-1 package there does not ship /etc/polkit-1/localauthority.conf.d anymore.
Compare the file list of the package on Bullseye: https://packages.debian.org/bullseye/arm64/policykit-1/filelist
With Bookworm:

Checking changelogs, the directory has been obsoleted: https://metadata.ftp-master.debian.org/changelogs//main/p/policykit-1/policykit-1_122-3_changelog

  • postinst: Only clean up config directories if not owned.
    If we only have polkitd installed, then we want to clean up the obsolete
    directory /etc/polkit-1/localauthority.conf.d on upgrade, but if we
    have polkitd-pkla installed, then it owns that directory and we should
    not remove it. (Closes: #1026425)

There is a legacy package which should pre-create it instead: https://packages.debian.org/bookworm/polkitd-pkla
But I guess it makes sense to use the modern methods instead. I have no deep insights, but looks like /etc/polkit-1/rules.d would be used nowadays.

EDIT: Related notes from Debian: https://www.debian.org/releases/bookworm/arm64/release-notes/ch-information.en.html#changes-to-polkit-configuration

EDIT2: Now it is @popcornmix: https://github.com/popcornmix/xbmc/blob/gbm/cmake/cpack/deb/postinst#L22-L38
Shall I open a new issue over there? I can also open a PR when legacy polkitd-pkla shall be used, but I do not know (yet) how to translate it into modern /etc/polkit-1/rules.d JavaScript rules.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants