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

mkRenamedOptionModule doesn't work in submodules #96006

Open
jD91mZM2 opened this issue Aug 22, 2020 · 6 comments · Fixed by #97023 · May be fixed by #360443
Open

mkRenamedOptionModule doesn't work in submodules #96006

jD91mZM2 opened this issue Aug 22, 2020 · 6 comments · Fixed by #97023 · May be fixed by #360443
Assignees
Labels
6.topic: module system About "NixOS" module system internals

Comments

@jD91mZM2
Copy link
Member

jD91mZM2 commented Aug 22, 2020

Describe the bug

I'm working on a home-manager module, where I now want to rename

programs.firefox.profiles.<name>.privacy.enableSettings

to

programs.firefox.profiles.<name>.privacy.settings.enable

The problem is, that list of submodules.

I tried to add

imports = [
  (mkRenamedOptionModule
    [ "privacy" "enableSettings" ]
    [ "privacy" "settings" "enable" ])
];

to the programs.firefox.profiles.<name> module, but that causes the error

warning: Git tree '/home/user/dotfiles' is dirty
warning: Git tree '/home/user/dotfiles' is dirty
error: --- ThrownError ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ nix-build
The option `programs.firefox.profiles.main.warnings' defined in `/home/user/dotfiles/nur-packages/hm-modules/programs/firefox-privacy.nix' does not exist.
(use '--show-trace' to show detailed location information)

There are 135 unread and relevant news items.
Read them by running the command 'home-manager news'.

because each submodule doesn't have a separate warning system.

Obviously doing

imports = [
  (mkRenamedOptionModule
    [ "programs" "firefox" "profiles" "privacy" "enableSettings" ]
    [ "programs" "firefox" "profiles" "privacy" "settings" "enable" ])
];

doesn't work either, because it's missing the profile name.

To Reproduce

Try to rename a suboption in an option of type types.attrsOf (types.submodule ({ config, ... }: {.

The error I got can be seen on my nur-packages' GitLab (on tag nixpkgs-issue-96006 which I created for this occasion).

Expected behavior

Unclear. I guess warnings would be propagated upwards, somehow, using magic?

Metadata
Please run nix-shell -p nix-info --run "nix-info -m" and paste the result.

  • system: "x86_64-linux"
  • host os: Linux 5.7.16, NixOS, 20.09.20200821.4a87da1 (Nightingale)
  • multi-user?: yes
  • sandbox: yes
  • version: nix-env (Nix) 2.4pre20200721_ff314f1
  • channels(user): ""
  • channels(root): "nixos-20.09pre237781.32b46dd897a"
  • nixpkgs: /nix/var/nix/profiles/per-user/root/channels/nixos
@jD91mZM2 jD91mZM2 added 0.kind: bug Something is broken 6.topic: module system About "NixOS" module system internals and removed 0.kind: bug Something is broken labels Aug 22, 2020
@aanderse
Copy link
Member

cc @infinisil

@infinisil
Copy link
Member

I've got a plan for fixing this! Will work on this soon probably :)

@infinisil
Copy link
Member

See PR above ^ :)

@chkno
Copy link
Member

chkno commented Feb 5, 2021

This was marked resolved with #97023 , but #97023 was reverted in #107159

@chkno chkno reopened this Feb 5, 2021
@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Aug 4, 2021
@infinisil
Copy link
Member

Still needed!

@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Aug 4, 2021
@stale stale bot added 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md and removed 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md labels Apr 29, 2022
@ncfavier
Copy link
Member

ncfavier commented May 18, 2022

For the record, this would be useful in https://github.com/nix-community/home-manager/blob/master/modules/misc/xdg-desktop-entries.nix where assertions are currently extracted from the submodules and shoved into top-level assertions.

I wonder if the right solution would be to desugar mkRenamedOptionModule [ "xdg" "desktopEntries" "<name>" "extraConfig" ] into the right thing. That way errors could show the full option path.

This probably overlaps with the more general discussion around submodule syntax, e.g. #146882 (comment)

pluiedev added a commit to pluiedev/home-manager that referenced this issue Jul 18, 2023
It simply cannot be done, if signing continues to be a submodule and
NixOS/nixpkgs#96006 continues to be a problem.
Maroka-chan added a commit to Maroka-chan/VPN-Confinement that referenced this issue Sep 8, 2024
`mkRenamedOptionModule` has been used to throw warnings if the old
option names are used, while still allowing for the old names to work.
A warning is only thrown for 'vpnnamespaces' as the warnings apparently
do not work for submodules. This might have something to do with
NixOS/nixpkgs#96006
@NixOS NixOS deleted a comment from stale bot Nov 30, 2024
@NixOS NixOS deleted a comment from stale bot Nov 30, 2024
@NixOS NixOS deleted a comment from infinisil Nov 30, 2024
roberth added a commit to roberth/nixpkgs that referenced this issue Nov 30, 2024
... i.e. without adding _any_ new strictness

This adds support for warnings in submodules.

Fixes NixOS#96006

To recap, NixOS#97023 didn't make it
because of unexpected infinite recursions. Context:
  - Attaching a warning to an expression can create an otherwise
    unnecessary data dependency, which can lead to infinite mutual
    recursion when a (typically very necessary) dependency exists in
    the other direction.
  - The `warnings` option is largely independent of the evaluation of the
    configuration, because it only reads from it. Nothing except
    `system.build.toplevel` reads from `warnings`.
    Not cyclical, everyone happy.
  - NixOS#97023 created data dependencies at submodule roots, not just `toplevel`

While something like NixOS#97023 is a very general solution that would solve
this `mkRenamedOptionModule` with ease, we don't need it for this particular
problem.
Specifically, in this case, we already have a data dependency where the new
option has a definition based on the old option. So, we can attach the
 warning to the `mkIf` condition of that definition and make it trigger when
the old option is about to be read anyway.

As a side effect of this change, these particular warnings don't appear in
the `warnings` option anymore. Maybe someone used those for testing, but
`mkRenamed`* usages aren't really worth testing, so this is a small price
to pay for support for renames in submodules.
@roberth roberth linked a pull request Nov 30, 2024 that will close this issue
14 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: module system About "NixOS" module system internals
Projects
None yet
5 participants