libwebp: set meta.pkgConfigModules

[sternenseemann]

Using the package from the pkgs fixpoint instead of finalAttrs.finalPackage to mirror the other tests which also (indirectly) use pkgs.libwebp. Slight disadvantage of this is that it's not possible to test overridden variants of libwebp this way.

Things done

• Built on platform:
  • x86_64-linux
  • aarch64-linux
  • x86_64-darwin
  • aarch64-darwin
• Tested, as applicable:
  • NixOS tests in nixos/tests.
  • Package tests at passthru.tests.
  • Tests in lib/tests or pkgs/test for functions and "core" functionality.
• Ran nixpkgs-review on this PR. See nixpkgs-review usage.
• Tested basic functionality of all binary files, usually in ./result/bin/.
• Nixpkgs Release Notes
  • Package update: when the change is major or breaking.
• NixOS Release Notes
  • Module addition: when adding a new NixOS module.
  • Module update: when the change is significant.
• Fits CONTRIBUTING.md, pkgs/README.md, maintainers/README.md and other READMEs.

---

Add a 👍 reaction to pull requests you find important.

[wolfgangwalther]

I'd argue that, even if the other tests are doing this effectively, we should still do it right here and use finalAttrs.finalPackage. Especially because there are separate variants of this test depending on libwebpmuxSupport. But I'd also say the same thing about the other PR.

It would technically be out of scope for this PR, but a welcome improvement, to override the other three packages in passthru.tests to be passed as libwebp = finalAttrs.finalPackage, too.

[sternenseemann]

I would like to avoid refactoring the expression since adding finalAttrs introduces even more bikeshedding questions which I don't really have the patience for. The upside is not really that big as well since it is relatively unlikely the test would be executed in practice since no attribute actually exposes libwebp.override { libwebpmuxSupport = false; }. I had the following downstream, but didn't bother committing it:

diff --git a/pkgs/by-name/li/libwebp/package.nix b/pkgs/by-name/li/libwebp/package.nix
index b814308b8ae0..933c0c8c4ca9 100644
--- a/pkgs/by-name/li/libwebp/package.nix
+++ b/pkgs/by-name/li/libwebp/package.nix
@@ -82,6 +82,14 @@ stdenv.mkDerivation rec {
     inherit (python3.pkgs) pillow imread;
     haskell-webp = haskellPackages.webp;
     pkg-config = testers.hasPkgConfigModules { package = libwebp; };
+    minimal-libwebp = libwebp.override {
+      pngSupport = false;
+      jpegSupport = false;
+      tiffSupport = false;
+      gifSupport = false;
+      libwebpmuxSupport = false;
+    };
+    minimal-pkg-config = testers.hasPkgConfigModules { package = passthru.tests.minimal-libwebp; };
   };
 
   meta = with lib; {

[wolfgangwalther]

I would like to avoid refactoring the expression since adding finalAttrs introduces even more bikeshedding questions which I don't really have the patience for.

I fail to see the potential for bike-shedding here. There is a single version reference that needs to be adjusted to finalAttrs, which is really uncontroversial. Not adjusting the other tests would be totally fine, imho, as I said, this would be out of scope.

In any case my comment is not blocking, your PR is not technically wrong.

[sternenseemann]

Well the question is precisely whether it should or should not use finalAttrs.

[wolfgangwalther]

I see. If that is really a question, then yes :)

For me, this is pretty clear: I don't think a big-scale "everything is finalAttrs" migration makes sense, yet. But for packages where this can sensibly be used, it's entirely fine to do it. And everything that uses finalAttrs.finalPackage is by definition such a case. finalAttrs.finalPackage is always better then pulling in <self> via arguments.

[sternenseemann]

See #310373. I don't agree really agree with the argument, though.

[roberth]

I've been writing finalAttrs.version for simplicity so I don't need rec, but if rec is preferable for that, we can do both: finalAttrs.finalPackage and rec's simple version reference.
Subjectively let is nicer than rec, but let's get it correct first.

[sternenseemann]

@roberth This was my takeaway from the raised concern as well, however there have been mass refactors both removing both simultaneous use of rec and finalAttrs (#414239) and I've seen the sentiment expressed that let … in … for pname, version etc. should be removed as well (#336172 (comment)).
I get the impression that these are treewide refactors for treewide refactor's sake to some extent and haven't engaged further.

I personally think finalAttrs + rec is maybe a little confusing, so I think let floating is the best solution.