Skip to content

aliases: warnOnInstantiate all aliases if config.warnAliases=true#414658

Closed
pbsds wants to merge 2 commits intoNixOS:masterfrom
pbsds:feat-warn-aliases-1749253960
Closed

aliases: warnOnInstantiate all aliases if config.warnAliases=true#414658
pbsds wants to merge 2 commits intoNixOS:masterfrom
pbsds:feat-warn-aliases-1749253960

Conversation

@pbsds
Copy link
Member

@pbsds pbsds commented Jun 7, 2025

The problem with ever dropping aliases from aliases.nix is that downstream users have no idea that certain attributes they rely on have been an alias for years. With warnings however we can expect users to be able to migrate, which in turn enables dropping aliases a lot quicker. With this merged i believe #414656 is very much possible

Tested with nix-env and on my nixos config.

I wanted to make config.warnAliases default to false for normal nixpkgs but true for nixos, but i could not figure out how to set that default without it then ignoring whatever the user configures in their nixos config.

I took extra care to make sure the implementation is performant and that there are no huge footguns in aliases.nix. It is possible for .__aliasWarningAttrName to leak, which is the case for vamp.vampSDK, but i believe this is not a real issue.

Showcase:

The jami-client alias is defined as:

jami-client = jami; # Added 2023-02-10

If you try to use it you wouldn't know it is an alias unless you use the nuclear option allowAliases=false:

$ nix-build . -A jami-client
/nix/store/0dz9vmg7r9wnh30kx95m6vc4s240wgb2-jami-20250523.0
$ nix-build . -A jami-client --arg config '{ allowAliases=false; }'
error: attribute 'jami-client' in selection path 'jami-client' not found

but with this PR you will instead see

$ nix-build . -A jami-client
trace: evaluation warning: 'jami-client' has been renamed to/replaced by 'jami'
/nix/store/0dz9vmg7r9wnh30kx95m6vc4s240wgb2-jami-20250523.0
$ nix-build . -A jami-client --arg config '{ warnAliases=false; }'
/nix/store/0dz9vmg7r9wnh30kx95m6vc4s240wgb2-jami-20250523.0
$ nix-build . -A jami-client --arg config '{ allowAliases=false; }'
error: attribute 'jami-client' in selection path 'jami-client' not found

Here is the stderr output of nix-env --extra-experimental-features no-url-literals --option system x86_64-linux -f ./. -qaP --json --meta:

https://gist.github.com/pbsds/bd6b4211086db4d6707cba98e0fcf5c7

(all of these examples had .outputName and .system excluded from warnOnInstantiate)

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/)
  • Nixpkgs 25.11 Release Notes (or backporting 24.11 and 25.05 Nixpkgs Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
  • NixOS 25.11 Release Notes (or backporting 24.11 and 25.05 NixOS Release notes)
    • (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.

This makes use of 'self' explicit in the let block, and communicates that 'super' should not generally be used in this file.
@github-actions github-actions bot added the 8.has: documentation This PR adds or changes documentation label Jun 7, 2025
@Aleksanaa Aleksanaa requested a review from RossComputerGuy June 7, 2025 06:06
@github-actions github-actions bot added 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-darwin: 1 This PR causes 1 package to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. labels Jun 7, 2025
@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/prs-ready-for-review/3032/5564

@pbsds pbsds requested a review from hsjobeki June 20, 2025 01:49
Copy link
Contributor

@hsjobeki hsjobeki left a comment

Choose a reason for hiding this comment

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

Neat change.
I see two open questions:

  • lib of pkgs should be strictly equal to just importing lib otherwise that could open the box of pandora. cc @roberth @infinisil
  • Should there be a more granular decision for warnAliases ?

else
name: alias: alias;

lib = lib'.extend (
Copy link
Contributor

@hsjobeki hsjobeki Jun 20, 2025

Choose a reason for hiding this comment

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

use of extend was discouraged. cc @roberth.
possibly extend lib without calling extend,
I am a bit worried about this because after this PR:
pkgs.lib != lib

A similar change was made in flake.nix and was highly regreted. We have to maintain that parallelism now and people are confused because about why lib is having different functions sometimes - depending how you import it.

Maybe bind __aliasWarningAttrName = { warnAliases = true; } ? That would also allow perPackage decisions which by default inherit true from the config of pkgs ?
(Just playing with ideas here)

Copy link
Member Author

@pbsds pbsds Jun 25, 2025

Choose a reason for hiding this comment

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

How about renaming lib to _lib in the file with a comment about how only using the "whitelisted" functions from a inherit (_lib) ...; in the let binding is allowed? Then we're free to make custom versions of some of the lib functions.

Maybe bind __aliasWarningAttrName = { warnAliases = true; } ? That would also allow perPackage decisions which by default inherit true from the config of pkgs ?
(Just playing with ideas here)

I don't quite understand what you mean by this nor what it achieves.

@nixpkgs-ci nixpkgs-ci bot added the 2.status: merge conflict This PR has merge conflicts with the target branch label Jul 1, 2025
@Pandapip1
Copy link
Member

Pandapip1 commented Jul 21, 2025

I wanted to make config.warnAliases default to false for normal nixpkgs but true for nixos, but i could not figure out how to set that default without it then ignoring whatever the user configures in their nixos config.

Wouldn't adding simple nixos module

{
  lib,
  ...
}:

{
  config.nixpkgs.config.warnAliases = lib.mkOptionDefault true;
}

work?

@mdaniels5757
Copy link
Member

mdaniels5757 commented Jul 21, 2025

Some aliases (e.g. clangXXStdenv) are intended to be kept indefinitely as alternative names, so I imagine a warning would be undesirable for them. Would this be a problem for those aliases?

Also, some aliases are not for derivations, but rather for functions. (e.g. runCommandNoCC, system). So lib.warnOnInstantiate won't work for them. Never mind, I missed line 219.

mdaniels5757

This comment was marked as resolved.

@roberth
Copy link
Member

roberth commented Jul 21, 2025

This is a very welcome change in behavior, but I think it needs to be a bit bolder, so that it can avoid complicated workarounds.
What if an alias definition looked like this?

let inherit (pkgs) handleAlias;
#...

foo = handleAlias { old = "foo"; new = "bar"; value = bar; release = 2511; };

Then handleAlias is defined in a scope where it can access the required context like config to make a decision, and the call contains all the info in an explicit way, without having to smuggle or override anything.

Speaking of smuggling ;), I've snuck a release attr in there that you could use to control when warnings are activated, perhaps using lib.oldestSupportedReleaseIsAtLeast.

@wolfgangwalther
Copy link
Contributor

What if an alias definition looked like this?
[...]

Another alias related draft PR can be found at #442066, where I intend to introduce "structured aliases" to solve some of these same problems (eventually).

@pbsds
Copy link
Member Author

pbsds commented Sep 14, 2025

I'm much more interested in mass-migrating the current aliases to that system once it is ready

@pbsds pbsds closed this Sep 14, 2025
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 8.has: documentation This PR adds or changes documentation 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-darwin: 1 This PR causes 1 package to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants