Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

modules: make apply mergeable #301472

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

modules: make apply mergeable #301472

wants to merge 1 commit into from

Conversation

Atry
Copy link
Contributor

@Atry Atry commented Apr 4, 2024

Description of changes

As discussed here and here, a restriction in module system in comparison to overlay is that we cannot visit previous value, nor transform it into a new one.

This PR aims to make apply mergeable, therefore modules can visit previous values by defining an apply function.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • 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/)
  • 24.05 Release Notes (or backporting 23.05 and 23.11 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.

Add a 👍 reaction to pull requests you find important.

@github-actions github-actions bot added 6.topic: module system About "NixOS" module system internals 6.topic: lib The Nixpkgs function library labels Apr 4, 2024
@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/drv-parts-configure-packages-like-nixos-systems/23629/27

@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/adding-more-control-over-overriding-default-module-definitions/4276/8

@roberth
Copy link
Member

roberth commented Apr 4, 2024

I don't think we should do this, because it makes more code dependent on the import order, which is too unpredictable and un-declarative, similar to what I've argued in the linked drv-parts thread.

@Atry
Copy link
Contributor Author

Atry commented Apr 4, 2024

Most existing overlays and overrides are commutative. The order does not matter at all in most use cases.

@Atry
Copy link
Contributor Author

Atry commented Apr 4, 2024

The use case would be to patch an existing package while not breaking other patches. In that case, the users would not depend on module application order.

@Atry
Copy link
Contributor Author

Atry commented Apr 4, 2024

because it makes more code dependent on the import order

Could you elaborate in what use case the code would depend on the import order?

@roberth
Copy link
Member

roberth commented Apr 4, 2024

The use case would be to patch an existing package while not breaking other patches. In that case, the users would not depend on module application order.

what use case

Some examples in haskell.lib are

  • doCheck and dontCheck, in which case the last one wins
  • buildFromSdist, creates a copy of the package, modifies it, and uses it as the src. The state of the copy depends on the order; only preceding operations are included.

@infinisil
Copy link
Member

infinisil commented Apr 4, 2024

I agree with Robert here. apply in general should be viewed as an anti-pattern, and ideally be deprecated completely.

@Atry
Copy link
Contributor Author

Atry commented Apr 4, 2024

Some examples in haskell.lib are

  • doCheck and dontCheck, in which case the last one wins
  • buildFromSdist, creates a copy of the package, modifies it, and uses it as the src. The state of the copy depends on the order; only preceding operations are included.

OK. I see your point.
In an ideal world, all packages are drv-parts, all configurations are module system, we would not need mergeable apply, then we can remove apply as an anti-pattern as @infinisil mentioned.

Unforunately given that nixpkgs still includes more packages than dream2nix. We still live in a world mixed of modules and overrides, we still need a way to apply a patch in the module system.

@roberth you said "That doesn’t mean that we shouldn’t have it. In fact by having them, we have a good place to document why they are the way they are, and that they’re a last resort."

@roberth
Copy link
Member

roberth commented Apr 4, 2024

@roberth you said "That doesn’t mean that we shouldn’t have it. In fact by having them, we have a good place to document why they are the way they are, and that they’re a last resort."

That was in reference to types endo and overlay, with checks that require either all definitions to have distinct mkOrder numbers. (Noting that a definition like mkMerge ([ a b ] ++ optionals c [ d ]) is a single definition and doesn't need mkOrder.)

In apply, we don't have the luxury of mkOrder, and while that could be added, I doubt that we should. Adding an explicit option to perform the postprocessing has the benefit that it's documented; more discoverable. It also doesn't hurt performance.

@Atry
Copy link
Contributor Author

Atry commented Apr 4, 2024

with checks that require either all definitions to have distinct mkOrder numbers.

Suppose I want to create a NixOS module that patches java command line to provide java --my-fancy-dev-flag, so I do this:

{lib, ...}: {
  config.programs.java.enable = true;
  options.programs.java.package = lib.mkOption {
    apply = old: old.override { ... };
  };

  # Don't use `nixpkgs.overlays` in case of invalidating the binary cache of packages depending on `pkgs.jdk`
  # config.nixpkgs.overlays = [(final: prev: { jdk = prev.jdk.override { ... }; })];
}

I want apply to be mergeable so that the module would not accidentally conflict with another module that also patches programs.java.package. How can I determine the mkOrder number if options.programs.java.package is an endo or overlay type?

@roberth
Copy link
Member

roberth commented Apr 4, 2024

It looks like you're recreating the traditional FHS problem of having only a single instance of a program, which has to satisfy all possible requirements simultaneously.

If you're sure that's what you want, you could improve the programs.java module to support all those requirements. Centralizing that makes sense because the requirements may interact. It also makes your improvements available to others, and invites others to share theirs, so you can make use of them too.

Otherwise, I would recommend nix shells for projects, and independent java packages for each service that needs one.
If some services have the same version and overrides, they may still share a store path, thanks to hashing.

If you don't want explicit support for the things you're doing, and you would rather expose the apply attribute through an option with a type like endo or overlay, as you say, you will have to figure out the numbers for your configuration somehow. I don't think that's exposed in options just yet, but maybe that could be added, perhaps in the form of raw definitions. I wonder how costly that is.

@Atry
Copy link
Contributor Author

Atry commented Apr 4, 2024

It looks like you're recreating the traditional FHS problem of having only a single instance of a program, which has to satisfy all possible requirements simultaneously.

The module is not necessarily a NixOS module. When it's a flake part or devenv module, the option could be a per project package, for example, languages.python.package. In that case, it's not a single instance of a program.

If you're sure that's what you want, you could improve the programs.java module to support all those requirements. Centralizing that makes sense because the requirements may interact. It also makes your improvements available to others, and invites others to share theirs, so you can make use of them too.

The build options are already available in pkgs.jdk as override arguments. Ideally programs.java should only include the runtime options of the program, not the build options.

you will have to figure out the numbers for your configuration somehow.

What does "have to" mean?

  1. This PR does not enforce the order number
  2. @roberth's proposal to enforce the order number does not help the typical use case, where the apply function is assumed commutative.

The doCheck vs dontCheck is a good example where overrides are assumed to be commutative, but not actually 100% commutative, which is unfortunate. However, how would an enforced order number mitigate the unfortune? If we enforce the order number, what is the best practice to determine the order number?

@physics-enthusiast
Copy link
Contributor

physics-enthusiast commented May 24, 2024

How about having a 2-level merging system? First merge all the non-apply options. Then we treat apply itself like an option (with mkDefaults, mkOverrides, and mkForces), and only apply one. Then those who want composition can set apply.apply. We could even add a mkPrecedence to lib to allow the creation of an arbitrarily large attrset tower of applys for those who need the increased composability.

i.e.

mkPrecedence = n: applyFunc:
    assert n >= 1
    assert (typeOf n == "int")
    if n==1 then
        { apply = applyFunc }
    else
        { apply = mkPrecedence (n-1) applyFunc }

This way, it becomes unambiguous which apply is applied first, throwing when that is not possible like our mkOption system does.

@roberth
Copy link
Member

roberth commented May 24, 2024

@Atry

you will have to figure out the numbers for your configuration somehow.

What does "have to" mean?

1. This PR does not enforce the order number

2. @roberth's proposal to enforce the order number does not help the typical use case, where the `apply` function is assumed commutative.

I was trying to illustrate why a composition of functions is not properly declarative, by showing workarounds. I don't think we need to go into much more detail here to see that the workarounds aren't great, but we could if you think we need to.

Crucially, it's important for the module system to provide sound abstractions that behave predictably, as much as possible. Every time we introduce a feature that's not perfect, but helps a particular use case, it will be used in cases where it's not needed, leading to annoyances and bugs. This in turn damages the reputation of the module system, making it seem unreliable.
Documentation can maybe mitigate this slightly, but not prevent this. It's self-defeating: the module system is great, so how bad could this be? We barely use this new option anyway...

However, how would an enforced order number mitigate the unfortune? If we enforce the order number, what is the best practice to determine the order number?

It gives a user-visible clue as to what might be going on. I think it's a bit early to ask for best practices; I think something might emerge.
What I do know is that it will be unpleasant. The best way to avoid it is to just have a single definition, in which case you won't have to specify anything like mkAfter/mkOrder/etc.
That's not modular, but that's because the original problem statement (function composition) just does not have a modular solution.

@roberth
Copy link
Member

roberth commented May 24, 2024

@physics-enthusiast

How about having a 2-level merging system? First merge all the non-apply options. Then we treat apply itself like an option (with mkDefaults, mkOverrides, and mkForces), and only apply one.

Laziness makes it so that the way things are evaluated is very flexible by default. We probably don't need to create "phases" like that at the module level, unless you really want to generate more actual options, in which case the overhead might double, or worst case something like exponential in the depth of submodule nestings. (In case both phases need to evaluate both phases of a submodule)

Instead of building something extra into the module system, we can already do this in the cases where we need it:

{ config, ... }:
{
  options = {
    foo = mkOption {
      type = <...>;
      apply = config.fooModifiers;
    };
    fooModifiers = mkOption {
      type = types.composedFunction;  # Example, this type doesn't exist yet
    };
  };
}

This way you can pick how you want the functions to be merged, and we don't need to complicate the module system or make it slower by even one conditional.
It also does not require that your users learn a new module system feature. Instead they'll find these options in the module documentation where it's relevant.

@physics-enthusiast
Copy link
Contributor

My bad, I had misunderstood apply as something the option setter supplied, rather than the option provider. I still think the ability to pass a magic value that essentially says "give me whatever value this would have been, and let me modify it" as a module user would be cool, but that's something for perhaps a different issue another day (if I ever get around to raising it), so please do disregard my earlier comment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants