Better error when filesystem format has an unsupported value

[iFreilicht]

Right now, the filesystem type's format argument supports arbitrary strings.

This can cause confusion when you expect to be able to insert zfs, for example, and expect it to just work, see #820.

One solution, suggested in #820 (comment), would be to check for known bad values like zfs, and return an error. This could be implemented with NixOS module assertions as we're relying on evalModules already.

Another option would be to change the type to only allow filesystems that can be built with an mkfs.<type> command. From what I can tell, this is the entire list of currently supported file systems:

• bfs
• cramfs
• minix
• xfs
• btrfs
• vfat
• ext2
• ext3
• ext4
• bcachefs
• f2fs

Both options have their merits. Any opinions?

[Enzime]

My preference would be the latter, we could even allow users to specify zfs and then we print out a helpful error message

[Lassulus]

the problem is with an inclusive list, is that people need to patch disko to add support for new filesystems. currently you can run any mkfs.* command and maybe the package is just missing but that can be passed by ammending PATH externally. I used that a couple of times for exfat or ntfs-3g.

[Enzime]

I think it would be fine if users just sent through PRs just to update the list of filesystems supported (without adding full support) whenever they encountered new filesystems that have a mkfs that they want to use

A few more that haven't been mentioned yet:

• mkfs.c from plan9port
• mkfs.fat from nu_scripts and dosfstools
• mkfs.erofs from erofs-utils
• mkfs.msdos from dosfstools

You can get a list of all of the Hydra built (allowUnfree = false;) ones:

$ nix run github:nix-community/nix-index-database -- --regex 'mkfs\..*'
util-linuxMinimal.man                             1,583 r /nix/store/9dblsj9vid6d0xd94j9d0z9wc3pvypq0-util-linux-minimal-2.39.4-man/share/man/man8/mkfs.8.gz
util-linuxMinimal.man                             1,335 r /nix/store/9dblsj9vid6d0xd94j9d0z9wc3pvypq0-util-linux-minimal-2.39.4-man/share/man/man8/mkfs.bfs.8.gz
util-linuxMinimal.man                             1,666 r /nix/store/9dblsj9vid6d0xd94j9d0z9wc3pvypq0-util-linux-minimal-2.39.4-man/share/man/man8/mkfs.cramfs.8.gz
util-linuxMinimal.man                             1,712 r /nix/store/9dblsj9vid6d0xd94j9d0z9wc3pvypq0-util-linux-minimal-2.39.4-man/share/man/man8/mkfs.minix.8.gz
...

[iFreilicht]

we could even allow users to specify zfs and then we print out a helpful error message

Ah yes that's a better option than the user simply seeing "value of zfs is not supported"

think it would be fine if users just sent through PRs just to update the list of filesystems supported (without adding full support) whenever they encountered new filesystems that have a mkfs that they want to use

Yeah I agree. And with a bit of effort I think we can basically create a list of all filesystems that we know we can support, and leave out those that we can't.

Of course there will be cases of someone wanting to use their custom-made filesystem together with a custom package, but maybe would could solve that differently then?

We could extend the type to allow either strings for these simple cases, or an attrset like

{
  name = "dwarffs";
  package = pkgs.dwarffs;
  binary = "dfs-create";
}

This would allow maximum flexibility and discoverability at the same time.

[iFreilicht]

Ah, and we should probably make this change a two-step process by first issuing warnings when a user specifies a format that we don't know about, and encourage posting an issue, and only actually change the type in the next major release.

[iFreilicht]

Good example of why an inclusive list will be a breaking change: #840

[iFreilicht]

I tried to implement the less controversial solution (assertions), but haven't succeeded yet. The obvious first thing to do was to add an assertion to _config:

      _config = lib.mkOption {
        internal = true;
        readOnly = true;
        default = lib.optional (config.mountpoint != null) {
          fileSystems.${config.mountpoint} = {
            device = config.device;
            fsType = config.format;
            options = config.mountOptions;
          };
+         assertions = [
+           {
+             assertion = !lib.elem config.format (lib.attrNames diskoLib._deviceTypes);
+             message = ''
+               Format ${config.format} is not valid for type = "filesystem"!
+               Please use type = "${config.format}" instead.
+             '';
+           }
+         ];
        };
        description = "NixOS configuration";
      };

However, when I actually tried to evaluate an erroneous config (simple-efi.nix with formats changed to zfs and mdraid), I instead got this error:

nix build .#checks.x86_64-linux.simple-efi                                                                                                                                                                               1 ✘  ▼  impure  
warning: Git tree '/home/felix/repos-new/disko' is dirty
trace: evaluation warning: mdadm: Neither MAILADDR nor PROGRAM has been set. This will cause the `mdmon` service to crash.
error:
       … while evaluating the attribute 'drvPath'
         at /nix/store/csfywra6032psdbgna9qcbdads87gmzw-source/lib/customisation.nix:365:7:
          364|     in commonAttrs // {
          365|       drvPath = assert condition; drv.drvPath;
             |       ^
          366|       outPath = assert condition; drv.outPath;

       … while evaluating 'strict' to select 'drvPath' on it
         at /builtin/derivation.nix:1:552:
       (stack trace truncated; use '--show-trace' to show the full trace)

       error:
       Failed assertions:
       - ZFS requires networking.hostId to be set
       - Automatic pool detection found an empty pool name, which can't be used.
       Hint: for `fileSystems` entries with `fsType = zfs`, the `device` attribute
       should be a zfs dataset name, like `device = "pool/data/set"`.
       This error can be triggered by using an absolute path, such as `"/dev/disk/..."`.

So somehow, the assertion wasn't merged, or not evaluated in time?

My second attempt was to add a new internal _assertions option to all types with a content option and extract that in the toplevel, but I didn't manage to do that, either.