nixos/modules: Add declarationPositions#249243
Conversation
|
One somewhat annoying result of this is that it will give quite useless locations for mkRenamedOptionsModule (pointing to the definition of lib.setAttrByPath). However this is really unavoidable, since that is the proximate source of that attrset. I haven't found any other bugs. I would also consider this one a non-bug, since as far as I know, all the users here ignore such options anyway. |
|
OK, I found another bug: cleverly defined options such as I think the most useful thing to do is to just use the old file name as a fallback and either: |
infinisil
left a comment
There was a problem hiding this comment.
This is nice!
One somewhat annoying result of this is that it will give quite useless locations for mkRenamedOptionsModule (pointing to the definition of lib.setAttrByPath)
OK, I found another bug: cleverly defined options such as programs.firefox.nativeMessagingHosts.ff2mpvand services.thanos.query.query.max-concurrent from dynamically generated modules have a null location.
I think it would be great to have a mkOption argument to set the option location manually for such cases. It could later be enforced by the manual build for all options to have a location. But this could be a future PR.
I think the most useful thing to do is to just use the old file name as a fallback and either:
{file = "blah.nix"; line = 1; column = 1;}, or{file = "blah.nix"; line = null; column = null;}. Thoughts on which one is a better idea?
The latter definitely, otherwise it's not distinguishable whether the location was determined or not.
In addition, this could benefit from a bit more testing :)
997f76f to
ed08e82
Compare
|
alright, applied all the suggestions. Thanks for the review! |
+1 Something along the lines of
It could be set explicitly in I'd expect that to cover almost everything. I don't find enforcement of positions particularly important. Might even be unproductive for module authors, if it's even relevant at all. |
ed08e82 to
c745c0b
Compare
c745c0b to
26a5174
Compare
roberth
left a comment
There was a problem hiding this comment.
A design question and some test suggestions.
Definitely on the right track though!
lib/modules.nix
Outdated
There was a problem hiding this comment.
Should we fall back to the position of the parent, somewhat similar to what we do with _file in imports? (where the imports relation corresponds to the option nesting relation, and the partiality comes from anonymous modules or generated options attrs, resp.)
There was a problem hiding this comment.
Not necessarily, since we get garbage for a lot of options anyway, so this wouldn't improve that much I don't think.
dev/nixpkgs2 » rg attrsets.nix ../nixpkgs/opts-tags | wc -l
957
dev/nixpkgs2 » cat ../nixpkgs/opts-tags | wc -l
11201
There was a problem hiding this comment.
Depends on the question in the other comment. We could fall back to the parent's position.
26a5174 to
c10e207
Compare
What it does: line and column level *declaration* position information:
$ nix repl .
nix-repl> :p nixosConfigurations.micro.options.environment.systemPackages.declarationPositions
[ { column = 7; file = "/nix/store/24aj3k7fgqv3ly7qkbf98qvphasrw9nb-source/nixos/modules/config/system-path.nix"; line = 63; } ]
Use cases:
- ctags over NixOS options, as will be presented at NixCon 2023 ;)
- improving the documentation pages to go to the exact line of the
declarations.
Related work:
- NixOS#65024
This one does it for all *definitions* rather than declarations, and
it was not followed through with due to performance worries.
- NixOS#208173
The basis for this change. This change is just a rebase of that one.
I split it out to add the capability before adding users of it, in
order to simplify review. However, the ctags script in there is a
sample user of this feature.
Benchmarks: conducted by evaluating my own reasonably complex NixOS
configuration with the command:
`hyperfine -S none -w 1 -- "nix eval .#nixosConfigurations.snowflake.config.system.build.toplevel.outPath"`
```
Benchmark 1: nix eval .#nixosConfigurations.snowflake.config.system.build.toplevel.outPath
Time (mean ± σ): 8.971 s ± 0.254 s [User: 5.872 s, System: 1.388 s]
Range (min … max): 8.574 s … 9.327 s 10 runs
Benchmark 1: nix eval .#nixosConfigurations.snowflake.config.system.build.toplevel.outPath
Time (mean ± σ): 8.766 s ± 0.160 s [User: 5.873 s, System: 1.346 s]
Range (min … max): 8.496 s … 9.033 s 10 runs
```
Summary of results: it seems to be in the noise, this does not cause any
visible regression in times.
c10e207 to
a1d3882
Compare
|
@roberth sorry I missed you at NixCon, I was running around with my hair metaphorically on fire on the hackday. I think this may be mergeable, or at least, I am not sure how to improve it further. |
Kinda same. No need to apologize! |
This is rather a strawman of a PR to try to figure out the best spot to
put this code and the documentation of how to use it. I don't really
know.
Sample content:
```
!_TAG_FILE_SORTED 1 1
!_TAG_FILE_ENCODING utf-8 1
_module.args /home/jade/dev/nixpkgs/lib/modules.nix 122
_module.check /home/jade/dev/nixpkgs/lib/modules.nix 186
_module.freeformType /home/jade/dev/nixpkgs/lib/modules.nix 193
_module.specialArgs /home/jade/dev/nixpkgs/lib/modules.nix 209
appstream.enable /home/jade/dev/nixpkgs/nixos/modules/config/appstream.nix 6
assertions /home/jade/dev/nixpkgs/nixos/modules/misc/assertions.nix 9
boot.binfmt.emulatedSystems /home/jade/dev/nixpkgs/nixos/modules/system/boot/binfmt.nix 272
```
Sample usage (can be used in parallel with `nix-doc tags`):
```
nix build -f nixos/release.nix optionsCtags -o opts-tags
(in vim): :set tags+=opts-tags
:tj boot.binfmt.emulatedSystems
```
This PR depends on and contains NixOS#249243. Marked as draft till that's
worked out. This PR pair replaces NixOS#208173.
This is rather a strawman of a PR to try to figure out the best spot to
put this code and the documentation of how to use it. I don't really
know.
Sample content:
```
!_TAG_FILE_SORTED 1 1
!_TAG_FILE_ENCODING utf-8 1
_module.args /home/jade/dev/nixpkgs/lib/modules.nix 122
_module.check /home/jade/dev/nixpkgs/lib/modules.nix 186
_module.freeformType /home/jade/dev/nixpkgs/lib/modules.nix 193
_module.specialArgs /home/jade/dev/nixpkgs/lib/modules.nix 209
appstream.enable /home/jade/dev/nixpkgs/nixos/modules/config/appstream.nix 6
assertions /home/jade/dev/nixpkgs/nixos/modules/misc/assertions.nix 9
boot.binfmt.emulatedSystems /home/jade/dev/nixpkgs/nixos/modules/system/boot/binfmt.nix 272
```
Sample usage (can be used in parallel with `nix-doc tags`):
```
nix build -f nixos/release.nix optionsCtags -o opts-tags
(in vim): :set tags+=opts-tags
:tj boot.binfmt.emulatedSystems
```
This PR depends on and contains NixOS#249243. Marked as draft till that's
worked out. This PR pair replaces NixOS#208173.
Description of changes
What it does: line and column level declaration position information:
Use cases:
Related work:
lib/modules.nix: provide detailed error locations #65024
This one does it for all definitions rather than declarations, and it was not followed through with due to performance worries.
nixos/docs: add a ctags script for nixos options #208173
The basis for this change. This change is just a rebase of that one. I split it out to add the capability before adding users of it, in order to simplify review. However, the ctags script in there is a sample user of this feature.
Benchmarks: conducted by evaluating my own reasonably complex NixOS configuration with the command:
hyperfine -S none -w 1 -- "nix eval .#nixosConfigurations.snowflake.config.system.build.toplevel.outPath"Summary of results: it seems to be in the noise, this does not cause any visible regression in times.
Things done
sandbox = trueset innix.conf? (See Nix manual)nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)