Skip to content

Conversation

@MattSturgeon
Copy link
Contributor

Make use of lib.showAttrPath instead of manually doing concatStringsSep ".".

This means edge-cases like . being included in an attr-name will be handled better.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 25.05 Release Notes (or backporting 24.11 and 25.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@github-actions github-actions bot added 6.topic: module system About "NixOS" module system internals 6.topic: lib The Nixpkgs function library labels Apr 12, 2025
@nix-owners nix-owners bot requested review from hsjobeki, infinisil and roberth April 12, 2025 05:06
@github-actions github-actions bot added 10.rebuild-darwin: 1 This PR causes 1 package to rebuild on Darwin. 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. labels Apr 12, 2025
@roberth
Copy link
Member

roberth commented Apr 12, 2025

The new behavior looks good to me. Could you add a test case for this new requirement?

@MattSturgeon
Copy link
Contributor Author

Could you add a test case for this new requirement?

Sure, I can do that later. OTTOYH do you know where tests for mkPackageOption live?

@roberth
Copy link
Member

roberth commented Apr 12, 2025

Probably lib/tests/modules.sh, but we have one or two docs tests in lib/tests/misc.nix also.

@MattSturgeon MattSturgeon force-pushed the mkPackageOption-showAttrPath branch from 5a81405 to 8c47f93 Compare April 13, 2025 17:04
@MattSturgeon
Copy link
Contributor Author

MattSturgeon commented Apr 13, 2025

Thanks.

I've amended the commit to add tests for the defaultText and example being attr-paths that contain non-identifier names.


I also noticed several test cases were declared in declare-mkPackageOption.nix but not tested in modules.sh, so I've added a secondary commit to remedy that.


One other theoretical edge-case I noticed is when name' (used in description) is derived from the last element in the name attr-path, this could potentially contain markdown syntax which ought to be escaped.

I don't necessarily want to address that in this PR, and we don't seem to have a lib.strings.escapeMD yet either.

In such cases, users can manually specify name and default separately, so this isn't a big deal.

There were several test case options declared in `declare-mkPackageOption.nix`
that were not actually tested in `modules.sh`.
Make use of `lib.showAttrPath` instead of manually doing `concatStringsSep "."`.

This means edge-cases such as the attr-path including names that are not
valid nix identifiers will be handled better.

See:
- https://nix.dev/manual/nix/2.26/language/identifiers
- https://nixos.org/manual/nixpkgs/unstable/#function-library-lib.attrsets.showAttrPath
@MattSturgeon MattSturgeon force-pushed the mkPackageOption-showAttrPath branch from 8c47f93 to 6107d48 Compare April 13, 2025 20:01
@MattSturgeon
Copy link
Contributor Author

@roberth are you able to review now that tests have been added?

Copy link
Contributor

@hsjobeki hsjobeki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking lgtm to me. Since only the tests where missing @roberth, im merging this one.

@hsjobeki hsjobeki merged commit 843af86 into NixOS:master Apr 21, 2025
29 checks passed
@MattSturgeon MattSturgeon deleted the mkPackageOption-showAttrPath branch April 21, 2025 08:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

6.topic: lib The Nixpkgs function library 6.topic: module system About "NixOS" module system internals 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-darwin: 1 This PR causes 1 package to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants