Move renames to their appropriate places #61570

infinisil · 2019-05-15T22:41:37Z

Having a centralized list of renamed/changed/removed options is bad
because

It breaks disabledModules, because disabling a module that has a
renamed option wouldn't disable the rename, but of course if the module
is disabled, the rename doesn't actually work because the option it
renames to doesn't exist anymore.
Modules don't see their renames right next to them, and people need to
edit this file instead of declaring renames right where it makes sense.

This is only a partial PR. I can update this PR with the rest if nobody has any complaints.

Motivation for this change

Things done

I have not tested this.

chreekat

I think this is a great idea. The only way it could be better is if documentation for these renamings is centralized somewhere (but I don't know where or how, so this is just aspirational commentary).

aanderse · 2019-12-03T02:07:30Z

@infinisil does anyone disagree this PR is a good idea? Any obstacles to merging, other than finishing it?

infinisil · 2019-12-03T17:35:20Z

Nope, finishing is really the only thing. I have more time from now on so maybe I can get to this soon

aanderse · 2019-12-03T18:10:13Z

I would love that. Every once in a while the issue this PR resolves bites me.

A centralized list for these renames is not good because: - It breaks disabledModules for modules that have a rename defined - Adding/removing renames for a module means having to find them in the central file - Merge conflicts due to multiple people editing the central file

infinisil · 2019-12-10T02:05:28Z

Rebased on master and did it for all options.

Difference of nix-build nixos/release.nix -A options before and after this commit:

diff <(cat result/share/doc/nixos/options.json | gron) <(cat result2/share/doc/nixos/options.json | gron)          
colordiff 1.0.18 (http://www.colordiff.org/)
(C)2002-2017 Dave Ewart, davee@sungate.co.uk

13575c13575
< json["security.pam.yubico.mode"].description = "Mode of operation.\n\nUse \"client\" for online validation with a YubiKey validation service such as\nthe YubiCloud.\n\nUse \"challenge-response\" for offline validation using YubiKeys with HMAC-SHA-1\nChallenge-Response configurations. See the man-page ykpamcfg(1) for further\ndetails on how to configure offline Challenge-Response validation. \n\nMore information can be found <link\nxlink:href=\"https://developers.yubico.com/yubico-pam/Authentication_Using_Challenge-Response.html\">here</link>.\n";
---
> json["security.pam.yubico.mode"].description = "Mode of operation.\n\nUse \"client\" for online validation with a YubiKey validation service such as\nthe YubiCloud.\n\nUse \"challenge-response\" for offline validation using YubiKeys with HMAC-SHA-1\nChallenge-Response configurations. See the man-page ykpamcfg(1) for further\ndetails on how to configure offline Challenge-Response validation.\n\nMore information can be found <link\nxlink:href=\"https://developers.yubico.com/yubico-pam/Authentication_Using_Challenge-Response.html\">here</link>.\n";
61145c61145
< json["services.openssh.knownHosts"].declarations[0] = "nixos/modules/rename.nix";
---
> json["services.openssh.knownHosts"].declarations[0] = "nixos/modules/services/networking/ssh/sshd.nix";
78567c78567
< json["services.sshd.enable"].declarations[0] = "nixos/modules/rename.nix";
---
> json["services.sshd.enable"].declarations[0] = "nixos/modules/services/networking/ssh/sshd.nix";
104960c104960
< json["users.extraGroups"].declarations[0] = "nixos/modules/rename.nix";
---
> json["users.extraGroups"].declarations[0] = "nixos/modules/config/users-groups.nix";
104969c104969
< json["users.extraUsers"].declarations[0] = "nixos/modules/rename.nix";
---
> json["users.extraUsers"].declarations[0] = "nixos/modules/config/users-groups.nix";

(the first is just a whitespace from editorconfig I'd guess) With this I can be sure that I didn't miss anything.

I also added an explanatory message to rename.nix that people shouldn't put stuff there anymore.

aanderse

Thank you very much @infinisil! 🎉

infinisil · 2019-12-10T02:26:45Z

Oh, well I guess renamed options won't show up in the manual. So here's a super fancy way to verify I didn't miss anything:

git log -p -1 | rg '[+-].*mk.*Module(.*)' -or '$1' | sort | uniq -u

Look at the changes of the last commit that do a mk.*Module, sort them, only output unique lines -> No lines get returned because all come in pairs adding/removing

infinisil · 2019-12-10T02:29:45Z

One last thing before merging: I want to check out the evaluation time difference. If that's not a problem I'll merge

infinisil · 2019-12-10T03:31:33Z

Done that, ran NIX_SHOW_STATS=1 nix-instantiate nixos/release-combined.nix -A tested two times before and after the commit, results are

before1

{
  "cpuTime": 221.438,
  "envs": {
    "number": 299007965,
    "elements": 380066660,
    "bytes": 7824660720
  },
  "list": {
    "elements": 297212376,
    "bytes": 2377699008,
    "concats": 8066450
  },
  "values": {
    "number": 533774798,
    "bytes": 12810595152
  },
  "symbols": {
    "number": 166745,
    "bytes": 6623422
  },
  "sets": {
    "number": 60350420,
    "bytes": 15206771776,
    "elements": 613498684
  },
  "sizes": {
    "Env": 16,
    "Value": 24,
    "Bindings": 8,
    "Attr": 24
  },
  "nrOpUpdates": 21191561,
  "nrOpUpdateValuesCopied": 435964457,
  "nrThunks": 336088985,
  "nrAvoided": 361443150,
  "nrLookups": 164188255,
  "nrPrimOpCalls": 149614461,
  "nrFunctionCalls": 266629750,
  "gc": {
    "heapSize": 12281708544,
    "totalBytes": 46943907840
  }
}

before2

{
  "cpuTime": 231.007,
  "envs": {
    "number": 299008253,
    "elements": 380067020,
    "bytes": 7824668208
  },
  "list": {
    "elements": 297212376,
    "bytes": 2377699008,
    "concats": 8066450
  },
  "values": {
    "number": 533775620,
    "bytes": 12810614880
  },
  "symbols": {
    "number": 166746,
    "bytes": 6623426
  },
  "sets": {
    "number": 60350420,
    "bytes": 15206777104,
    "elements": 613498906
  },
  "sizes": {
    "Env": 16,
    "Value": 24,
    "Bindings": 8,
    "Attr": 24
  },
  "nrOpUpdates": 21191561,
  "nrOpUpdateValuesCopied": 435964457,
  "nrThunks": 336089249,
  "nrAvoided": 361443990,
  "nrLookups": 164188375,
  "nrPrimOpCalls": 149614917,
  "nrFunctionCalls": 266629942,
  "gc": {
    "heapSize": 11627298816,
    "totalBytes": 46943922224
  }
}

after1

{
  "cpuTime": 225.745,
  "envs": {
    "number": 298980818,
    "elements": 380032705,
    "bytes": 7823954728
  },
  "list": {
    "elements": 297148656,
    "bytes": 2377189248,
    "concats": 8065678
  },
  "values": {
    "number": 533741824,
    "bytes": 12809803776
  },
  "symbols": {
    "number": 166742,
    "bytes": 6623401
  },
  "sets": {
    "number": 60343148,
    "bytes": 15198102256,
    "elements": 613139878
  },
  "sizes": {
    "Env": 16,
    "Value": 24,
    "Bindings": 8,
    "Attr": 24
  },
  "nrOpUpdates": 21189442,
  "nrOpUpdateValuesCopied": 435598183,
  "nrThunks": 336070225,
  "nrAvoided": 361421402,
  "nrLookups": 164189015,
  "nrPrimOpCalls": 149605563,
  "nrFunctionCalls": 266605507,
  "gc": {
    "heapSize": 11747434496,
    "totalBytes": 46932711536
  }
}

after2

{
  "cpuTime": 221.895,
  "envs": {
    "number": 298981106,
    "elements": 380033065,
    "bytes": 7823962216
  },
  "list": {
    "elements": 297148656,
    "bytes": 2377189248,
    "concats": 8065678
  },
  "values": {
    "number": 533742646,
    "bytes": 12809823504
  },
  "symbols": {
    "number": 166743,
    "bytes": 6623405
  },
  "sets": {
    "number": 60343148,
    "bytes": 15198107584,
    "elements": 613140100
  },
  "sizes": {
    "Env": 16,
    "Value": 24,
    "Bindings": 8,
    "Attr": 24
  },
  "nrOpUpdates": 21189442,
  "nrOpUpdateValuesCopied": 435598183,
  "nrThunks": 336070489,
  "nrAvoided": 361422242,
  "nrLookups": 164189135,
  "nrPrimOpCalls": 149606019,
  "nrFunctionCalls": 266605699,
  "gc": {
    "heapSize": 11828625408,
    "totalBytes": 46932808208
  }
}

In short: No significant difference.

infinisil · 2019-12-23T15:39:37Z

It breaks disabledModules, because disabling a module that has a
renamed option wouldn't disable the rename, but of course if the module
is disabled, the rename doesn't actually work because the option it
renames to doesn't exist anymore.

Turns out this is wrong, even with this change it doesn't work. A disabledModules currently only disables that very module, not also all the imports it does. However I'm looking into changing the module system to do that.

infinisil · 2020-01-03T19:24:46Z

I opened #76857 which fixes this

With this change, disabledModules applies recursively, meaning if you have a module "foo.nix" with imports = [ ./bar.nix ]; then setting disabledModules = [ "foo.nix" ]; will disable both "foo.nix" and "bar.nix", whereas previously only "foo.nix" would be disabled. This change along with NixOS#61570 allows modules to be fully disabled even when they have some `mkRenamedOption` imports.

infinisil · 2020-01-09T17:23:55Z

#76857 is now merged, so along with this PR, it's now possible to disable modules even when they have renames :)

With this change, disabledModules applies recursively, meaning if you have a module "foo.nix" with imports = [ ./bar.nix ]; then setting disabledModules = [ "foo.nix" ]; will disable both "foo.nix" and "bar.nix", whereas previously only "foo.nix" would be disabled. This change along with NixOS#61570 allows modules to be fully disabled even when they have some `mkRenamedOption` imports. (cherry picked from commit de5f73d)

Now possible with NixOS/nixpkgs#61570 and NixOS/nixpkgs#76857

This was a mistake in #61570, this does not belong to prometheus

ofborg bot added 6.topic: nixos 8.has: module (update) 10.rebuild-darwin: 0 10.rebuild-linux: 0 labels May 16, 2019

chreekat approved these changes May 16, 2019

View reviewed changes

This was referenced Sep 10, 2019

Multiple buildkite agent support #68378

Closed

Fix services.redshift.provider backwards-incompatibility #68852

Merged

infinisil mentioned this pull request Oct 20, 2019

nixos/httpd: remove deprecated extraSubservices option #71067

Merged

10 tasks

infinisil force-pushed the move-renames branch from 6f6b221 to 4ee3e8b Compare December 10, 2019 01:54

infinisil requested review from peti and WilliButz as code owners December 10, 2019 01:54

infinisil changed the title ~~[Partial] Move renames to their appropriate places~~ Move renames to their appropriate places Dec 10, 2019

ofborg bot added 6.topic: printing 6.topic: qt/kde 6.topic: xfce The Xfce Desktop Environment 8.has: documentation labels Dec 10, 2019

infinisil requested a review from aanderse December 10, 2019 02:06

aanderse approved these changes Dec 10, 2019

View reviewed changes

infinisil merged commit ae94e89 into NixOS:master Dec 10, 2019

infinisil deleted the move-renames branch December 10, 2019 03:31

infinisil mentioned this pull request Jan 3, 2020

Apply disabledModules recursively #76857

Merged

3 tasks

infinisil added a commit to infinisil/system that referenced this pull request Mar 28, 2020

disable nixpkgs' murmur module, replacing it with my own

19a252a

Now possible with NixOS/nixpkgs#61570 and NixOS/nixpkgs#76857

infinisil mentioned this pull request Jun 18, 2020

nixos/fontconfig: Move deprecated ultimate removals to relevant module #91065

Merged

2 tasks

flokli pushed a commit that referenced this pull request Jun 18, 2020

nixos/fontconfig: Move deprecated ultimate removals to relevant module

78453e6

This was a mistake in #61570, this does not belong to prometheus

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move renames to their appropriate places #61570

Move renames to their appropriate places #61570

infinisil commented May 15, 2019

chreekat left a comment

aanderse commented Dec 3, 2019

infinisil commented Dec 3, 2019

aanderse commented Dec 3, 2019

infinisil commented Dec 10, 2019

aanderse left a comment

infinisil commented Dec 10, 2019

infinisil commented Dec 10, 2019

infinisil commented Dec 10, 2019

infinisil commented Dec 23, 2019

infinisil commented Jan 3, 2020

infinisil commented Jan 9, 2020

Move renames to their appropriate places #61570

Move renames to their appropriate places #61570

Conversation

infinisil commented May 15, 2019

Motivation for this change

Things done

chreekat left a comment

Choose a reason for hiding this comment

aanderse commented Dec 3, 2019

infinisil commented Dec 3, 2019

aanderse commented Dec 3, 2019

infinisil commented Dec 10, 2019

aanderse left a comment

Choose a reason for hiding this comment

infinisil commented Dec 10, 2019

infinisil commented Dec 10, 2019

infinisil commented Dec 10, 2019

infinisil commented Dec 23, 2019

infinisil commented Jan 3, 2020

infinisil commented Jan 9, 2020