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

mkDevos and evalDevosArgs: Move flake implementation logic into lib #206

Closed
wants to merge 18 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions doc/start/index.md
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ In addition, the [binary cache](../../cachix) is added for faster deployment.
- [Make installable ISO](./iso.md)
- [Bootstrap Host](./bootstrapping.md)
- [Already on NixOS](./from-nixos.md)
- [Use mkDevos](./mkDevos.md)


[install-nix]: https://nixos.org/manual/nix/stable/#sect-multi-user-installation
37 changes: 37 additions & 0 deletions doc/start/use-mkdevos.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
# Use mkDevos
You can also use devos as a flake input and get the advantage of using
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not a big fan of just throwing options at the user. It increases complexity and defeats the purpose of being easy-onboarding.

We should clearly blueprint the use case in simple english so that users can identify themselves as "yes that's me, I'll do it this way".

devos.lib

  • If you think devos template is tpp complicated ...

  • Once we are 1.0, don't fork this (!), use it as a lib instead...

  • ...

Copy link
Member Author

Choose a reason for hiding this comment

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

I think for now it should be documented as a 'next step' and another option. My reasoning is that there is some work to switch to the mkDevos method, and it's not an easy change.

nix flakes to sync with upstream changes in devos.

You can either use the default template or use the 'mkdevos' template which only
includes the necessary files for `mkDevos` usage. It can be pulled with:
```sh
nix flake init -t github:divnix/devos#mkdevos
```

Once you have a template, edit your flake.nix to add devos as an input:
```nix
inputs = {
...
devos.url = "github:divnix/devos";
};
```
then replace the outputs section with this:
```nix
outputs = { self, devos, ... }: devos.lib.mkDevos { inherit self; };
Copy link
Contributor

Choose a reason for hiding this comment

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

From a clean naming point of view this should be devos.lib.mkFlake.

"Make a flake with devos". Else we immediatly have a reasoning issue:

What is this Devos, that you can make with devos?!?

Copy link
Contributor

Choose a reason for hiding this comment

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

devos.mkFlake for external devos-as-a-library users.

Comment on lines +13 to +20

Choose a reason for hiding this comment

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

Consider instead of specifying individual changes be like "this is how your flake should look like" and users can add extra attributes if necessary

Copy link
Member Author

Choose a reason for hiding this comment

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

I like that idea. I think docs should be its own PR too, discussion for that can happen on its own.

```

Now you can sync with upstream devos changes just like any other flake input.

If you would like to change your directory's structure, you can pass
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not a big fan of nobleing this into the docs as a stand-alone use case: I'm not sure if preference is a use case motivated all by itself.

Also we should not throw options at users without clear guidance in what situations they have to use which option.

arguments to `mkDevos` to outline where certain things are. You can take a
look at [evalDevosArgs](../../lib/devos/evalDevosArgs.nix) to learn about
the arguments that it can take.

> ##### Notes:
Copy link
Contributor

@blaggacao blaggacao Mar 24, 2021

Choose a reason for hiding this comment

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

A few too many notes, I guess. If it merits exhibition, then it probably also deserves more than a note.

We should find other means for them.

Copy link
Member Author

Choose a reason for hiding this comment

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

My thoughts too, the docs need some work

> - All inputs used in devos must also be added to your flake, due to an
> [issue](https://github.com/NixOS/nix/pull/4641) with the `follows` attribute.
> - You can append `/community` to the `devos.url` to get access to community modules
> and packages which can be added in [extern](../../extern).
> - Many folders in devos will no longer be needed and can, optionally, be deleted.
> - To use community profiles, you will have to copy them to your tree
Copy link
Contributor

@blaggacao blaggacao Mar 24, 2021

Choose a reason for hiding this comment

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

Why? — asks the reader (not me).

Is it relevant (enough) to mention? Can't it be solved with better UX?

Copy link
Member Author

Choose a reason for hiding this comment

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

Well its because profiles are not exported anywhere in the flake. I don't see the point in adding the why to docs. And also at some point we could try to export profiles under nixosModules to fix this.

> - If you use this method, you will no longer be able to edit devos internals.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this can be dropped. Rather we need to define clear guidance when users are expected to use which method:

  • library
  • template
  • fork

I think maybe we should avoid (end user) documenting this PR alltogether at this moment and rather have a discussion about what usage modes we recommend for which situation.

Copy link
Member Author

Choose a reason for hiding this comment

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

I would appreciate being told this, just to make it clear the cons of using mkDevos.

69 changes: 6 additions & 63 deletions flake.nix
Original file line number Diff line number Diff line change
Expand Up @@ -28,68 +28,11 @@
pkgs.inputs.nixpkgs.follows = "nixos";
};

outputs = inputs@{ deploy, nixos, nur, self, utils, ... }:
let
inherit (self) lib;
inherit (lib) os;

extern = import ./extern { inherit inputs; };

multiPkgs = os.mkPkgs;

outputs = {
nixosConfigurations =
import ./hosts (nixos.lib.recursiveUpdate inputs {
inherit multiPkgs extern;
defaultSystem = "x86_64-linux";
lib = nixos.lib.extend (final: prev: {
dev = self.lib;
});
});

homeConfigurations = os.mkHomeConfigurations;

nixosModules =
let moduleList = import ./modules/module-list.nix;
in lib.pathsToImportedAttrs moduleList;

homeModules =
let moduleList = import ./users/modules/module-list.nix;
in lib.pathsToImportedAttrs moduleList;

overlay = import ./pkgs;
overlays = lib.pathsToImportedAttrs (lib.pathsIn ./overlays);

lib = import ./lib { inherit nixos self inputs; };

templates.flk.path = ./.;
templates.flk.description = "flk template";
defaultTemplate = self.templates.flk;

deploy.nodes = os.mkNodes deploy self.nixosConfigurations;
outputs = inputs@{ deploy, nixos, nur, self, utils, ... }:
let
lib = import ./lib { inherit self nixos inputs; };

Choose a reason for hiding this comment

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

Just a little idea for the future - separate the library implementation from this template repository

(probably not relevant as part of this PR)

Copy link
Member Author

Choose a reason for hiding this comment

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

I think I would be for that. @nrdxp @blaggacao thoughts?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm definitely up for it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Definitly, in the coming days, until 1.0 we might need the benefit of atomic spanning PRs, still, though.

in
lib.mkDevos {
inherit self;
};

systemOutputs = utils.lib.eachDefaultSystem (system:
let pkgs = multiPkgs.${system}; in
{
checks =
let
tests = nixos.lib.optionalAttrs (system == "x86_64-linux")
(import ./tests { inherit self pkgs; });
deployHosts = nixos.lib.filterAttrs
(n: _: self.nixosConfigurations.${n}.config.nixpkgs.system == system) self.deploy.nodes;
deployChecks = deploy.lib.${system}.deployChecks { nodes = deployHosts; };
in
nixos.lib.recursiveUpdate tests deployChecks;

packages = utils.lib.flattenTreeSystem system
(os.mkPackages { inherit pkgs; });

devShell = import ./shell {
inherit self system;
};
}
);
in
nixos.lib.recursiveUpdate outputs systemOutputs;
}
4 changes: 3 additions & 1 deletion lib/attrs.nix
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,9 @@ rec {
paths' = lib.filter (lib.hasSuffix ".nix") paths;
in
genAttrs' paths' (path: {

Choose a reason for hiding this comment

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

In Nix usually, collection itself (in this case paths') is specified as the last argument. While we are at it - maybe it's time to change it as well?

name = lib.removeSuffix ".nix" (baseNameOf path);
name = lib.removeSuffix ".nix"
# Safe as long this is just used as a name
(builtins.unsafeDiscardStringContext (baseNameOf path));
Copy link
Contributor

@blaggacao blaggacao Mar 26, 2021

Choose a reason for hiding this comment

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

What's motivating this change?

(And typo in the comment)

Copy link
Member Author

Choose a reason for hiding this comment

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

see pr that changes pathsToImportedAttrs I put reasoning there.

value = import path;
});

Expand Down
1 change: 1 addition & 0 deletions lib/default.nix
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ lib.makeExtensible (final:
lists = callLibs ./lists.nix;
strings = callLibs ./strings.nix;

inherit (os) mkDevos;
inherit (attrs) mapFilterAttrs genAttrs' pathsToImportedAttrs concatAttrs;
inherit (lists) pathsIn;
inherit (strings) rgxToString;
Expand Down
11 changes: 11 additions & 0 deletions lib/devos/default.nix
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,21 @@
config = { allowUnfree = true; };
};

importIf = path: fallback:
Copy link
Contributor

@blaggacao blaggacao Mar 26, 2021

Choose a reason for hiding this comment

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

This does not allow to declare things inline (within the flake). There is no reason to enforce library users to use any sort of file hierarchy.

To reflect that, rather let's use this implementation:

https://github.com/numtide/flake-utils/blob/master/simpleFlake.nix#L31

Copy link
Contributor

Choose a reason for hiding this comment

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

Big reverbaration step of mine, see other comment about my thought process, here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah with the planned mkFlake I don't think the 'importIf' function will be necessary. I really only made it to maintain backwards compatibility so mkDevos could be used on devos itself. But I see now why that would be better done outside of lib.

if builtins.pathExists path then import path else fallback;

Choose a reason for hiding this comment

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

Won't it crash if path does not contain default.nix file?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, but this is supposed to be a thin wrapper around nix's import and that would crash if there is no default.nix.


profileMap = map (profile: profile.default);

evalDevosArgs = dev.callLibs ./evalDevosArgs.nix;

mkDevos = dev.callLibs ./mkDevos.nix;

mkNodes = dev.callLibs ./mkNodes.nix;

mkHosts = dev.callLibs ./mkHosts.nix;

mkSuites = dev.callLibs ./mkSuites.nix;

mkProfileAttrs = dev.callLibs ./mkProfileAttrs.nix;

mkPkgs = dev.callLibs ./mkPkgs.nix;
Expand Down
166 changes: 166 additions & 0 deletions lib/devos/evalDevosArgs.nix
Original file line number Diff line number Diff line change
@@ -0,0 +1,166 @@
{ self, dev, nixos, inputs, ... }:
Copy link
Contributor

Choose a reason for hiding this comment

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

dev. This PR is totally the wrong place to complain, but every time I read it, I have to go through an uncounted number of mental hops to figure out what it means. 😕

What is it? (Can we make that evident from its name?)

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't like dev either, it is kind of confusing. I'm not sure how to fix it though outside of better documentation. Its just a side effect of needing to export lib as lib in the flake, but also requiring a way to access devos lib within other devos lib functions.

Copy link
Collaborator

Choose a reason for hiding this comment

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

perhaps simply renaming to osLib or devLib. I really just went with dev to keep it as convenient as lib, i.e. three chars.

Copy link
Member Author

@Pacman99 Pacman99 Mar 27, 2021

Choose a reason for hiding this comment

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

I personally think dev is ok, my main issue is the fact that lib in some places refers to devos lib and in other places refers to nixos lib. perhaps we can just standardize dev and use it everywhere even in flake.nix. So dev always refers to devos lib and lib always refers to nixos lib. I'm confusing myself nevermind. We should definitely export the devos lib as lib in flake.nix.


{ args }:
let
argOpts = with nixos.lib; { config, options, ... }:
let
inherit (dev) os;
inherit (os) importIf;
Copy link
Contributor

@blaggacao blaggacao Mar 24, 2021

Choose a reason for hiding this comment

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

Could that be devos.lib.maybeImport just as if any external lib consumer would imvoke it at the calling site? The os token is similarily unfortunate as is dev in my opinion.

Or to align with how we import:

import (devos.lib) maybeImport

Copy link
Contributor

Choose a reason for hiding this comment

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

Looka like it would be maybeImportElse, now.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm also not sold on not declaring it locally as long as it is not of actual use in another place.

Copy link
Member Author

Choose a reason for hiding this comment

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

I could just declare it in evalDevosArgs, its really only useful there.
Still thinking of names, importWithFallback is along the lines of what I'll probably go with.

Copy link
Member Author

Choose a reason for hiding this comment

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

Should not be necessary with mkFlake


cfg = config;
inherit (cfg) self;

inputAttrs = types.functionTo types.attrs;
in
{
options = with types; {
self = mkOption {
type = addCheck attrs nixos.lib.isStorePath;
description = "The flake to create the devos outputs for";
};
hosts = mkOption {
type = path;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not convinced of any benefits of encoding any specific file hierarchy into the library's API. The library should require to abstractly fulfill its own requirements and nothing more.

How people organize their files does not matter as long as their contents fulfill the devos requirement.

This stance complements my call for maybeImport: people can also declare functions directly within the top level flake.

Note, that flake.nix will shrink dramatically. Hence the only sane purpose we can give it is "index". We should not hide that away into a library function.

Copy link
Contributor

@blaggacao blaggacao Mar 26, 2021

Choose a reason for hiding this comment

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

An alternative is to layer another funktion:

devos.mkDevosFlake (to encode file hierarchy) over devos.mkFlake, but we really forego a stripped down flake.nix "index"-service if we don't always specify paths directly within the flake.nix.

So we shoudn't do a mkdDevosFlake / mkDevos at all: the "devos" part in mkDevos would actually become a template. That is much more inclusive of potential future library users.

Copy link
Member Author

Choose a reason for hiding this comment

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

Will address in planned mkFake PR. So only devos encodes its own filesystem api within its own flake.nix, but its not encoded within lib.

default = "${self}/hosts";
defaultText = "\${self}/hosts";
description = "Path to directory containing host configurations";
};
packages = mkOption {
# functionTo changes arg names which breaks flake check
type = types.anything // {
check = builtins.isFunction;
description = "Nixpkgs overlay";
};
default = importIf "${self}/pkgs" (final: prev: {});

Choose a reason for hiding this comment

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

It makes me wonder if users won't get confused at first that something gets picked up by just creating a folder (without manually declaring it)

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point, I should be able to address this soon when switching to mkFlake which will be more general and then devos will use mkFlake on itself. That way the importIf is not necessary either.. @blaggacao I think this is a good example of the corruption you mentioned.

defaultText = "\${self}/pkgs";
description = ''
Overlay for custom packages that will be included in treewide 'pkgs'.
This should follow the standard nixpkgs overlay format - two argument function
that returns an attrset.
'';
};
modules = mkOption {
type = listOf anything;
default = importIf "${self}/modules/module-list.nix" [];
defaultText = "\${self}/modules/module-list.nix";
apply = dev.pathsToImportedAttrs;
description = "list of modules to include in confgurations";
};
userModules = mkOption {
type = listOf anything;
default = importIf "${self}/users/modules/module-list.nix" [];
defaultText = "\${self}/users/modules/module-list.nix";
apply = dev.pathsToImportedAttrs;
description = "list of modules to include in home-manager configurations";
};
profiles = mkOption {
type = path;
default = "${self}/profiles";
defaultText = "\${self}/profiles";
apply = os.mkProfileAttrs;
description = "path to profiles folder";
};
userProfiles = mkOption {
type = path;
default = "${self}/users/profiles";
defaultText = "\${self}/users/profiles";
apply = os.mkProfileAttrs;
description = "path to user profiles folder";
};
suites =
let
defaults = { user = {}; system = {}; };
in
mkOption {
type = inputAttrs;
default = importIf "${self}/suites" ({...}: defaults);
defaultText = "\${self}/suites";
apply = suites: defaults // os.mkSuites {
inherit suites;
inherit (config) profiles users userProfiles;
};
description = ''
function with inputs 'users' and 'profiles' that returns attribute 'system'
which defines suites passed to configurations as the suites specialArg
'';
};
users = mkOption {
type = path;
default = "${self}/users";
defaultText = "\${self}/users";
apply = os.mkProfileAttrs;
description = "path to folder containing user profiles";
};
extern =
let
defaults = {
modules = []; overlays = []; specialArgs = {};
userModules = []; userSpecialArgs = [];
};
in
mkOption {
type = inputAttrs;
default = importIf "${self}/extern" ({...}: defaults);
defaultText = "\${self}/extern";
# So unneeded extern attributes can safely be deleted
apply = x: defaults // (x { inputs = inputs // self.inputs; });
description = ''
Function with argument 'inputs' with all devos and this flake's inputs.
The function should return an attribute set with modules, overlays, and
specialArgs to be included across devos
'';
};
overlays = mkOption {
type = path;
default = "${self}/overlays";
defaultText = "\${self}/overlays";
apply = x: dev.pathsToImportedAttrs (dev.pathsIn x);
description = "path to folder containing overlays which will be applied to pkgs";
};
overrides =
let
defaults = { modules = []; disabledModules = []; packages = _: _: _: {}; };
in
mkOption {
type = attrs;
default = importIf "${self}/overrides" defaults;
apply = x: defaults // x;
defaultText = "\${self}/overrides";
description = "attrset of packages and modules that will be pulled from nixpkgs master";
};
genDoc = mkOption {
type = functionTo attrs;
internal = true;
description = "function that returns a generated options doc derivation given nixpkgs";
};
};
config.genDoc =
let
singleDoc = name: value: ''
## ${name}
${value.description}

${optionalString (value ? type) ''
*_Type_*:
${value.type}
''}
${optionalString (value ? default) ''
*_Default_*
```
${builtins.toJSON value.default}
```
''}
${optionalString (value ? example) ''
*_Example_*
```
${value.example}
```
''}
'';
in
pkgs: with pkgs; writeText "devosOptions.md"
(concatStringsSep "" (mapAttrsToList singleDoc (nixosOptionsDoc { inherit options; }).optionsNix));
};
in
nixos.lib.evalModules {
modules = [ argOpts args ];
}
Loading