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

treewide: use mkPackageOption #261702

Merged
merged 1 commit into from
Nov 30, 2023

Conversation

h7x4
Copy link
Member

@h7x4 h7x4 commented Oct 17, 2023

Description of changes

This commit replaces a lot of usages of mkOption with the package type, to be mkPackageOptionMD, in order to reduce the amount of code.

I haven't changed all of these, but this is a good amount of them. Maybe we could just start with this, and do another PR later? I might add on stuff as time goes if it doesn't seem to get merged anytime soon.

Also, I haven't used any tool for this, I've just manually gone through them all. There might be some human errors due to that.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • 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/)
  • 23.11 Release Notes (or backporting 23.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.
    Module Updates
    • changes are backward compatible
    • removed options are declared with mkRemovedOptionModule
    • changes that are not backward compatible are documented in release notes
    • module tests succeed on ARCHITECTURE This is probably too large for me to build, not entirely sure how I would go about doing this.
    • options types are appropriate
    • options description is set
    • options example is provided I've transferred the examples where the option already had one
    • documentation affected by the changes is updated

@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` labels Oct 17, 2023
@h7x4 h7x4 force-pushed the replace-mkoption-with-mkpackageoption branch 3 times, most recently from 7647f54 to 71edf34 Compare October 17, 2023 20:47
@h7x4 h7x4 force-pushed the replace-mkoption-with-mkpackageoption branch 2 times, most recently from 24546ed to 72fa5b0 Compare October 17, 2023 21:25
@h7x4 h7x4 added 8.has: clean-up 6.topic: lib The Nixpkgs function library and removed 6.topic: vim labels Oct 17, 2023
@github-actions github-actions bot added 6.topic: vim and removed 6.topic: lib The Nixpkgs function library labels Oct 17, 2023
@h7x4 h7x4 marked this pull request as ready for review October 17, 2023 21:30
@wegank
Copy link
Member

wegank commented Oct 17, 2023

Why not just use mkPackageOption?

mkPackageOptionMD = mkPackageOption;

@h7x4
Copy link
Member Author

h7x4 commented Oct 17, 2023

@wegank Mostly for ease of backporting, at least for however many days we have left. A treewide PR from mkPackageOptionMD to mkPackageOption will probably be done at some point in 23.11, as mkPackageOptionMD stuff existed before this PR.

Edit: On second thought, I realize this doesn't make sense. Changes usually don't get backported as entire files, only the commits themselves. And since there are no reason to backport this change, I could've just as well just used mkPackageOption

@h7x4 h7x4 force-pushed the replace-mkoption-with-mkpackageoption branch 2 times, most recently from d67482d to bc8afd4 Compare October 18, 2023 00:14
@h7x4 h7x4 requested a review from piegamesde as a code owner October 18, 2023 00:14
@github-actions github-actions bot added the 6.topic: emacs Text editor label Oct 18, 2023
@h7x4 h7x4 force-pushed the replace-mkoption-with-mkpackageoption branch from bc8afd4 to 4bfc4ed Compare October 18, 2023 00:32
@h7x4 h7x4 requested a review from a team as a code owner October 18, 2023 00:32
@h7x4 h7x4 force-pushed the replace-mkoption-with-mkpackageoption branch from 4bfc4ed to 42a785d Compare October 18, 2023 01:59
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 labels Oct 18, 2023
@h7x4 h7x4 force-pushed the replace-mkoption-with-mkpackageoption branch from 42a785d to 23351e4 Compare October 18, 2023 15:46
@h7x4
Copy link
Member Author

h7x4 commented Oct 18, 2023

This is ready for reviews. I don't think I'll be changing it much more apart from review comments.

If it doesn't get merged within 23.11 feature freeze, I'll swap all the mkPackageOptionMDs to their mkPackageOption variant, but for now I think I'll just keep it as is, since we'll need another treewide PR to convert the others back anyway.

@h7x4 h7x4 requested a review from wegank October 18, 2023 22:39
@h7x4 h7x4 added the needs_reviewer (old Marvin label, do not use) label Oct 18, 2023
@delroth delroth added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Oct 19, 2023
@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-ready-for-review/3032/2815

@h7x4 h7x4 force-pushed the replace-mkoption-with-mkpackageoption branch from 23351e4 to dba47e0 Compare November 9, 2023 15:02
@h7x4 h7x4 force-pushed the replace-mkoption-with-mkpackageoption branch from dba47e0 to 2aea7b6 Compare November 27, 2023 00:20
@github-actions github-actions bot added the 6.topic: jupyter Interactive computing tooling: kernels, notebook, jupyterlab label Nov 27, 2023
@h7x4 h7x4 changed the title treewide: use mkPackageOptionMD treewide: use mkPackageOption Nov 27, 2023
@h7x4
Copy link
Member Author

h7x4 commented Nov 27, 2023

Updated the PR from mkPackageOptionMD -> mkPackageOption, and fixed the merge conflicts

@h7x4 h7x4 force-pushed the replace-mkoption-with-mkpackageoption branch from 2aea7b6 to 584e252 Compare November 27, 2023 00:23
This commit replaces a lot of usages of `mkOption` with the package
type, to be `mkPackageOption`, in order to reduce the amount of code.
@h7x4 h7x4 force-pushed the replace-mkoption-with-mkpackageoption branch from 584e252 to 0a37316 Compare November 27, 2023 00:28
@h7x4 h7x4 requested a review from wegank November 27, 2023 00:35
@wegank wegank merged commit feeae48 into NixOS:master Nov 30, 2023
@h7x4 h7x4 deleted the replace-mkoption-with-mkpackageoption branch December 8, 2023 07:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: emacs Text editor 6.topic: jupyter Interactive computing tooling: kernels, notebook, jupyterlab 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 6.topic: vim 8.has: clean-up 8.has: module (update) This PR changes an existing module in `nixos/` 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 12.approvals: 1 This PR was reviewed and approved by one reputable person needs_reviewer (old Marvin label, do not use)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants