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

package overrides using a mini module system #312432

Draft
wants to merge 28 commits into
base: master
Choose a base branch
from

Conversation

Atemu
Copy link
Member

@Atemu Atemu commented May 17, 2024

Description of changes

Putting a package's configuration options into function arguments in order to expose them via override is a well established pattern but has major issues:

  • No discoverablilty/documentation of available options
  • Namespace collisions (calling an option "xorg" would result in callPackage inserting the xorg package set)
  • Odd syntax and scoping limitations when declaring options (? and no ability to use let bindings)
  • No type checking
  • No good error messages
  • No sane way to transparently communicate breaking changes

But we have a well-established solution to all those problems: The module system.

This is a little thought that came to my mind while thinking about RFC 169:
Why couldn't we just use the module system for configuring packages?

The extent to which this is done here is merely the configuration options that were explicitly exposed by the package before which is similar to how modules are used NixOS; explicit options provided by the package maintainer to customise some specific aspect of the package.

The only limitation I currently see is that it is a bit awkward to compose multiple overrides:

# This is fine
(mlterm.override {
  configuration = {
    gui = {
      wayland = false;
      sdl2 = false;
    };
  };
}).override
  # This requires advanced knowledge of the moule system and overrides
  (prev: {
    configuration = {
      imports = [ prev.configuration ];
      gui.sdl2 = true;
    };
  })

This could however be remedied using a dedicated overrideFeatures function.

As for performance:

Edit: Out of date. See below.

Previous:

[atemu@HEPHAISTOS nixpkgs]$ hyperfine --warmup 1 -r 30 'nix-instantiate -A ffmpeg'
Benchmark 1: nix-instantiate -A ffmpeg
  Time (mean ± σ):     716.5 ms ±  13.9 ms    [User: 430.6 ms, System: 143.5 ms]
  Range (min … max):   658.7 ms … 738.2 ms    30 runs
  
[atemu@HEPHAISTOS nixpkgs]$ hyperfine --warmup 10 -r 30 'nix-instantiate -A mlterm'
Benchmark 1: nix-instantiate -A mlterm
  Time (mean ± σ):     739.5 ms ±  33.0 ms    [User: 445.2 ms, System: 151.8 ms]
  Range (min … max):   688.9 ms … 792.6 ms    30 runs

With modules:

[atemu@HEPHAISTOS nixpkgs]$ hyperfine --warmup 1 -r 30 'nix-instantiate -A ffmpeg'
Benchmark 1: nix-instantiate -A ffmpeg
  Time (mean ± σ):     712.8 ms ±  21.4 ms    [User: 428.8 ms, System: 143.0 ms]
  Range (min … max):   657.3 ms … 748.0 ms    30 runs
  
[atemu@HEPHAISTOS nixpkgs]$ hyperfine --warmup 10 -r 30 'nix-instantiate -A mlterm'
Benchmark 1: nix-instantiate -A mlterm
  Time (mean ± σ):     740.9 ms ±  35.9 ms    [User: 446.8 ms, System: 152.2 ms]
  Range (min … max):   687.8 ms … 795.2 ms    30 runs

There might be a slight difference here but none that I could measure using runtime.

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.

@Atemu Atemu requested review from roberth and doronbehar May 17, 2024 12:01
@roberth
Copy link
Member

roberth commented May 17, 2024

My thoughts on this draft:

Unfortunately we can't measure the performance impact by adding only a handful of examples.
The performance impact of the module system is bounded by how much you ask it to do.
That means that we don't have to expect the impact of, say, loading NixOS's massive module list, but something very small for small configs like these.
However, that means that for a realistic picture, we need to apply this solution to all packages in ffmpeg's closure, or a known portion for which we get a statistically significant difference.

With the Nixpkgs architecture team, we've looked into applying the module system more broadly. See e.g.

Reducing the scope of module system usage may alleviate the performance issue somewhat, but then the question is whether this is the most effective allocation of our evaluation performance "budget". (I don't have an answer)
By applying the solution to only part of the problem, I feel like we're almost making the situation worse because of the added complexity of having different solutions for what are actually instances of the same problem - package configuration is just parameters, just like dependencies are, or "environment" things like the platforms. Shouldn't they all be discoverable, mergeable, etc?

Copy link
Contributor

@doronbehar doronbehar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this! Not because I'm hopeful about the performance improvement (in fact I'm skeptic about this detail), but mostly because finally options have a description written with Nix and not in a comment.

@AndersonTorres
Copy link
Member

Obviouslt my question is: how hard will be to teach this to newcomers?

@Atemu
Copy link
Member Author

Atemu commented Jun 2, 2024

Teach what to newcomers? The module system they likely already use in NixOS?

@AndersonTorres
Copy link
Member

Yes, given that Nixpkgs is not tied to NixOS.

@Atemu
Copy link
Member Author

Atemu commented Jun 3, 2024

What part do you expect to be hard to teach to newcomers? From the perspective of a user, they provide an attrset of configuration either way. I don't see how usage of this API would be any harder to understand than the status quo; using a module system requires basically no knowledge of its internal mechanisms.

I'd argue it'd be slightly simpler to use actually because error messages would be a lot easier to understand: "anonymous function at ... called with unexpected argument withFoo" vs. "the option foo does not exist". And that's ignoring the errors you could catch via typecheck instead of incredibly confusing "cannot coerce ... into a string".

@AndersonTorres
Copy link
Member

What part do you expect to be hard to teach to newcomers?

Any part not well documented, to begin with.

https://nixos.org/manual/nixpkgs/stable/#module-system

Note

This chapter is new and not complete yet. For a gentle introduction to the module system, in the context of NixOS, see Writing NixOS Modules in the NixOS manual.

(bold mine)

@Atemu
Copy link
Member Author

Atemu commented Jun 7, 2024

I do not see how the developer documentation for setting up a module system would be relevant to users merely using a module system that someone else already set up.

As per my previous comment, no newcomer needs to know about the module system's internal mechanisms in order to configure something with it.

@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/is-there-a-way-to-get-the-description-of-a-package-rather-than-the-software-it-contains/46742/4

@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/need-for-coorinated-enforced-boolean-switch-arguments-in-nixpkgs/48948/5

@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/a-sad-state-of-nixpkgs/49397/5

@Atemu
Copy link
Member Author

Atemu commented Aug 2, 2024

I just noticed that there has been prominent precedent for this in Nixpkgs for many years now: The structured Linux kernel config drv.

It uses much the same pattern as what I "came up" with. (I didn't actively think of the kernel config but it may have subconsciously influenced me.)

Introduced all the way back in 2018 by @teto: #42838

Slightly less nice but necessary because we pass packages individually
@doronbehar
Copy link
Contributor

I don't think we have that many packages that would want to use this module system, hence I don't consider this price as too high for gradual insertion of this feature into Nixpkgs. I'd like to see this getting moved forward by rebasing etc, and see ofborg's checks will be green. I'd also like to practice afterwards in converting Gnuradio's and mpd's different features to use this module system.

I'd also like to discuss how the docs would look like.

@Atemu
Copy link
Member Author

Atemu commented Sep 14, 2024

I'm confident ofBorgs checks will pass because this evals to the exact same result (modulo order).

@Atemu
Copy link
Member Author

Atemu commented Sep 14, 2024

I've done a few more cleanups and refactors and I think I'm at a point where I'm quite happy with how the ffmpeg derivation turned out and can't immediately think of anything I'd change about the design anymore.

Comment on lines 1047 to 1049
"--target_os=${if stdenv.hostPlatform.isMinGW then "mingw64" else stdenv.hostPlatform.parsed.kernel.name}"
"--arch=${stdenv.hostPlatform.parsed.cpu.name}"
"--pkg-config=${buildPackages.pkg-config.targetPrefix}pkg-config"
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've decided to keep these flags be managed manually; without implying them from the modules eval. Rationale being that it's just a few and they're not really something you'd usually (if ever) want to touch.

Forgot to actually delete it a few commits ago
@Atemu
Copy link
Member Author

Atemu commented Sep 14, 2024

I'm pretty much on the same page w.r.t. eval time increase. For now, this only really makes sense to do in complex packages such as ffmpeg or Linux. A suspected eval time increase by ~1 standard deviation (10^-2 s) isn't a great cost to pay considering a typical NixOS eval takes 2-3 orders of magnitude longer.

While I'd like better documentation, the status quo is that nothing about package customisation is documented outside of code and its comments which is retained. I'd rather wait until we have a proper place and system coordinated for that at which point it should be trivial to plug ffmpeg's options into that.

What I'd like to discuss more is how to handle the override situation as it's all but intuitive or easy and I can't think of an easy solution. The best I can currently come up with is a dedicated override function (i.e. overrideFeatures).

@doronbehar
Copy link
Contributor

What I'd like to discuss more is how to handle the override situation as it's all but intuitive or easy and I can't think of an easy solution. The best I can currently come up with is a dedicated override function (i.e. overrideFeatures).

Which would just be a thin wrapper around .override { configuration = ...; }?

While I'd like better documentation, the status quo is that nothing about package customisation is documented outside of code and its comments which is retained. I'd rather wait until we have a proper place and system coordinated for that at which point it should be trivial to plug ffmpeg's options into that.

It doesn't feel like a big deal to me - we have a place already to document how to use Nix expressions of packages, it's just not automatically generated. I agree we don't have to settle now on how exactly to build the workflow of adding these docs automatically, but I'd like to see that it is at least possible to generate some documentation files automatically from these expressions.

@Atemu
Copy link
Member Author

Atemu commented Sep 15, 2024

Which would just be a thin wrapper around .override { configuration = ...; }?

It'd work as described in the OP.

Though implementing it could be tricky because an override functions' scope is not local to the package.

we have a place already to document how to use Nix expressions of packages

Where? That's news to me.

I'd like to see that it is at least possible to generate some documentation files automatically from these expressions.

This already works for NixOS, I don't see why it wouldn't work here aswell.

In fact, you can already introspect the documentation using a repl/nix-instantiate:

$ nix-instantiate --eval -A ffmpeg.eval.options.features.ass.description
"Control ass support in ffmpeg: (Advanced) SubStation Alpha subtitle rendering."

@Atemu
Copy link
Member Author

Atemu commented Sep 15, 2024

This already works for NixOS, I don't see why it wouldn't work here aswell.

I tried it and it's trivial: pkgs.nixosOptionsDoc { inherit (ffmpeg.eval) options; }

What I noticed is that it puts a literal ‹name› where the submodule's name should be in sentences like The dependencies required to enable ‹name› support.. Not sure what's going on there.

@doronbehar
Copy link
Contributor

Which would just be a thin wrapper around .override { configuration = ...; }?

It'd work as described in the OP.

Though implementing it could be tricky because an override functions' scope is not local to the package.

Hmm OK I see. Now I read in the OP that it is only an issue with multiple overrides? Why would someone need to do that?

@Atemu
Copy link
Member Author

Atemu commented Sep 16, 2024

It's only an issue with composing overrides indeed but you never know what people might need to do with your derivation.

@aanderse
Copy link
Member

multiple overrides are definitely a thing in nixos

consider a module which provides a package option watch requires a specific option - and allows the user to further customize options

i don't recall the concrete example to link at the moment but i if you look you will find it

@Atemu
Copy link
Member Author

Atemu commented Sep 16, 2024

Steam does this for instance:

apply = steam: steam.override (prev: {
extraEnv = (lib.optionalAttrs (cfg.extraCompatPackages != [ ]) {
STEAM_EXTRA_COMPAT_TOOLS_PATHS = extraCompatPaths;
}) // (lib.optionalAttrs cfg.extest.enable {
LD_PRELOAD = "${pkgs.pkgsi686Linux.extest}/lib/libextest.so";
}) // (prev.extraEnv or {});
extraLibraries = pkgs: let
prevLibs = if prev ? extraLibraries then prev.extraLibraries pkgs else [ ];
additionalLibs = with config.hardware.graphics;
if pkgs.stdenv.hostPlatform.is64bit
then [ package ] ++ extraPackages
else [ package32 ] ++ extraPackages32;
in prevLibs ++ additionalLibs;
extraPkgs = p: (cfg.extraPackages ++ lib.optionals (prev ? extraPkgs) (prev.extraPkgs p));
} // lib.optionalAttrs (cfg.gamescopeSession.enable && gamescopeCfg.capSysNice)
{
buildFHSEnv = pkgs.buildFHSEnv.override {
# use the setuid wrapped bubblewrap
bubblewrap = "${config.security.wrapperDir}/..";
};
});

As you can see, it also needs to take great care of overriding in a composable manner but that's only necessary when composing values of arguments; composing top-level arguments themselves without "deeper" values is trivial.

@AndersonTorres AndersonTorres mentioned this pull request Oct 10, 2024
13 tasks
@AndersonTorres AndersonTorres mentioned this pull request Oct 25, 2024
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.status: merge conflict This PR has merge conflicts with the target branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants