nixgl: forward override calls to wrapped package

[Smona]

Description

Currently, wrapping packages with nixGL.wrap will break any options in home manager modules that use override to modify the package.

This change overrides override on the wrapper package, such that it will instead forward the call to the internal package, and return a wrapped version of the result with the same arguments.

Tested via the programs.chromium.commandLineArgs option mentioned in #6016, with several other wrapped packages in my config to regression test.

Fixes #6016.

Checklist

• Change is backwards compatible.

• Code formatted with ./format.

• Code tested through nix-shell --pure tests -A run.all or nix develop --ignore-environment .#all using Flakes.

• Test cases updated/added. See example.

• Commit messages are formatted like

  {component}: {description}
  
  {long description}
  

  See CONTRIBUTING for more information and recent commit messages for examples.

• If this PR adds a new module

  • Added myself as module maintainer. See example.

Maintainer CC

[exzombie]

I'm a bit lost as to how all of this works. First, I'm confused as to how using buildCommand interacts with the old (i.e., overridden) package. I thought the buildCommand in the new package applies to the new package; why would it have any effect on the old package (e.g., chromium)? I mean, according to the docs, using buildCommand with mkDerivation disables other build phases, so, if it were able to affect the old package, it wouldn't get built in the first place. What am I misunderstanding? How does #6016 come about?

Second, you are replacing the override function, and I don't understand where it enters the picture and how it helps with #6016. I think this should be explained in a comment in the code for future readers because it isn't obvious. Looking at the code without knowing what issue it is solving, I'd be wondering "why have this here? I'm sure if someone wants to override a package argument, they would just use config.lib.nixGL.wrap (thePackage.override { arg1=other; }), no?" In other words, I would never expect anyone to want to override the wrapper package. I don't understand where this occurs.

[Smona]

buildCommand in the new package does apply to the new package, and in a way that's the problem.

Basically, the chromium module implements the commandLineArgs option by calling override on the chromium package to forward that arg into it. The chromium derivation accepts this argument, and uses it in its own buildCommandto adjust the wrapper it generates. but the nixGL wrapper derivation overrides this buildCommand, so when it receives this commandLineArgs argument, it doesn't know what to do with it, and the argument is ignored. What we want is for this argument to make it into the wrapped package, which knows what to do with it, and have the results baked into the outputs that we are linking into the nixGL wrapper (in this case, chromium wrapper scripts which include the specified command line flags). It does get very hard to follow the multiple layers of wrapping 😅

My goal with this wrapper is to have the resulting package behave identically to the original one, to avoid bugs like this when someone passes a wrapped package into other home manager modules. Otherwise, a user passing a wrapped package into a module would have to figure out why other options aren't working, and then essentially re-implement the module options by directly modifying the package they are wrapping, which is a bad user experience. wrapping a package with nixGL should not break other module options someone is already using, as it did in #6016.

Forwarding override calls from the nixGL wrapper to the inner package, and then returning a wrapped version of the result, seems like a clean way to make override calls on the wrapper behave just like override calls on the unmodified derivation, and fix any module options which similarly rely on calling override on packages. We could instead have the nixGL wrapper extend the buildCommand rather than replacing it, but I'm pretty sure this would trigger rebuilding of any wrapped package from scratch, which isn't feasible (i haven't actually tested this though).

I figured this change would be a bit unclear, which is why a left a comment, but I'll admit it was hard to come up with a succinct explanation of what the new override function is for, and what it's doing. I figured anyone who's curious could blame the change, and then find this PR and the related issue, which should make it clear what the override override is for. But you've shown you are quite good at writing documentation, so if you have an idea of how to better explain what's going on here succinctly in a comment i'm all ears :)

[exzombie]

Ah, I see, thanks for the explanation. I totally missed the fact that many HM modules will add their own wrapper, which is obvious in retrospect, although I wonder how common the approach taken by Chromium actually is. With this in mind, it seems to me that the approach you have taken here is appropriate. I'll try to come up with an explanation to add to the code; sure, blaming would find it, but that's more work than someone reading the code casually is typically willing to do :) .

Thanks for clearing this up. This issue is something that is important to keep in mind whenever you make a package wrapper, and I haven't realized it before.

[Smona]

@rycee this should be ready for review!

[Smona]

Looks like the build failures are the same as #6039 and unrelated to this change.

[bamhm182]

The build failures seem to have sprung up as a result of something having changed in nixpkgs-unstable recently. The referenced podman PR had similar errors on a 2024-10-29 flake, but the issues are no longer there on 2024-11-02 and 2024-11-05 flakes. Not really sure what to make of it, but posting to confirm it is unrelated to your changes.

EDIT: I set the nixpkgs-unstable flake to one from 2024-10-02 and it's passing as expected. Don't have time right now to try and track down a specific rev that caused the tests to start failing.