RPi-Distro / repo

Issue tracking for the archive.raspberrypi.org repo
37 stars 1 forks source link

Reduce repository overhead #256

Closed MichaIng closed 2 years ago

MichaIng commented 2 years ago

While fixing further conflicts between RPi desktop packages and LXDE, I had a look into a few packages shipped by the RPi repository, the patches they include and whether those are useful for RPi in general or for the RPi desktop only. While doing so I found that a lot of packages are, including all other packages from the same source, shipped with only a tiny change, which IMHO would make much more sense to either send upstream (or to Debian) or include in e.g. raspberrypi-sys-mods.

E.g. all packages from the policykit-1 source are shipped by RPi repository, while the only patch they contain is an additional little config file: /etc/polkit-1/localauthority.conf.d/51-debian-sudo.conf

[Configuration]
AdminIdentities=unix-group:sudo

I'm not sure about the practical implications this has, probably some extended permissions within desktop sessions when the user is member of the sudo group, like pi. However, I couldn't find where the source of this package/patch is hosted, hence no commit which explains the patch to be able to track down whether this still makes sense, or probably makes sense to apply upstream. But until then, wouldn't it make sense to ship this single config file as part of raspberrypi-sys-mods or raspberrypi-ui-mods, instead of shipping the whole package stack? This makes it much more transparent IMO, easier to skip, if wanted, (as its no overridden Debian core packages) and reduces overhead on maintaining and dealing with the RPi repository in general.

A similar example is the alsa-utils package, where a single tiny ALSA init configuration is shipped (which has not a practical effect, at best a visual one) and as a result the whole source package stack is shipped for four architectures (including i386 and amd64).

I didn't look through many more, but it seems to be or have been a strategy, which IMHO would make sense to change.

Overriding Debian packages causes additional issues when e.g. on Raspberry Pi OS 64-bit or a Debian repo (instead of Raspbian) + RPi repo are used and backports enabled, in which case the Debian repo packages are used and the intended packages for RPi are lost. In other cases is causes strange upgrade looks (#245), as the version suffix seems to be not consequently seen as "higher" version. So as long as there are no larger source changes (and even if), I see generally benefits in bundling RPi specific patches in separate packages instead of overriding the original packages names, especially when the changes are a single config file, and as the raspberrypi-(sys|net|ui)-mods and pi-gen do already exist to cover such things. Another point is transparency, as the packages meta/control files do not contain the RPi maintainer contact or actual source of the package and changes, but instead contact and links to the original Debian maintainers and Git repository are left in place, where the applied patches are of course not tracked, and hence it is hard to get the initial intention, whether this is still valid or wanted in the personal case etc.

XECDesign commented 2 years ago

The policykit package is a fix for a Raspbian bug. IIRC, rules file assumes it's either building for Debian or Ubuntu and skips installing that file if it's neither. The change to the package is just to make sure the behaviour is consistent between Debian and Raspbian.

I don't believe I made the change to alsa-utils, but I'm certain the change was made to fix an issue and isn't just a visual one.

I prefer to keep changes within the packages those changes belong. It doesn't make sense to me to ship a file for a package if that package isn't installed. Also, if the file changes in the upstream package, I wouldn't know about it and then the version installed by sys-mods wouldn't match what should've been installed by policykit, for example.

In the future, I can add a ~0 suffix to backports, to avoid overriding Debian's versions and indicate that there were no changes to the package.

Although this wasn't always the case, and I'm not the only one making changes, but I try to document changes in the changelog file and link to relevant issues, so that would be a good place to look. They'll also usually contain the name of the individual responsible for the change and who added it to the package.

MichaIng commented 2 years ago

The policykit package is a fix for a Raspbian bug. IIRC, rules file assumes it's either building for Debian or Ubuntu and skips installing that file if it's neither. The change to the package is just to make sure the behaviour is consistent between Debian and Raspbian.

Oh, you're right, that file is missing on Raspbian but present on Debian: https://packages.debian.org/bullseye/amd64/policykit-1/filelist I'd try to fix that on Raspbian then, but I guess it's practice there to not touch the sources at all, including the build scripts. Probably the changelog entry could then be a little more detailed than "Fix 51-debian-sudo.conf" 🙂. Furthermore, the two other packages from this source could be skipped, but I guess this makes building them more complicated as its not simply debhelper but some post-processing required, at least to change the dependency versions to be without the suffix.

Found the code: https://sources.debian.org/src/policykit-1/0.105-31/debian/rules/#L32-L41 Changing dpkg-vendor --is debian to dpkg-vendor --derives-from debian would cover Raspbian, as /etc/dpkg/origins/raspbian contains Parent: Debian. But Ubuntu would then match as well 🙈, so no Debian-side fix possible, aside of adding Raspbian as new explicit vendor. Alternatively setting DEB_VENDOR=debian env var for that build would do it and probably this is what you do already for the RPi build.

I prefer to keep changes within the packages those changes belong.

Generally I agree, but it has the mentioned downsides as well. Also exactly in case of policykit you're not consistent with this 😛: https://github.com/RPi-Distro/raspberrypi-sys-mods/blob/master/var/lib/polkit-1/localauthority/10-vendor.d/55-storage.pkla But as the debian-sudo.conf change is not one away from Debian but back to Debian, I agree that it would be highly confusing to have this in raspberrypi-sys-mods.

In the future, I can add a ~0 suffix to backports, to avoid overriding Debian's versions and indicate that there were no changes to the package.

You mean regarding the cmake package issue? I understand, so there is no suffix as it's a build of the unchanged Debian backports sources, makes sense. ~0 would be a workaround, but actually I don't believe it is intended that two exactly matching package versions from different repos cause such an upgrade loop.

I try to document changes in the changelog file and link to relevant issues, so that would be a good place to look.

Yes this is the major issue I had, to find info (by myself, without contacting someone) where/why a change/patch has been done. If no browsable Git repository exists, then a link to an issue or so would be perfectly fine, or a sentence that makes it clear.

XECDesign commented 2 years ago

I'd try to fix that on Raspbian then, but I guess it's practice there to not touch the sources at all, including the build scripts.

There are cases where Debian's own rules files take Raspbian into account (like making sure gcc builds armv6 hfp binaries by default), so I don't think there any reason it couldn't be fixed there. It's just quicker and less painful to add a patch ourselves than go through Debian's processes. If somebody else wants to upstream changes, that's great, but I'm not inclined to do so myself.

Probably the changelog entry could then be a little more detailed than "Fix 51-debian-sudo.conf"

Yeah, I wouldn't claim that to be an example of a helpful changelog entry.

Generally I agree, but it has the mentioned downsides as well. Also exactly in case of policykit you're not consistent with this stuck_out_tongue: https://github.com/RPi-Distro/raspberrypi-sys-mods/blob/master/var/lib/polkit-1/localauthority/10-vendor.d/55-storage.pkla

Because the package that file comes from shouldn't actually be coupled to that package. I believe it was lifted from one of Ubuntu's gnome packages. Obviously we can't drag in gnome dependencies, and would like that file to be present even on headless systems, so it makes more sense to include it in sys-mods. I feel like there's no one size fits all approach and these things just need to be taken on a case-by-case basis.

I don't believe it is intended that two exactly matching package versions from different repos cause such an upgrade loop.

Yeah, I haven't seen that before cmake, but I suspect it's because the checksums of the deb files don't match. Under normal circumstances you'd never have deb packages with the same name/version, but different checksums. Shouldn't be an issue in bullseye where I'll make sure to add the suffix.

MichaIng commented 2 years ago

If somebody else wants to upstream changes, that's great

I'll give it a try.

I feel like there's no one size fits all approach and these things just need to be taken on a case-by-case basis.

True. Also shipping it with policykit-1 makes breaks the ability to have this unmodified.

I suspect it's because the checksums of the deb files don't match

Was my guess as well, as e.g. having two mirrors of the same repo added does not cause the loop. An extreme edge case, but if there is no explicit reason to treat the same package with same version but different checksum as newer, it can be probably fixed upstream. I'll give it a try.

XECDesign commented 2 years ago

Closing, since I don't think that there's anything to change on our end.

MichaIng commented 2 years ago

I agree. Thinking through this again makes me agree that package-related changes should stay in the particular package. To keep forks/patches minimal, sending them upstream is a different topic.