Reduce warning spam from nix search

[sersorrel]

When you nix search nixpkgs for the first time (or use nix-env -u), you get an enormous number of warnings printed due to renamed or deprecated packages, like:

trace: evaluation warning: cinnamon.bulky was moved to top-level. Please use pkgs.bulky directly.
trace: evaluation warning: cinnamon.cinnamon-common was moved to top-level. Please use pkgs.cinnamon-common directly.
trace: evaluation warning: cinnamon.cinnamon-control-center was moved to top-level. Please use pkgs.cinnamon-control-center directly.
trace: evaluation warning: cinnamon.cinnamon-desktop was moved to top-level. Please use pkgs.cinnamon-desktop directly.
trace: evaluation warning: cinnamon.cinnamon-gsettings-overrides was moved to top-level. Please use pkgs.cinnamon-gsettings-overrides directly.
...etc...

There seems to be no appetite on the Nix side to hide these warnings when doing a search (2024-08-21 Nix team meeting minutes), so let's work around the problem in Nixpkgs instead.

The approach I take is to modify a few attributes of the wrapped derivation so that they emit a warning when evaluated: each of the outputs, as well as drvPath, is wrapped with lib.warn. This is in contrast to the current approach, where evaluating the derivation itself (to do things like "find its name and version", which are necessary for nix search) causes the warning.

Outstanding issues:

• This approach doesn't generalise well to things that aren't derivations. For example, the whole of cudaPackages is currently wrapped in a lib.warn, and writeReferencesToFile (a function) is as well, so these warnings persist after this PR.
• I don't understand enough Nix internals to know why wrapping drvPath works here or if it's the right thing to do. In trace: warning while doing nix-env -u #306276 (comment), @roberth suggested instead wrapping outPath, but for me this didn't seem to ever print the warnings.
• The name wrapWithWarn should probably be changed; other candidates include mkStub, mkWarnDrv, etc.
• lib.derivations is perhaps not the correct place for this helper to live, and it may need tests too – I'm unlikely to have the energy to figure this out on my own, but if someone else wants to put in the work or tell me what to do, feel free.
• Does this negatively impact time taken to evaluate Nixpkgs? Is any impact outweighed by the UX improvement?

Fixes #306276, related to NixOS/nix#10882

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:
  • 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/)
• 24.11 Release Notes (or backporting 23.11 and 24.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.

---

Add a 👍 reaction to pull requests you find important.

[drupol]

Nice, this improves the UX !

[roberth]

I'm ok for this to be top level in lib, but then a slightly more specific name would be nice.
Starting with warn makes it more discoverable in the repl.
How about warnOnInstantiate or warnOutputs?
(well, depends on the outcome of the other comment - maybe skip this one for now)

[sersorrel]

I've switched to warnOnInstantiate at least for now, I agree that starting with warn is nice.

[roberth]

Maybe also wrap override and overrideAttrs if they exist? That doesn't cover all possible overriding functions (passthru.withFoo = finalAttrs.finalPackage.overrideAttrs ...;) but that's quite rare.

Perhaps it'd be better to cover all attributes except like meta, name and type?

Regardless of what works best, it would be good to document how it works.

[sersorrel]

Changed it to wrap everything except those three attributes. Also added a sentence on what exactly it does, and a hint that it's related to nix search.

[hsjobeki]

Since package is not as well defined as a derivation is. And you also call it 'The derivation to wrap.' in your docs.

Suggested change

	msg: pkg:
	msg: drv:

Or did you really mean package? This includes derivations that are built by mkDerivation rather than only builtins.derivation

[roberth]

I think it's good to talk about these things as packages at the language level, because of the package attrset definition, but until packages and derivations are untangled in more places I don't expect anyone to get it right anyway, if there even is such a thing as right (before more untangling, e.g. NixOS/nix#6507, Nixpkgs code for rfc 92 dynamic derivations, etc).
(Also let's not make a fuss about it until that stuff is more relevant in Nixpkgs. I'm already oversharing.)

[sersorrel]

Changed to drv – may as well be consistent.

[hsjobeki]

This reminds me of

https://github.com/NixOS/nix/blob/2e2198fd912a8725eabd96fc4530cc92401dafad/src/libexpr/primops/derivation.nix#L35

instead of adding a new default output here pkgs.outputs or [ "out" ] i would map over the existing list
pkg.all which is an empty list.

Your current line assumes there must be some default output added called "out"

[roberth]

pkg.outputs is the correct source of truth here.

all only has the values, and you can't get the output attribute name from those, or at least not without questionable code with many assumptions.

[sersorrel]

I think this is not relevant if we just wrap (almost) all attributes, rather than specifically targetting outputs.

[Ma27]

May I ask why we're showing aliases in search tooling in the first place? Given that these are removed frequently it seems better to not show them in the first place (unless explicitly enabled perhaps).

Like, please don't consider this comment blocking, this seems like a reasonable (and probably quicker) fix. But to me it seems we have an underlying issue here.

[roberth]

@Ma27 great question. This is a problem at the interface between Nix and Nixpkgs. As far as I know, we currently don't have a good way to make explicit that an attribute or package is an alias. Some ideas:

A meta.isAlias attribute

• would require deep-ish evaluation by the CLI, and probably a new function that sets it
• doesn't work for whole package sets

A special attribute in the package set, e.g. recurseIntoAttrs { __aliases = ["foo"]; foo = pkgs.foo_2; }

• efficient (unless you would derive it from package values, in which case it'd be ~equal)
• more robust, as an alias that fails to eval would not even be traversed by the CLI
• perhaps less "intuitive", not being part of the package, but often that's actually accurate because it's the attribute that's to be avoided, not the package

[hsjobeki]

@sersorrel can you rebase this?
@roberth anything open to pevent merging?

Keeping this open for too long will require continous rebases. And this is a nice improvement already.

[sersorrel]

rebased. now we see only the trace: evaluation warning: CUDA versions older than 12.0 will be removed in Nixpkgs 25.05; see the 24.11 release notes for more information warning repeated nine times, which is 86 fewer warnings than current master.

[roberth]

Found a small simplification, but let's get this in first, so we don't get more conflicts.

[infinisil]

No need to wait for ofborg evaluation anymore, since we have the new GitHub Action-based evaluation now :)

This LGTM! I say let's merge this while it's hot and do the simplification in a new PR

[philiptaron]

Wow, this is great!