Skip to content

Conversation

@kuflierl
Copy link
Member

@kuflierl kuflierl commented Sep 27, 2025

This PR tries to remove generic with expressions that make it hard to discern the source of a function (and for LSP's to notice mistakes).

see also: #208242

All changes were made by hand with assistance from regex and lsp.

Some removed with expressions include:

  • with lib
  • with types

Things done

  • Built on platform:
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • Tested, as applicable:
  • Ran nixpkgs-review on this PR. See nixpkgs-review usage.
  • Tested basic functionality of all binary files, usually in ./result/bin/.
  • Nixpkgs Release Notes
    • Package update: when the change is major or breaking.
  • NixOS Release Notes
    • Module addition: when adding a new NixOS module.
    • Module update: when the change is significant.
  • Fits CONTRIBUTING.md, pkgs/README.md, maintainers/README.md and other READMEs.

Add a 👍 reaction to pull requests you find important.

@nixpkgs-ci nixpkgs-ci bot added 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 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/` 6.topic: hardware Drivers, Firmware and Kernels 6.topic: pantheon The Pantheon desktop environment labels Sep 27, 2025
@kuflierl kuflierl force-pushed the with-lib-modules branch 7 times, most recently from 27ef833 to 78f3f4c Compare September 27, 2025 20:34
@kuflierl kuflierl marked this pull request as ready for review September 30, 2025 11:01
@nixpkgs-ci nixpkgs-ci bot added the 2.status: merge conflict This PR has merge conflicts with the target branch label Oct 5, 2025
@nixpkgs-ci nixpkgs-ci bot removed the 2.status: merge conflict This PR has merge conflicts with the target branch label Oct 9, 2025
@kuflierl kuflierl force-pushed the with-lib-modules branch 3 times, most recently from 06ae956 to 4da1611 Compare October 9, 2025 20:52
@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/5933

Copy link
Member

@osbm osbm left a comment

Choose a reason for hiding this comment

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

I couldnt review maubot and synapse modules because of the huge diff but rest seems extremely well done.

I just have 1 thing to point out but this seems very much ready.

Thank you so much for this this makes the codes very readable in my opinion

@kuflierl kuflierl requested a review from osbm October 22, 2025 16:06
@nixpkgs-ci nixpkgs-ci bot added the 12.approvals: 1 This PR was reviewed and approved by one person. label Oct 22, 2025
@nixpkgs-ci nixpkgs-ci bot added the 2.status: merge conflict This PR has merge conflicts with the target branch label Oct 30, 2025
@kuflierl kuflierl force-pushed the with-lib-modules branch 2 times, most recently from f39cc3f to 01fc87f Compare October 31, 2025 22:25
@nixpkgs-ci nixpkgs-ci bot removed the 2.status: merge conflict This PR has merge conflicts with the target branch label Oct 31, 2025
@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/6008

@mmlb
Copy link
Member

mmlb commented Nov 1, 2025

Did you consider inhert (lib) ... instead? I gave it a quick try on uni-sync.nix and the diff is much nicer:

[11:04:59]-[~/c/g/n/nixpkgs]-[manny@zennix]
jj diff
diff --git a/nixos/modules/hardware/uni-sync.nix b/nixos/modules/hardware/uni-sync.nix
index 2fc9377256..0512b4d6bc 100644
--- a/nixos/modules/hardware/uni-sync.nix
+++ b/nixos/modules/hardware/uni-sync.nix
@@ -4,9 +4,17 @@
   pkgs,
   ...
 }:
-with lib;
 let
   cfg = config.hardware.uni-sync;
+  inherit (lib)
+    literalExpression
+    maintainers
+    mkEnableOption
+    mkIf
+    mkOption
+    mkPackageOption
+    types
+    ;
 in
 {
   meta.maintainers = with maintainers; [ yunfachi ];
[11:05:04]-[~/c/g/n/nixpkgs]-[manny@zennix]
nil diagnostics nixos/modules/hardware/uni-sync.nix
[--:--:--]-[~/c/g/n/nixpkgs]-[manny@zennix]

Its not super obvious but in the page @AndersonTorres links to in #208242 this is shown as the alternative in the tip.

@kuflierl
Copy link
Member Author

kuflierl commented Nov 1, 2025

Did you consider inhert (lib) ... instead? I gave it a quick try on uni-sync.nix and the diff is much nicer:

[11:04:59]-[~/c/g/n/nixpkgs]-[manny@zennix]
jj diff
diff --git a/nixos/modules/hardware/uni-sync.nix b/nixos/modules/hardware/uni-sync.nix
index 2fc9377256..0512b4d6bc 100644
--- a/nixos/modules/hardware/uni-sync.nix
+++ b/nixos/modules/hardware/uni-sync.nix
@@ -4,9 +4,17 @@
   pkgs,
   ...
 }:
-with lib;
 let
   cfg = config.hardware.uni-sync;
+  inherit (lib)
+    literalExpression
+    maintainers
+    mkEnableOption
+    mkIf
+    mkOption
+    mkPackageOption
+    types
+    ;
 in
 {
   meta.maintainers = with maintainers; [ yunfachi ];
[11:05:04]-[~/c/g/n/nixpkgs]-[manny@zennix]
nil diagnostics nixos/modules/hardware/uni-sync.nix
[--:--:--]-[~/c/g/n/nixpkgs]-[manny@zennix]

Its not super obvious but in the page @AndersonTorres links to in #208242 this is shown as the alternative in the tip.

Yes, that is an option, but i really don't like it due to the fact that i can not determine if it's a builtin or if its coming from lib. But i did do it for "lib.types" in some places

@mmlb
Copy link
Member

mmlb commented Nov 2, 2025

Did you consider inhert (lib) ... instead? I gave it a quick try on uni-sync.nix and the diff is much nicer:

[11:04:59]-[~/c/g/n/nixpkgs]-[manny@zennix]
jj diff
diff --git a/nixos/modules/hardware/uni-sync.nix b/nixos/modules/hardware/uni-sync.nix
index 2fc9377256..0512b4d6bc 100644
--- a/nixos/modules/hardware/uni-sync.nix
+++ b/nixos/modules/hardware/uni-sync.nix
@@ -4,9 +4,17 @@
   pkgs,
   ...
 }:
-with lib;
 let
   cfg = config.hardware.uni-sync;
+  inherit (lib)
+    literalExpression
+    maintainers
+    mkEnableOption
+    mkIf
+    mkOption
+    mkPackageOption
+    types
+    ;
 in
 {
   meta.maintainers = with maintainers; [ yunfachi ];
[11:05:04]-[~/c/g/n/nixpkgs]-[manny@zennix]
nil diagnostics nixos/modules/hardware/uni-sync.nix
[--:--:--]-[~/c/g/n/nixpkgs]-[manny@zennix]

Its not super obvious but in the page @AndersonTorres links to in #208242 this is shown as the alternative in the tip.

Yes, that is an option, but i really don't like it due to the fact that i can not determine if it's a builtin or if its coming from lib. But i did do it for "lib.types" in some places

I think lib is preferred over builtins in nixpkgs, which should probably be enforced if true, but that seems like a separate issue. I don't really like the full path on each use myself, but imo minimizing the diff would be the nicest option.

@AndersonTorres
Copy link
Member

In theory lib is more future- and past-safe. Sometimes a functionality is promoted to builtin or demoted to nix code. The lib in nixpkgs includes guards for cases when the interpreter does not have this or that builtin.

Further, treating everything as lib avoids the bikeshedding :)

@nixpkgs-ci nixpkgs-ci bot added the 2.status: merge conflict This PR has merge conflicts with the target branch label Nov 5, 2025
@wolfgangwalther
Copy link
Contributor

Merge conflicted after #443046. Not closing, because the other PR was only about meta = with lib, so there is a lot more here.

@nixpkgs-ci nixpkgs-ci bot removed the 2.status: merge conflict This PR has merge conflicts with the target branch label Dec 10, 2025
@kuflierl
Copy link
Member Author

I currently don't have much time and can not really do any major changes to my change-set.
I would honestly love it if this could be merged or dealt with in another way.

@osbm osbm requested a review from wolfgangwalther December 11, 2025 05:09
@wolfgangwalther wolfgangwalther removed their request for review December 11, 2025 08:07
@nixpkgs-ci nixpkgs-ci bot added the 2.status: merge conflict This PR has merge conflicts with the target branch label Dec 13, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

2.status: merge conflict This PR has merge conflicts with the target branch 6.topic: hardware Drivers, Firmware and Kernels 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 6.topic: pantheon The Pantheon desktop environment 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 This PR causes between 1 and 10 packages to rebuild on Linux. 12.approvals: 1 This PR was reviewed and approved by one person.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants