| Message ID | 20221204044347.23147-1-maxim.cournoyer@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [bug#59661,v2,1/3] system: Rename and move %base-packages-disk-utilities. | expand |
Hi, Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis: > Rationale: It is only used in INSTALLATION-OS and doesn't make sense to be > used in another context, given that file systems now automatically pull their > dependencies since commit 45eac6cdf5c8d9d7b0c564b105c790d2d2007799 (services: > Add file system utilities to profile). > > * gnu/system.scm (%base-packages-disk-utilities): Deprecate and rename to... > * gnu/system/install.scm (%installer-disk-utilities): ... this. > (installation-os) [packages]: Adjust accordingly. Efraim, this variable was added in e6e076281e62518056987e9ddd3d96fccab20475 and used in 4170af491c8bc3b0a5308116a26e758d8ff245c5; do you think it’s okay to remove now? (It LGTM, but I’d like to make sure we don’t create churn.) > +(define-deprecated %base-packages-disk-utilities #f '()) ‘#f’ here would lead to weird deprecation messages. I’d make it: (define-deprecated %base-packages-disk-utilities %base-packages '()) This is not quite accurate but it should convey the idea. Thanks, Ludo’.
Hi Ludovic, Ludovic Courtès <ludo@gnu.org> writes: > Hi, > > Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis: > >> Rationale: It is only used in INSTALLATION-OS and doesn't make sense to be >> used in another context, given that file systems now automatically pull their >> dependencies since commit 45eac6cdf5c8d9d7b0c564b105c790d2d2007799 (services: >> Add file system utilities to profile). >> >> * gnu/system.scm (%base-packages-disk-utilities): Deprecate and rename to... >> * gnu/system/install.scm (%installer-disk-utilities): ... this. >> (installation-os) [packages]: Adjust accordingly. > > Efraim, this variable was added in > e6e076281e62518056987e9ddd3d96fccab20475 and used in > 4170af491c8bc3b0a5308116a26e758d8ff245c5; do you think it’s okay to > remove now? (It LGTM, but I’d like to make sure we don’t create churn.) > >> +(define-deprecated %base-packages-disk-utilities #f '()) > > ‘#f’ here would lead to weird deprecation messages. I’d make it: > > (define-deprecated %base-packages-disk-utilities %base-packages '()) > > This is not quite accurate but it should convey the idea. I had shown an actual message example produced when using #f. It is what I want (it just mentions the variable is deprecated, and doesn't mention a replacement -- there are none in this case). For a quick reference, this is how 'warn-about-deprecation' is defined: --8<---------------cut here---------------start------------->8--- (define* (warn-about-deprecation variable properties #:key replacement) (let ((location (and properties (source-properties->location properties)))) (if replacement (warning location (G_ "'~a' is deprecated, use '~a' instead~%") variable replacement) (warning location (G_ "'~a' is deprecated~%") variable)))) --8<---------------cut here---------------end--------------->8--- So I prefer my version. If you still think the produced message is weird, I'll need a bit more explanation to understand why you think so :-).
On Sun, Dec 04, 2022 at 05:20:42PM +0100, Ludovic Courtès wrote: > Hi, > > Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis: > > > Rationale: It is only used in INSTALLATION-OS and doesn't make sense to be > > used in another context, given that file systems now automatically pull their > > dependencies since commit 45eac6cdf5c8d9d7b0c564b105c790d2d2007799 (services: > > Add file system utilities to profile). > > > > * gnu/system.scm (%base-packages-disk-utilities): Deprecate and rename to... > > * gnu/system/install.scm (%installer-disk-utilities): ... this. > > (installation-os) [packages]: Adjust accordingly. > > Efraim, this variable was added in > e6e076281e62518056987e9ddd3d96fccab20475 and used in > 4170af491c8bc3b0a5308116a26e758d8ff245c5; do you think it’s okay to > remove now? (It LGTM, but I’d like to make sure we don’t create churn.) I looked back through the commits around there. The whole point was the following commit, to divide the longish haphazard list of packages into sets of a sort. If it's only used in (gnu system install) then I don't see a problem with moving it there. The closest I know of to another user of %base-packages-disk-utilities is my gparted guix image¹. > > +(define-deprecated %base-packages-disk-utilities #f '()) > > ‘#f’ here would lead to weird deprecation messages. I’d make it: > > (define-deprecated %base-packages-disk-utilities %base-packages '()) > > This is not quite accurate but it should convey the idea. Would it be better to do (define-deprecated/alias-public %base-packages-disk-utilities %installer-disk-utilities) And remove the export from the list at the top of (gnu system)? ¹ https://git.sr.ht/~efraim/guix-config/tree/master/item/gparted.scm
Hi Efraim, Efraim Flashner <efraim@flashner.co.il> writes: [...] > Would it be better to do > > (define-deprecated/alias-public %base-packages-disk-utilities > %installer-disk-utilities) > > And remove the export from the list at the top of (gnu system)? Currently %installer-disk-utilities is not exported from (gnu system install). And even if it was, we'd still have to import it in (gnu system), which would perhaps introduce a module cycle. So I think the deprecation warning saying it's gone (i.e., using #f as replacement) is the most accurate and least problematic option.
Hi, Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis: > Ludovic Courtès <ludo@gnu.org> writes: [...] >>> +(define-deprecated %base-packages-disk-utilities #f '()) >> >> ‘#f’ here would lead to weird deprecation messages. I’d make it: >> >> (define-deprecated %base-packages-disk-utilities %base-packages '()) >> >> This is not quite accurate but it should convey the idea. > > I had shown an actual message example produced when using #f. It is > what I want (it just mentions the variable is deprecated, and doesn't > mention a replacement -- there are none in this case). For a quick > reference, this is how 'warn-about-deprecation' is defined: > > (define* (warn-about-deprecation variable properties > #:key replacement) > (let ((location (and properties (source-properties->location properties)))) > (if replacement > (warning location (G_ "'~a' is deprecated, use '~a' instead~%") > variable replacement) > (warning location (G_ "'~a' is deprecated~%") > variable)))) > > So I prefer my version. Oh I had overlooked it, sorry for the confusion. I prefer your version as well; LGTM! Ludo’.
Hi, Efraim Flashner <efraim@flashner.co.il> skribis: > On Sun, Dec 04, 2022 at 05:20:42PM +0100, Ludovic Courtès wrote: >> Hi, >> >> Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis: >> >> > Rationale: It is only used in INSTALLATION-OS and doesn't make sense to be >> > used in another context, given that file systems now automatically pull their >> > dependencies since commit 45eac6cdf5c8d9d7b0c564b105c790d2d2007799 (services: >> > Add file system utilities to profile). >> > >> > * gnu/system.scm (%base-packages-disk-utilities): Deprecate and rename to... >> > * gnu/system/install.scm (%installer-disk-utilities): ... this. >> > (installation-os) [packages]: Adjust accordingly. >> >> Efraim, this variable was added in >> e6e076281e62518056987e9ddd3d96fccab20475 and used in >> 4170af491c8bc3b0a5308116a26e758d8ff245c5; do you think it’s okay to >> remove now? (It LGTM, but I’d like to make sure we don’t create churn.) > > I looked back through the commits around there. The whole point was the > following commit, to divide the longish haphazard list of packages into > sets of a sort. If it's only used in (gnu system install) then I don't > see a problem with moving it there. OK. >> (define-deprecated %base-packages-disk-utilities %base-packages '()) >> >> This is not quite accurate but it should convey the idea. > > Would it be better to do > > (define-deprecated/alias-public %base-packages-disk-utilities > %installer-disk-utilities) > > And remove the export from the list at the top of (gnu system)? I don’t think we want users to rely on ‘%installer-disk-utilities’ in their OS config, so I’d go for Maxim’s version. Thanks, Ludo’.
diff --git a/gnu/system.scm b/gnu/system.scm index 3478afcec4..1c119c31b6 100644 --- a/gnu/system.scm +++ b/gnu/system.scm @@ -38,6 +38,7 @@ (define-module (gnu system) #:use-module (guix gexp) #:use-module (guix records) #:use-module (guix packages) + #:use-module (guix deprecation) #:use-module (guix derivations) #:use-module (guix profiles) #:use-module ((guix utils) #:select (substitute-keyword-arguments)) @@ -49,9 +50,6 @@ (define-module (gnu system) #:use-module (gnu packages bash) #:use-module (gnu packages compression) #:use-module (gnu packages cross-base) - #:use-module (gnu packages cryptsetup) - #:use-module (gnu packages disk) - #:use-module (gnu packages file-systems) #:use-module (gnu packages firmware) #:use-module (gnu packages gawk) #:use-module (gnu packages guile) @@ -896,20 +894,7 @@ (define %base-packages-networking ;; many people are familiar with, so keep it around. iw wireless-tools)) -(define %base-packages-disk-utilities - ;; A well-rounded set of packages for interacting with disks, - ;; partitions and filesystems, included with the Guix installation - ;; image. - (list parted gptfdisk ddrescue - ;; We used to provide fdisk from GNU fdisk, but as of version 2.0.0a - ;; it pulls Guile 1.8, which takes unreasonable space; furthermore - ;; util-linux's fdisk is already available, in %base-packages-linux. - cryptsetup mdadm - dosfstools - btrfs-progs - f2fs-tools - jfsutils - xfsprogs)) +(define-deprecated %base-packages-disk-utilities #f '()) (define %base-packages ;; Default set of packages globally visible. It should include anything diff --git a/gnu/system/install.scm b/gnu/system/install.scm index 003c49a3e7..d34d974338 100644 --- a/gnu/system/install.scm +++ b/gnu/system/install.scm @@ -48,6 +48,9 @@ (define-module (gnu system install) #:use-module (gnu packages bootloaders) #:use-module (gnu packages certs) #:use-module (gnu packages compression) + #:use-module (gnu packages cryptsetup) + #:use-module (gnu packages disk) + #:use-module (gnu packages file-systems) #:use-module (gnu packages fonts) #:use-module (gnu packages fontutils) #:use-module (gnu packages guile) @@ -458,6 +461,20 @@ (define %issue \x1b[1;33mUse Alt-F2 for documentation.\x1b[0m ") +(define %installer-disk-utilities + ;; A well-rounded set of packages for interacting with disks, partitions and + ;; file systems, included with the Guix installation image. + (list parted gptfdisk ddrescue + ;; We used to provide fdisk from GNU fdisk, but as of version 2.0.0a + ;; it pulls Guile 1.8, which takes unreasonable space; furthermore + ;; util-linux's fdisk is already available, in %base-packages-linux. + cryptsetup mdadm + dosfstools + btrfs-progs + f2fs-tools + jfsutils + xfsprogs)) + (define installation-os ;; The operating system used on installation images for USB sticks etc. (operating-system @@ -530,7 +547,7 @@ (define installation-os font-dejavu font-gnu-unifont grub ; mostly so xrefs to its manual work nss-certs) ; To access HTTPS, use git, etc. - %base-packages-disk-utilities + %installer-disk-utilities %base-packages)))) (define* (os-with-u-boot os board #:key (bootloader-target "/dev/mmcblk0")