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

Move renames to their appropriate places #61570

Merged
merged 1 commit into from Dec 10, 2019
Merged

Conversation

@Infinisil
Copy link
Member

@Infinisil Infinisil commented May 15, 2019

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.

  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • 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 nix-review --run "nix-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)
  • Assured whether relevant documentation is up to date
  • Fits CONTRIBUTING.md.

Copy link
Contributor

@chreekat chreekat left a comment

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
Copy link
Contributor

@aanderse aanderse commented Dec 3, 2019

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

@Infinisil
Copy link
Member Author

@Infinisil Infinisil commented Dec 3, 2019

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

@aanderse
Copy link
Contributor

@aanderse aanderse commented Dec 3, 2019

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 Infinisil force-pushed the Infinisil:move-renames branch from 6f6b221 to 4ee3e8b Dec 10, 2019
@Infinisil Infinisil requested review from peti and WilliButz as code owners Dec 10, 2019
@Infinisil Infinisil changed the title [Partial] Move renames to their appropriate places Move renames to their appropriate places Dec 10, 2019
@Infinisil
Copy link
Member Author

@Infinisil Infinisil commented Dec 10, 2019

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.

@Infinisil Infinisil requested a review from aanderse Dec 10, 2019
Copy link
Contributor

@aanderse aanderse left a comment

Thank you very much @Infinisil! 🎉

@Infinisil
Copy link
Member Author

@Infinisil Infinisil commented Dec 10, 2019

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
Copy link
Member Author

@Infinisil Infinisil commented Dec 10, 2019

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

@Infinisil
Copy link
Member Author

@Infinisil Infinisil commented Dec 10, 2019

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 Infinisil merged commit ae94e89 into NixOS:master Dec 10, 2019
12 checks passed
12 checks passed
Evaluation Performance Report Evaluator Performance Report
Details
grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-darwin nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A darwin-tested
Details
grahamcofborg-eval-nixos nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release-combined.nix -A tested
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A manual
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A options
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A manual
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A tarball
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A unstable
Details
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
grahamcofborg-eval-package-list-no-aliases nix-env -qa --json --file . --arg config { allowAliases = false; }
Details
@Infinisil Infinisil deleted the Infinisil:move-renames branch Dec 10, 2019
@Infinisil
Copy link
Member Author

@Infinisil Infinisil commented Dec 23, 2019

  • 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 added a commit to Infinisil/nixpkgs that referenced this pull request Jan 3, 2020
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 Infinisil mentioned this pull request Jan 3, 2020
3 of 3 tasks complete
Infinisil added a commit to Infinisil/nixpkgs that referenced this pull request Jan 3, 2020
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
Copy link
Member Author

@Infinisil Infinisil commented Jan 3, 2020

I opened #76857 which fixes this

Infinisil added a commit to Infinisil/nixpkgs that referenced this pull request Jan 5, 2020
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 added a commit to Infinisil/nixpkgs that referenced this pull request Jan 5, 2020
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 added a commit to Infinisil/nixpkgs that referenced this pull request Jan 7, 2020
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 added a commit to Infinisil/nixpkgs that referenced this pull request Jan 8, 2020
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 added a commit to Infinisil/nixpkgs that referenced this pull request Jan 9, 2020
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 added a commit to Infinisil/nixpkgs that referenced this pull request Jan 9, 2020
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
Copy link
Member Author

@Infinisil Infinisil commented Jan 9, 2020

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

dtzWill added a commit to dtzWill/nixpkgs that referenced this pull request Jan 19, 2020
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)
Infinisil added a commit to Infinisil/system that referenced this pull request Mar 28, 2020
@WilliButz

This comment has been minimized.

@Infinisil
I'm a little late to the party but I just noticed the fontconfig options and was wondering if this was a mistake or actually somehow needed in the exporters module? :)

This comment has been minimized.

Copy link
Member Author

@Infinisil Infinisil replied Jun 18, 2020

Oh yeah that looks like a mistake! -> #91065

flokli pushed a commit that referenced this pull request Jun 18, 2020
This was a mistake in #61570, this
does not belong to prometheus
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.