Heavy split and refactor of flake.nix #1
Conversation
| resolveOverlays = first: overlays: with nixpkgs.lib; let | ||
| # Passing inputs here means overlays can be called with the standard signature | ||
| # but still be parametrized with flakes, maintaining composability. | ||
| overlay' = flip (composeManyExtensions ([ (final: prev: { inherit inputs; }) ] ++ overlays)); |
There was a problem hiding this comment.
What is the reason for flipping overlay arguments?
There was a problem hiding this comment.
And cant we just partially apply overlays for inputs instead?
Like having function overlay :: inputs -> final -> prev -> attrs and call it overlay inputs which results in overlay :: final -> prev -> attrs
| ''; | ||
| }; | ||
| }; | ||
| mkSystem = system: modules: nixpkgs.lib.nixosSystem { inherit system modules; }; # TODO does the system argument make sense? |
There was a problem hiding this comment.
Yes, its system dependent
| }; | ||
| mkSystem = system: modules: nixpkgs.lib.nixosSystem { inherit system modules; }; # TODO does the system argument make sense? | ||
| in { | ||
| overlays.default = import ./nix/overlay.nix; |
There was a problem hiding this comment.
Reference to comment about passing inputs to overlay, this is how you'd do it:
| overlays.default = import ./nix/overlay.nix; | |
| overlays.default = import ./nix/overlay.nix inputs; |
| }; | ||
| }); | ||
| # Expose the module for use as an input in another flake | ||
| nixosModules.liberaforms = callModule ./nix/module.nix; |
There was a problem hiding this comment.
Not sure why are you performing explicit inputs passing to modules but I'm sure that it can be avoided
| # Set the default package | ||
| // {default = self.packages.${system}.liberaforms;} | ||
| packages = genSystems (system: | ||
| let attrs = resolveOverlays pkgsFor.${system} [ (import ./nix/overlay.nix) ]; |
There was a problem hiding this comment.
| let attrs = resolveOverlays pkgsFor.${system} [ (import ./nix/overlay.nix) ]; | |
| let attrs = resolveOverlays pkgsFor.${system} [ self.overlays.default ]; |
| @@ -26,7 +22,7 @@ in { | |||
|
|
|||
| package = mkOption { | |||
| type = types.package; | |||
| default = self.packages.${pkgs.system}.default; | |||
| default = packages.default; | |||
There was a problem hiding this comment.
| default = packages.default; | |
| inherit (packages) default; |
| @@ -182,7 +178,7 @@ in { | |||
| after = ["network.target" "postgresql.service"]; | |||
| requires = ["postgresql.service"]; | |||
| restartIfChanged = true; | |||
| path = with pkgs; [postgresql_11 liberaforms-env openssl]; | |||
| path = with pkgs; [postgresql_11 penv openssl]; | |||
There was a problem hiding this comment.
nixpkgs already has PostgreSQL service defined, we just need to configure it. no need to define a new one
| final: prev: | ||
| (with builtins; seq ((hasAttr "inputs" prev) || throw "If you are calling this directly, make sure the overlays have an `inputs` attribute conforming to the flakes interface.")) | ||
| (let | ||
| packages = final.inputs.self.packages.${prev.system}; | ||
| in { |
There was a problem hiding this comment.
| final: prev: | |
| (with builtins; seq ((hasAttr "inputs" prev) || throw "If you are calling this directly, make sure the overlays have an `inputs` attribute conforming to the flakes interface.")) | |
| (let | |
| packages = final.inputs.self.packages.${prev.system}; | |
| in { | |
| inputs: final: prev: | |
| { |
There is never a reason to use packages in overlays. Please use prev argument instead
There was a problem hiding this comment.
I'm...not even sure why that's there. That should almost definitely be final.
| ${filteredReq} | ||
| ${extraReq} | ||
| ''; | ||
| in final.inputs.mach-nix.lib.${prev.system}.mkPython { inherit requirements; }; |
There was a problem hiding this comment.
Please avoid using final so much
There was a problem hiding this comment.
Final is the whole point of using overlays though.
| checkPhase = '' | ||
| source ${./test_env.sh.in} | ||
| initPostgres $(mktemp -d) | ||
|
|
||
| # Run pytest on the installed version. A running postgres database server is needed. | ||
| (cd tests && cp test.ini.example test.ini && pytest -k "not test_save_smtp_config") #TODO why does this break? | ||
|
|
||
| shutdownPostgres | ||
| ''; |
There was a problem hiding this comment.
Consider moving this to passthru.tests
|
I generally agree with Gytis here. It feels like these changes overcomplicate the code instead of simplifying it, even though they split it up more. Also, one question: what does composability mean here and why should we care for it? In my approach I haven't used a single |
|
My rather bad explanation is it means you can take things and build / combine more things out of them(, without having to fight them to fit). Kind of the inverse of reductionism. It!s kind of the opposite of everything being tightly coupled and hard to use anywhere else. |
|
I've applied a lot of changes from here into master. Thanks @deliciouslytyped for the refactor! |
I didn't look at module.nix
pr for code review @gytis-ivaskevicius