kubo: 0.20.0 -> 0.21.0

[Mayeu]

Description of changes

https://github.com/ipfs/kubo/releases/tag/v0.21.0

The AcceleratedDHT experimental configuration flag graduated to stable, kubo should auto migrate the configuration, but I'm unsure if we should also warn the user via the module?

Things done

• Built on platform(s)
  • x86_64-linux
  • aarch64-linux
  • x86_64-darwin
  • aarch64-darwin
• For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
• Tested, as applicable:
  • NixOS test(s) (look inside nixos/tests)
  • and/or package tests
  • or, for functions and "core" functionality, tests in lib/tests or pkgs/test
  • made sure NixOS tests are linked to the relevant packages
• Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
• Tested basic functionality of all binary files (usually in ./result/bin/)
• 23.11 Release Notes (or backporting 23.05 Release notes)
  • (Package updates) Added a release notes entry if the change is major or breaking
  • (Module updates) Added a release notes entry if the change is significant
  • (Module addition) Added a release notes entry if adding a new NixOS module
• Fits CONTRIBUTING.md.

Since I'm on macOS I wasn't able to run the package tests, they seems to be linux only.

[Luflosi]

The AcceleratedDHT experimental configuration flag graduated to stable, kubo should auto migrate the configuration, but I'm unsure if we should also warn the user via the module?

Yes, because the migration is run only once while the NixOS module replaces the config file every time. The user configuration is also applied after the migration.

[Mayeu]

@Luflosi: thank you for the review. I'll go take a look at the module and check out how to do that.

[Mayeu]

@Luflosi I didn't realise ofborg would automatically re-asked a review. The current change for the module don't yet work as I'm getting this error when evaluating:

error: evaluation aborted with the following error message: 'cannot find attribute `services.kubo.settings.Experimental.AcceleratedDHTClient''

[fabianhjr]

Seems like that setting hasn't existed / doesn't exist in the module

It would be part of the definition on lines ~179 onwards:

      settings = mkOption {
        type = lib.types.submodule {
          freeformType = settingsFormat.type;

          options = {
            Addresses.API = mkOption {
              type = types.oneOf [ types.str (types.listOf types.str) ];
              default = [ ];
              description = lib.mdDoc ''
                Multiaddr or array of multiaddrs describing the address to serve the local HTTP API on.
                In addition to the multiaddrs listed here, the daemon will also listen on a Unix domain socket.
                To allow the ipfs CLI tools to communicate with the daemon over that socket,
                add your user to the correct group, e.g. `users.users.alice.extraGroups = [ config.services.kubo.group ];`
              '';
            };

            Addresses.Gateway = mkOption {
              type = types.oneOf [ types.str (types.listOf types.str) ];
              default = "/ip4/127.0.0.1/tcp/8080";
              description = lib.mdDoc "Where the IPFS Gateway can be reached";
            };

            Addresses.Swarm = mkOption {
              type = types.listOf types.str;
              default = [
                "/ip4/0.0.0.0/tcp/4001"
                "/ip6/::/tcp/4001"
                "/ip4/0.0.0.0/udp/4001/quic"
                "/ip4/0.0.0.0/udp/4001/quic-v1"
                "/ip4/0.0.0.0/udp/4001/quic-v1/webtransport"
                "/ip6/::/udp/4001/quic"
                "/ip6/::/udp/4001/quic-v1"
                "/ip6/::/udp/4001/quic-v1/webtransport"
              ];
              description = lib.mdDoc "Where Kubo listens for incoming p2p connections";
            };
          };
        };
        description = lib.mdDoc ''
          Attrset of daemon configuration.
          See [https://github.com/ipfs/kubo/blob/master/docs/config.md](https://github.com/ipfs/kubo/blob/master/docs/config.md) for reference.
          You can't set `Identity` or `Pinning`.
        '';
        default = { };
        example = {
          Datastore.StorageMax = "100GB";
          Discovery.MDNS.Enabled = false;
          Bootstrap = [
            "/ip4/128.199.219.111/tcp/4001/ipfs/QmSoLSafTMBsPKadTEgaXctDQVcqN88CNLHXMkTNwMKPnu"
            "/ip4/162.243.248.213/tcp/4001/ipfs/QmSoLueR4xBeUbY9WZ9xGUUxunbKWcrNFTDAadQJmocnWm"
          ];
          Swarm.AddrFilters = null;
        };
      };

Without the rename works fine, would suggest to limit this pull request to the update and open a second PR focused on tweaking the settings of the nixos module.

[Mayeu]

@fabianhjr:

Seems like that setting hasn't existed / doesn't exist in the module

OK so mkRenamedOptionModule only works if the option was present as some point in the module? Or would the error be solved by adding the to option to the settings definition? (Because I don't really get how the evaluation could know that a specific option existed in the history of the module but not anymore.)

[Mayeu]

Without the rename works fine, would suggest to limit this pull request to the update and open a second PR focused on tweaking the settings of the nixos module.

I'm OK with this, @Luflosi would that be fine for you too? If so I'll remove the mkRenamedOptionModule from this PR and open a new one for the module.

[Luflosi]

I'm a little worried that some people on the unstable channel have this option set and I don't know what will happen in that case.
I'll try to debug why mkRenamedOptionModule isn't working like we expect it to.

[Mayeu]

@Luflosi thank you for taking a look, it's a bit over my Nix/Nixpkgs knowledge 😅

[Luflosi]

I think I managed to fix it, take a look at #241931. But I don't know why this works or even what the problem was.
Who can I ping who has some knowledge about how the module system works? Maybe @roberth?

[roberth]

I would guess that mkRenamedOptionModule doesn't work because the paths involved aren't actually options. Are you referring to this?

error: evaluation aborted with the following error message: 'cannot find attribute `services.kubo.settings.Experimental.AcceleratedDHTClient''

If it's not a true option, it's not guaranteed to exist, and mkRenamedOptionModule wouldn't be designed to deal with such a situation. Maybe we should have a mkRenamedSettingModule that works for attributes that may not exist? It won't be able to remove the attribute at the old location though (something mkRenamedOptionModule can't do either). So then it may have to be mkRemovedSettingModule? That way we force the user to remove or rename it. It'd be little more than an assertion though.

cc @infinisil

[Luflosi]

Are you referring to this?

Yes, exactly.

If it's not a true option, it's not guaranteed to exist, and mkRenamedOptionModule wouldn't be designed to deal with such a situation.

I mean if a normal option gets renamed, the old option also no longer exists, right? So how would this situation be different? Shouldn't this be what mkRenamedOptionModule is supposed to do?

So is the problem here that the option is a child of settings, which is a submodule?

If all else fails, I can just write an assertion but I'd prefer a nicer solution as I can imagine that this situation is not exclusive to the Kubo module. mkRemovedSettingModule would be better than manually writing an assertion though I would prefer just altering the existing mkRenamedOptionModule and mkRemovedOptionModule implementations to handle this case properly if possible.

[roberth]

the old option also no longer exists, right?

That's not really the case. They do stick around. The fact renames are implemented by a module instead of a built-in feature sort of hints at that, but is by no means obvious.

Normally it's not a problem because we don't take config and then serialize the whole thing to a configuration file. When you use RFC 42 settings (ie freeformType and/or attrsOf) that changes and it becomes a problem.

So is the problem here that the option is a child of settings, which is a submodule?

It's not even an option, but yes, it is a child of settings.
I would expect the same problem to occur for renaming an attribute in attrsOf str.
submodule is involved, but its use with a freeformType makes it similar to the attrsOf str case.

I would prefer just altering the existing mkRenamedOptionModule and mkRemovedOptionModule implementations

The current behavior does serve as a protection against typos IIUC, so I think all three functions have their own use case to serve.

[Luflosi]

@Mayeu can you add an assertion to the existing list of assertions instead? I think something like this should work:

{
  assertion = !((lib.versionAtLeast cfg.package.version "0.21") && (builtins.hasAttr "Experimental" cfg.settings) && (builtins.hasAttr "AcceleratedDHTClient" cfg.settings.Experimental));
  message = ''
    The `services.kubo.settings.Experimental.AcceleratedDHTClient` option was renamed to `services.kubo.settings.Routing.AcceleratedDHTClient` in Kubo 0.21.
  '';
}

I haven't tested this yet though.

[Mayeu]

@Luflosi thanks, I tested with and without the AcceleratedDHT setting and it works as intended 👍

[Mayeu]

What's the next step to get this merged in, any one of you have write access to the repo?

[fabianhjr]

I don't have commit access

[nixos-discourse]

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-already-reviewed/2617/1044

[matthiasbeyer]

Someone please merge this, I have issues running the 0.20.0 (see ipfs/kubo#10056).

Maybe this resolves the issue...

[Mayeu]

@K900: thank you for merging this 🙏