Move host and suite implementation logic to lib #216

Pacman99 · 2021-03-26T15:45:32Z

This is just the mkSuites and mkHosts part of the mkDevos PR. I would like to avoid changing mkSuites and mkHosts api, to make it easier to rebase changes into the mkDevos branch. But if necessary we can change them. And to that end, there is some more logic added to the flake.nix now which would ideally not be there if devos was meant to be a template. But since the goal is to move towards a lib function with template, this is just a step in that direction.

Pacman99 · 2021-03-26T15:46:50Z

flake.nix

+        suites = import ./suites;
+        users = os.mkProfileAttrs "${self}/users";
+        profiles = os.mkProfileAttrs "${self}/profiles";
+        userProfiles = os.mkProfileAttrs "${self}/users/profiles";
+      };
+
      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;
-            });
-          });
+        nixosConfigurations = os.mkHosts {
+          dir = "${self}/hosts";
+          overrides = import ./overrides;


All paths here have to passed as strings or nix says paths aren't allowed to refer to the flake store path. Makes no sense to me since those paths are within the flake, but this is the workaround.

That means even ./hosts does not work? If so, that's really "interesting".

Yup, I dealt with this in the mkFlake PR by converting everything to string using apply keys. But I don't have that option here. I think its because of builtins.readDir reacting weirdly to paths. But overall I think I've found when working with store paths, everything is just easier if their strings.

Pacman99 · 2021-03-26T15:47:48Z

lib/devos/mkProfileAttrs.nix

@@ -27,7 +27,7 @@ let mkProfileAttrs =
    f = n: _:
      lib.optionalAttrs
        (lib.pathExists "${dir}/${n}/default.nix")
-        { default = /. + "${dir}/${n}"; }
+        { default = builtins.toPath "${dir}/${n}"; }


store paths cannot be appended to paths. This is a deprecated function(I think?), but it is the only way I found of converting a store path to a 'path' type. And we need these as paths for the disabledModules part of isoConfig to work.

disabledModules should also work by using a partial path string relative to the systems nixpkgs/nixos/modules directory. So maybe this isn't needed after all.

But we are trying to disable paths in devos, so we can't pass anything relative to nixpkgs/nixos/modules. The other option is to set modulesPath to "profiles/" but I'm not sure if that breaks other things in the module system. I think its better to just include this change for now, to get these functions working. And in the future work out a way to pass these paths as strings and still get it to work with disabledModules. I want to avoid changing isoConfig in this PR.

disabledModule is less brittle when working with keys. In a sharing model, modules can also come from external devos flakes.

Equality of store paths is cheap even in those scenarios. Equality of relative path-strings sets us up for trouble.

I believe this is the cause of the BORS failure. For whatever reason, the core in mkHosts and this the one imported here have different hashes, meaning toPath must have changed it somehow. We may want to figure out exactly how, or #136 may be reopened.

edit

actually I was wrong. I'm gonna work on this later tonight and try to at least determine the real cause.

This seems to work as just "${dir}/${n}", had you tried that before toPath?

No I know that works. The reason this has to be passed as a path is for the disabledModules section of isoConfig. If you look here: https://github.com/NixOS/nixpkgs/blob/1dfe690209af14a91b32dbf2f379ca02d050e1a1/lib/modules.nix#L241

Any string passed to disabledModules is always appended to modulesPath. But if you pass it as a path, then it works.

Ohh I just read your commit message, I guess toPath doesn't help either.

I think this PR will just break isoConfig then. We will need to find a better fix for that.

Ah, now I see, so we could probably just map (x: /. + x) to fix isoConfig. Or maybe we could remove the string context before converting to a path with /. so we can keep profiles as proper paths in the first place.

blaggacao · 2021-03-27T16:06:35Z

Can you check if it works with just declaring relative paths? (./hosts, etc.)

Otherwise this changes is already really enjoyable to see.

What's really nice about this:

in flake.nix we have the top level inputs
but we also can see what the precise inputs are for devos specific functions (in a clear and simple way)

So overall, I feel it's easier (even for newcomers with a little bit of programming knowledge) to "see what's going on".

Pacman99 · 2021-03-27T16:28:52Z

Can you check if it works with just declaring relative paths? (./hosts, etc.)

Heres the error I get:

error: access to path '/nix/store/i3z291bqqjmsgr4mpv8ys90da7h1nazy-hosts/NixOS.nix' is forbidden in restricted mode
(use '--show-trace' to show detailed location information)

I have no clue whats happening, because that store path that it is talking about actually exists and there is a README and NixOS.nix in there. Which means at some point some function ended up getting nix to create a new store path for the directory it was reading with the necessary files, and then error itself out because it doesn't ave access to those files.

blaggacao

Maybe this could be the culprit...

lib/devos/mkHosts.nix

Pacman99 · 2021-03-27T17:48:53Z

So the only solution I can think of for the paths issue is change any lib function that uses builtins.readDir to convert the dir to a string before passing it to the builtin. I really don't like the solution though.

blaggacao · 2021-03-27T17:51:13Z

I really don't like the solution though.

If we call it an "anti corruption layer" against nix quircks, would that appease?

I think, as a general rule, working with paths is always a better option, simply because it is a richer type to mean the same.

Pacman99 · 2021-03-27T17:54:48Z

I would actually argue the opposite, specially for store paths. All of nixpkgs passes around store paths as strings. And I think nix in general treats paths as a shorthand to represent a string. So paths actually have more chance for corruption since they can be relative or absolute and can cause issues with builtin functions. String paths on the other hand are always absolute and they universally work on all builtin functions. While I would like users to be able to pass in paths, I think of it more of a convenience and the mkFlake PR does allow this and solves the issue by converting to string immediately.

Pacman99 · 2021-03-27T17:56:05Z

But for this PR since we are still in the stage of devos where users aren't necessarily meant to edit their flake.nix, It think the current solution is best to just pass the paths as strings for profiles/hosts.

blaggacao · 2021-03-27T18:00:00Z

LGTM! Let's address this separately and get this rather merged.

Strange thing that nixpkgs prefers strings over paths. At the very least it's bad semantics and an absent anti corruption fuse somewhere deep down.

As I understand it, paths are checked for equality by their internal nix store paths, hence wether relative or absolute, they seem to be always canonicalized.

blaggacao · 2021-03-27T18:03:02Z

bors r+

bors · 2021-03-27T18:03:04Z

👎 Rejected by too few approved reviews

blaggacao

LGTM (from cell phone)

blaggacao · 2021-03-27T18:14:20Z

bors r+

216: Move host and suite implementation logic to lib r=blaggacao a=Pacman99 This is just the `mkSuites` and `mkHosts` part of the `mkDevos` PR. I would like to avoid changing mkSuites and mkHosts api, to make it easier to rebase changes into the mkDevos branch. But if necessary we can change them. And to that end, there is some more logic added to the flake.nix now which would ideally not be there if devos was meant to be a template. But since the goal is to move towards a lib function with template, this is just a step in that direction. Co-authored-by: Pacman99 <[email protected]>

bors · 2021-03-27T18:15:21Z

Build failed:

ci/hercules/evaluation

Pacman99 · 2021-03-27T18:31:55Z

huh core is being imported twice in profilesTest. Ohh so core is being imported by mkHosts, and since profile's test imports allProfiles it gets imported twice. @nrdxp why was this not an issue before? I'm not sure what I changed to only trigger this now.

nrdxp · 2021-03-27T20:41:18Z

Is it the same issue from #136? Essentially we fixed it by insuring that all profiles in mkProfilesAttrs a just a path to the profile, since the nix module system seems to compare the paths and remove redundant entries from imports, but it doesn't do the same for a module function.

Update

After pulling the branch and running nix flake check the profilesTest passed without issue. Perhaps this is another issue with stable nix. It could be that toPath was deprecated in the unstable branch and has different behavior in stable. In any case, we can probably just filter out core from allProfiles since it is always present via mkHosts regardless.

2nd Update

Okay, so I actually built profilesTest with nixStable and it passed on my machine locally. So I'm actually a bit confused about what caused the failure in CI now.

/cc @blaggacao @Pacman99

blaggacao · 2021-03-27T20:55:02Z

👍 for filtering out core from allProfiles.

Pacman99 · 2021-03-27T21:19:08Z

Yeah I am a little confused, it should have been able to filter it out, relative and absolute paths compare well
Also random idea, maybe core could be a module thats enabled by default instead. So that way users can customize what they want out of core. something like devos.core.enable.

Pacman99 · 2021-03-27T21:23:36Z

bors try

bors · 2021-03-27T21:24:32Z

try

Build failed:

ci/hercules/evaluation

nrdxp · 2021-03-27T23:24:45Z

Since this PR also introduces new lib functions. A minimal test case is in order to ensure corner cases are worked out, or at least documented before merge. I am gonna try to help out with this later tonight when I have more time.

Pacman99 · 2021-03-28T02:33:20Z

Since this PR also introduces new lib functions. A minimal test case is in order to ensure corner cases are worked out, or at least documented before merge. I am gonna try to help out with this later tonight when I have more time.

That would be great! I'm not sure how to write tests for lib functions, so any demonstration would be helpful.

nrdxp · 2021-03-28T04:49:58Z

Sorry for force push, meant to send it to divnix branch but got pushed to yours by mistake. I didn't actually change your commits, only added and then removed one, so it should still be the same. Just experimenting with the CI issue.

Pacman99 · 2021-03-28T04:53:22Z

#223 should fix the CI issue. But that requires a consensus to actually move core to modules, which is a fairly big change - not code wise but api wise, since historically core has always been a profile.

nrdxp · 2021-03-28T05:04:16Z

Pushed two commits that fixed CI (even if #223 is merged, it's nice to know what actually caused the issue), and removed the call to builtins.toPath. Will try to work up some tests for these funcitons now.

nrdxp · 2021-03-28T05:05:00Z

bors try

bors · 2021-03-28T05:05:50Z

try

Build succeeded:

lib/devos/mkHosts.nix

Pacman99 · 2021-03-28T22:09:16Z

added a test for suites and profiles. Not sure how to do one for mkHosts, since that results in a nixosSystem which is really large, so writing the expected for that might not be possible..

nrdxp · 2021-03-29T05:49:02Z

mkHosts should be fine without testing, since its results are technically tested when the test VM is booted in profilesTest. Of course I would like to make some more elaborate tests in the future, which will all rely on the logic in mkHosts to provide the system environment.

An example that comes to mind is shutdown hang (I hate it when systemd hangs on shutdown, I'd at least like to know if an update to nixpkgs is gonna cause my system to do it).

store paths can't be appended to a path

to test suites and profile processing

Pacman99 · 2021-03-29T16:11:37Z

That makes sense, then hopefully the testSuites I added is enough.
I cleaned up git history, and since mkHosts test isn't needed, this ready for review.

Pacman99 · 2021-03-29T16:56:50Z

lib/devos/devosSystem.nix

+              # all strings in disabledModules get appended to modulesPath
+              # so convert each to list which is not a string but can be coerced to one
+              disabledModules = map (x: [ x ])
+                (lib.remove modules.core suites.allProfiles);


Best fix I could come up with to ensure the profiles still get disabled since we now pass profiles as strings again.

why this is necessary: https://github.com/NixOS/nixpkgs/blob/1dfe690209af14a91b32dbf2f379ca02d050e1a1/lib/modules.nix#L241

and nix doesn't allow store paths to be appended to relative paths

convert each to a list which doesn't get appended to modulesPath

nrdxp

bors r+

bors · 2021-03-29T17:21:35Z

Build succeeded:

Pacman99 commented Mar 26, 2021

View reviewed changes

This was referenced Mar 26, 2021

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

Closed

lib: init and use mkFlake and evalFlakeArgs #218

Closed

blaggacao reviewed Mar 27, 2021

View reviewed changes

lib/devos/mkHosts.nix Show resolved Hide resolved

Pacman99 force-pushed the hosts-suites branch from be22620 to 134a904 Compare March 27, 2021 17:59

blaggacao approved these changes Mar 27, 2021

View reviewed changes

bors bot added a commit that referenced this pull request Mar 27, 2021

Try #216:

eb49932

nrdxp force-pushed the hosts-suites branch from e827aec to 134a904 Compare March 28, 2021 04:45

bors bot added a commit that referenced this pull request Mar 28, 2021

Try #216:

05fab8d

Pacman99 commented Mar 28, 2021

View reviewed changes

lib/devos/mkHosts.nix Show resolved Hide resolved

Pacman99 force-pushed the hosts-suites branch from 006a060 to 496f0cf Compare March 29, 2021 16:05

Pacman99 added 5 commits March 29, 2021 09:05

lib/mkProfileAttrs: don't change to path

8ab865a

store paths can't be appended to a path

move suites implementation logic to lib as mkSuites

7ebd6b5

move hosts implementation logic to lib as mkHosts

ec7eff6

flake: update to use mkSuites and mkHosts

9270088

tests: init testSuites

259ec11

to test suites and profile processing

Pacman99 force-pushed the hosts-suites branch from 496f0cf to 259ec11 Compare March 29, 2021 16:05

Pacman99 commented Mar 29, 2021

View reviewed changes

isoConfig: fix disabling profiles

7650526

convert each to a list which doesn't get appended to modulesPath

Pacman99 force-pushed the hosts-suites branch from 7d0f4d4 to 7650526 Compare March 29, 2021 17:00

nrdxp approved these changes Mar 29, 2021

View reviewed changes

bors bot merged commit ed17d9e into divnix:core Mar 29, 2021

Pacman99 deleted the hosts-suites branch April 1, 2021 20:13

Move host and suite implementation logic to lib #216

Move host and suite implementation logic to lib #216

Conversation

Pacman99 commented Mar 26, 2021

Choose a reason for hiding this comment

blaggacao Mar 27, 2021 • 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

blaggacao Mar 27, 2021 • edited Loading

Choose a reason for hiding this comment

nrdxp Mar 27, 2021 • edited Loading

Choose a reason for hiding this comment

edit

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

nrdxp Mar 28, 2021 • edited Loading

Choose a reason for hiding this comment

blaggacao commented Mar 27, 2021 • edited Loading

Pacman99 commented Mar 27, 2021

blaggacao left a comment

Choose a reason for hiding this comment

Pacman99 commented Mar 27, 2021

blaggacao commented Mar 27, 2021 • edited Loading

Pacman99 commented Mar 27, 2021

Pacman99 commented Mar 27, 2021

blaggacao commented Mar 27, 2021 • edited Loading

blaggacao commented Mar 27, 2021

bors bot commented Mar 27, 2021

blaggacao left a comment • edited Loading

Choose a reason for hiding this comment

blaggacao commented Mar 27, 2021

bors bot commented Mar 27, 2021

Pacman99 commented Mar 27, 2021 • edited Loading

nrdxp commented Mar 27, 2021 • edited Loading

Update

2nd Update

blaggacao commented Mar 27, 2021

Pacman99 commented Mar 27, 2021

Pacman99 commented Mar 27, 2021

bors bot commented Mar 27, 2021

try

nrdxp commented Mar 27, 2021 • edited Loading

Pacman99 commented Mar 28, 2021

nrdxp commented Mar 28, 2021

Pacman99 commented Mar 28, 2021 • edited Loading

nrdxp commented Mar 28, 2021 • edited Loading

nrdxp commented Mar 28, 2021

bors bot commented Mar 28, 2021

try

Pacman99 commented Mar 28, 2021

nrdxp commented Mar 29, 2021

Pacman99 commented Mar 29, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nrdxp left a comment

Choose a reason for hiding this comment

bors bot commented Mar 29, 2021

blaggacao Mar 27, 2021 •

edited

Loading

blaggacao Mar 27, 2021 •

edited

Loading

nrdxp Mar 27, 2021 •

edited

Loading

nrdxp Mar 28, 2021 •

edited

Loading

blaggacao commented Mar 27, 2021 •

edited

Loading

blaggacao commented Mar 27, 2021 •

edited

Loading

blaggacao commented Mar 27, 2021 •

edited

Loading

blaggacao left a comment •

edited

Loading

Pacman99 commented Mar 27, 2021 •

edited

Loading

nrdxp commented Mar 27, 2021 •

edited

Loading

nrdxp commented Mar 27, 2021 •

edited

Loading

Pacman99 commented Mar 28, 2021 •

edited

Loading

nrdxp commented Mar 28, 2021 •

edited

Loading