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

Jump to bottom

lib/modules: mkRemovedOptionModule: warning instead of error #101408

Closed
wants to merge 1 commit into from
Closed

lib/modules: mkRemovedOptionModule: warning instead of error #101408

wants to merge 1 commit into from

Conversation

gebner
Copy link
Member

@gebner gebner commented Oct 22, 2020

Motivation for this change

I want my nixos configuration to work on both unstable and stable. The option services.dbus.socketActivated has been removed because it is now always enabled on unstable. I still need to set on stable because it defaults to false there.

The comment in mkRemovedOptionModule says it should produce a warning and not a hard error. This PR makes it so.

cc @worldofpeace

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@gebner gebner mentioned this pull request Oct 22, 2020
Merged
@rycee
Copy link
Member

rycee commented Oct 22, 2020

The assertion seems intentionally added by @globin in b08b0bc. An alternative would be to do a custom solution in dbus.nix?

@rycee rycee mentioned this pull request Oct 22, 2020
Merged
@gebner
Copy link
Member Author

gebner commented Oct 22, 2020

I'm not sure I agree with @globin here. There are lots of ways an updated module can break your configuration. See for example the dbus change here which might break if you start dbus manually (as you had to before, if you had an uncommon configuration).

Is there maybe an easy way to only set an option if it is defined? This would at least help me with my current issue.

@rycee
Copy link
Member

rycee commented Oct 22, 2020

See the above PR for the custom solution alternative. Dunno which one is best but I imagine @globin had a good reason for changing the behavior.

@gebner
Copy link
Member Author

gebner commented Oct 22, 2020

For reference, this is the PR introducing the hard error: #69419

There wasn't much discussion about the change.

@infinisil
Copy link
Member

This works to only set options if they exist:

{ options, ... }: {
  config = optionalAttrs (options ? some.option) {
    some.option = "foo";
  };
}

Also related, my #97023 will allow assertions to be changed to warnings or silence them completely.

@gebner
Copy link
Member Author

gebner commented Oct 25, 2020

Let's get #97023 merged then.

@gebner gebner closed this Oct 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@edolstra edolstra Awaiting requested review from edolstra

@infinisil infinisil Awaiting requested review from infinisil infinisil is a code owner

@nbp nbp Awaiting requested review from nbp

Assignees
No one assigned
Labels
10.rebuild-darwin: 0 10.rebuild-linux: 0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@gebner @rycee @infinisil