feat: add lib to flake outputs exposing multiService func #31

adrian-gierakowski · 2023-07-20T20:52:25Z

closes #25

nix run github:srid/nixci completed successfully

adrian-gierakowski · 2023-07-20T20:54:11Z

flake.nix

@@ -7,6 +7,10 @@
      path = builtins.path { path = ./example; filter = path: _: baseNameOf path == "flake.nix"; };
    };

+    # NOTE: this is a lib factory, as it takes: { pkgs, lib }
+    # so before using we need to pass it to pkgs.callPackage
+    lib = import ./nix/lib.nix;


not sure if it's common for the lib output to be a function but nix flake check didn't complain

for this to be a function we'd have add nixpkgs as flake input

In addition to not being able to use inputs.nixpkgs.lib the submodule below passes pkgs as well, as specialArgs, we necessarily need some perSystem context. @roberth is there an idiomatic way here?

I wonder if it would be better to expose this via an overlay:

final: _: { services-flake = { lib = import ./nix/lib.nix { pkgs = final; }; }; }

or if you want to give it a bit more thought, we could not expose it at all and I'd import the file directly via:

import "${services-flake}/nix/lib.nix" { inherit pkgs; }

I would go with importing it directly as a file for now, until we have a cleaner way to expose it.

Cool, I’ll update the PR accordingly

Thanks! I will merge it once you make the change.

I wonder if it would be better to expose this via an overlay:

Overlays are for making changes to the package set, not for exposing additions.

roberth

Some ideas. It looks like your lib doesn't need a pkgs.

roberth · 2023-07-25T08:48:33Z

flake.nix

@@ -7,6 +7,10 @@
      path = builtins.path { path = ./example; filter = path: _: baseNameOf path == "flake.nix"; };
    };

+    # NOTE: this is a lib factory, as it takes: { pkgs, lib }
+    # so before using we need to pass it to pkgs.callPackage
+    lib = import ./nix/lib.nix;


Suggested change

lib = import ./nix/lib.nix;

lib = {

withPkgs = import ./nix/lib.nix;

};

This leaves room for functions that don't depend on pkgs.
Accepting a lib attribute in the parameter is implied.

roberth · 2023-07-25T08:50:07Z

flake.nix

@@ -7,6 +7,10 @@
      path = builtins.path { path = ./example; filter = path: _: baseNameOf path == "flake.nix"; };
    };

+    # NOTE: this is a lib factory, as it takes: { pkgs, lib }
+    # so before using we need to pass it to pkgs.callPackage
+    lib = import ./nix/lib.nix;


I wonder if it would be better to expose this via an overlay:

Overlays are for making changes to the package set, not for exposing additions.

roberth · 2023-07-25T08:56:55Z

flake.nix

+    # NOTE: this is a lib factory, as it takes: { pkgs, lib }
+    # so before using we need to pass it to pkgs.callPackage
+    lib = import ./nix/lib.nix;


Suggested change

# NOTE: this is a lib factory, as it takes: { pkgs, lib }

# so before using we need to pass it to pkgs.callPackage

lib = import ./nix/lib.nix;

lib = {

/*

Type:

{ lib, pkgs } -> { ... }

A library of system-specific functions.

*/

withPkgs = import ./nix/lib.nix;

};

This leaves room for functions that don't depend on pkgs.
Accepting a lib attribute in the parameter is implied.

roberth · 2023-07-25T08:58:19Z

nix/lib.nix

+{ pkgs, lib ? pkgs.lib }: {
+  # Create an attrsOf module wrapper (`services.${name}`) around the given submodule.
+  #
+  # where module filename is of form `${name}.nix`. The submodule takes this
+  # 'name' parameter, and is expected to set the final process-compose config in
+  # its `outputs.settings` option.
+  multiService = mod:
+    let
+      # Derive name from filename
+      name = lib.pipe mod [
+        builtins.baseNameOf
+        (lib.strings.splitString ".")
+        builtins.head
+      ];
+    in
+    { config, ... }: {


You could take pkgs from the module parameters instead.

Suggested change

{ pkgs, lib ? pkgs.lib }: {

# Create an attrsOf module wrapper (`services.${name}`) around the given submodule.

#

# where module filename is of form `${name}.nix`. The submodule takes this

# 'name' parameter, and is expected to set the final process-compose config in

# its `outputs.settings` option.

multiService = mod:

let

# Derive name from filename

name = lib.pipe mod [

builtins.baseNameOf

(lib.strings.splitString ".")

builtins.head

];

in

{ config, ... }: {

{ lib }: {

# Create an attrsOf module wrapper (`services.${name}`) around the given submodule.

#

# where module filename is of form `${name}.nix`. The submodule takes this

# 'name' parameter, and is expected to set the final process-compose config in

# its `outputs.settings` option.

multiService = mod:

let

# Derive name from filename

name = lib.pipe mod [

builtins.baseNameOf

(lib.strings.splitString ".")

builtins.head

];

in

{ config, pkgs, ... }: {

thank you @roberth, this is my favourite option

@srid @shivaraj-bh what do you think

Btw. is the lib flake output expected to be { lib } -> { ... } or just { ... }?

Ya, let's go with this approach.

But I don't believe you need the { lib } parameter either (you can use pkgs.lib instead and move the let block inside the module block). Which means this could simply be exposed as lib.multiService.

pkgs.lib does require configuring and evaluating pkgs, so in order to have a more consistent pattern across the ecosystem, it is beneficial to accept lib separately. If it doesn't matter in your case, take lib ? pkgs.lib.

Btw. is the lib flake output expected to be { lib } -> { ... } or just { ... }?

It doesn't come with expectations, but you could do { ... } if you take lib from the flake inputs.
Here flakes is an example of a framework that does too much, even if it's just fairly simple dependency injection. It's there, so we're expected to use it, even if it's not quite the right thing. Maybe it should be { ... } // { withLib = ... } if you want to both satisfy all expectations and make the interface fully general.
Could be over-engineered or just right. I'll leave that up to you.

pkgs.lib does require configuring and evaluating pkgs, so in order to have a more consistent pattern across the ecosystem, it is beneficial to accept lib separately.

makes sense

srid · 2023-09-06T14:28:07Z

Anything to be done here? @adrian-gierakowski

closes juspay#25

adrian-gierakowski · 2023-09-06T15:32:51Z

Anything to be done here? @adrian-gierakowski

this should now be ready to merge

I've removed the need for injecting pkgs and lib (as you suggested in #31 (comment)) so now the flake's lib output is simply an attrset

adrian-gierakowski commented Jul 20, 2023

View reviewed changes

adrian-gierakowski force-pushed the feat/add-lib-to-flake-outputs-exposing-multiService-func branch from 5f4ccd8 to bdf7997 Compare July 20, 2023 20:54

roberth reviewed Jul 25, 2023

View reviewed changes

feat: add lib to flake outputs exposing multiService func

695c97c

closes juspay#25

adrian-gierakowski force-pushed the feat/add-lib-to-flake-outputs-exposing-multiService-func branch from bdf7997 to 695c97c Compare September 6, 2023 15:31

srid merged commit 14904d0 into juspay:main Sep 6, 2023
3 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add lib to flake outputs exposing multiService func #31

feat: add lib to flake outputs exposing multiService func #31

adrian-gierakowski commented Jul 20, 2023 •

edited

Loading

adrian-gierakowski Jul 20, 2023

srid Jul 20, 2023 •

edited

Loading

adrian-gierakowski Jul 24, 2023

shivaraj-bh Jul 24, 2023

adrian-gierakowski Jul 24, 2023

shivaraj-bh Jul 24, 2023

roberth Jul 25, 2023

roberth left a comment

roberth Jul 25, 2023

roberth Jul 25, 2023

roberth Jul 25, 2023

roberth Jul 25, 2023

adrian-gierakowski Jul 25, 2023

srid Jul 25, 2023

roberth Jul 25, 2023

roberth Jul 25, 2023

srid Jul 25, 2023

srid commented Sep 6, 2023

adrian-gierakowski commented Sep 6, 2023

feat: add lib to flake outputs exposing multiService func #31

feat: add lib to flake outputs exposing multiService func #31

Conversation

adrian-gierakowski commented Jul 20, 2023 • edited Loading

Choose a reason for hiding this comment

srid Jul 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roberth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srid commented Sep 6, 2023

adrian-gierakowski commented Sep 6, 2023

adrian-gierakowski commented Jul 20, 2023 •

edited

Loading

srid Jul 20, 2023 •

edited

Loading