On 12/12/2022 03:15, Cyril Brulebois wrote: > I've glanced at depthcharge-tools-installer and while I'm no debconf > expert and I can't assess the postinst with high certainty, the overall > impression was good enough for me to sign and upload; I did change the > Maintainer to debian-boot@ (same as other packages under installer-team/) > and moved you to Uploaders, so you might want to subscribe via tracker > once the package is accepted if you don't follow debian-boot@.
Thanks for the upload, and for the tracker subscription tip. I filter debian-boot@ into a folder that I do try to read, but a way that could notify me more prominently could be better. > [...] > > A couple of nitpicks/questions anyway: > > - depthcharge_tools_set_board(): > > The generated comment mentions preseed while I don't think preseed is > involved at all, and I suppose the comment should just mention > debian-installer instead? It's only created if depthcharge-tools-installer/board is set. That template is not asked as a user-visible question, but can be set via a preseed file or in the kernel command line [1]. I do the latter for testing things in a VM, and meant that by 'preseed'. [1] Using boot parameters to preseed questions https://www.debian.org/releases/testing/amd64/apbs02.en.html#preseed-bootparms > - initramfs_tools_conf(): > > Having MODULES overriden in a separate config file might be surprising > to admins. Did you consider adjusting this variable directly in the > main initramfs-tools config file instead? I tried to match what base-installer does [2]. In general, I think it's better to prefer config.d mechanisms over modifying the config files, to avoid conffile conflicts if/when the defaults change later on. [2] base-installer initramfs-tools driver inclusion policy handling https://salsa.debian.org/installer-team/base-installer/-/blob/master/library.sh#L571-592 > - isinstallable: > > It mentions “these values” but only checks for one. Should there other > patterns in that grep? Outdated comment that I missed. There's also cros_efi and cros_legacy, but they are included in some (apparently broken) GRUB/syslinux .cfg files to tell ChromeOS that we're not using its verified boot method. I used to check for them until I figured out it doesn't make sense to.