Shouldn't this be just a library function? It's a useful algorithm, but I doubt that it should be a type like this. It forces the attribute names like before and after to be generic and not very useful as shown by rycee's request to rename. However, renaming will make this type hard to reuse. Also the type description DAG of ${t} is not helpful at all. If you'd replace such a use case by an attrsOf (submodule ... ) and an assertion that calls the library function, it will enforce the same rules and we'll have a much better user experience.

for representing directed acyclic graphs

I don't know why, but every time I hear about acrylic graphs I think of like nail extensions 🤣

@roberth I guess I can make this a library function in addition as well, though the algorithm is really just a simple wrapper around lists.toposort.

I think it makes sense for it to be a type, because that's the most convenient way to use it. It takes care of a bunch of boilerplate:

• The type returns an ordered list of the result already (in contrast with attrsOf), so the module author doesn't need to call the sorting function where it's used. This also saves the module author from having to throw an error for cycles.
• The submodule type for the data, before and after options is very generic and makes sense in all DAG's. By having this predefined the module author doesn't need to create those options themselves.

@infinisil Let's see if we can improve this then. My concern is that a small convenience for the module author comes at the cost of usability for, well, users.

Perhaps a usage by the module author could look like this:

type = types.dagOf { predecessors = v: v.after; successors = v: v.before; } (types.submodule { options.before = .....; ..... })

• types.dag now uses DAG terminology, to avoid any confusion with the actual domain
• data is gone 🎉
• module author can now choose submoduleWith or reuse a submodule type
• module author can write domain-specific descriptions for options like before (the current hardcoded ones are ambiguous or wrong, as can be expected to happen when you're juggling generic stuff)
• the arguments passed here can probably be the defaults, let's see

Systemd works like this:

If unit foo.service contains the setting Before=bar.service and both units are being started, bar.service's start-up is delayed until foo.service has finished starting up.

This seems to be the inverse of what this PR is doing (unless I'm not mapping between the domains correctly??). To quote this PR's before doc:

The given entries should be ordered before this one.

Am I reading this right? All I know for sure is that the user should get to read docs like the first of the two.

@roberth Oops I think I wrote that doc description the wrong way around. I'll give your idea some thought though, I think I like it!

Changed this a bunch, now it's a lot simpler, and only requires giving a before relationship. See the tests for how such a before looks for after and before-style options.

I’m having doubt whether allowing arbitrary turing complete predicates (aka before) is the best way to model DAGs. There’s no way to have any static introspection in the ordering short of actually running all the before functions.

In particular, I don’t know if you can actually have any guarantees about the result being acyclic by modeling it this way?

Also properties I would expect from a DAG, like “if B is a subtree and A is before B, then every child of B is after A” are not a given.

I’m having doubt whether allowing arbitrary turing complete predicates (aka before) is the best way to model DAGs. There’s no way to have any static introspection in the ordering short of actually running all the before functions.

Nix is a lazy Turing complete language, which means that behind everything that "looks like data" is a thunk that may evaluate to bottoms like exceptions or the infinite recursion you seem to want to prevent somehow.
That said, an "adjacency list" representation, like systemd.*.*.before, may lend itself to a more efficient implementation. We don't seem to have needed a faster implementation of toposort, as far as I know.

In particular, I don’t know if you can actually have any guarantees about the result being acyclic by modeling it this way?

The type converts the definitions to a list that is sorted according to the before function. Such an outcome is not possible when the input is cyclic, so the non-exception result is guaranteed to be acyclic.

The type converts the definitions to a list that is sorted according to the before function. Such an outcome is not possible when the input is cyclic, so the non-exception result is guaranteed to be acyclic.

Yes, but it would mean an “infinite recursion” in practice, right? Which is not very user-friendly.

Plus, this is still an issue in my mind:

Also properties I would expect from a DAG, like “if B is a subtree and A is before B, then every child of B is after A” are not a given.

Yes, but it would mean an “infinite recursion” in practice, right? Which is not very user-friendly.

toposort produces a distinct attrset describing the cycle, which is turned into an exception by types.orderOf.

It would be nice to have a check function though.

Also properties I would expect from a DAG, like “if B is a subtree and A is before B, then every child of B is after A” are not a given.

That's what this type is all about, to ensure that either it will be the case or an error is raised. Perhaps you could formulate your concern differently or provide an example that is handled poorly by this type and tell us what you would expect it to do instead.

That's what this type is all about, to ensure that either it will be the case or an error is raised.

Which is what I’m not sure about. I looked at the implementation of toposort, but I don’t know what happens if arbitrary predicates are passed to it. Maybe it’s fine.

Maybe more importantly: Do we have applications for this type?

Ignoring backwards compatibility, this could be used for:

• The fileSystems option (see https://github.com/NixOS/nixpkgs/blob/master/nixos/modules/tasks/filesystems.nix#L13)
• The activationScripts option (see https://github.com/NixOS/nixpkgs/blob/master/nixos/modules/system/activation/activation-script.nix#L85-L90), currently uses lib/strings-with-deps.nix, which only supports specifying predecessors, no successors. Could also be used for home-manager's activation scripts: https://github.com/nix-community/home-manager/blob/master/modules/home-environment.nix#L311-L320
• Anywhere where you want to have an ordered list. This is a good replacement for types.listOf in such a case, because types.listOf doesn't let you override entries, while this type does. Example in home-manager: https://github.com/nix-community/home-manager/blob/master/modules/programs/ssh.nix#L414-L428. This type is the start of my efforts to deprecate listOf actually :)

After some discussion on IRC, here is my take on this.

I think we need an abstraction over this, because, as @infinisil stated, it could be used in few places in nixpkgs, instead of having, shall I call them hacks?, stuff like lib/strings-with-deps.nix which makes it hard to determine a definitive place for where your entry will end up[1].

This abstraction can be, in my opinion, anything, as long as it is just generic enough for wide usage, but not too generic so modules don't have to re-implement too much (untested) logic in their definition.

Which brings me to my next point, testing. Having an abstraction also means that it can be thoroughly tested and vetted. If each module that needs this starts to re-implement its own logic for this, we will probably end up with some of them not getting it quite right and edge cases where the resulting configuration will not be the expected result.

Onto an example, I am using this for #105319. The relevant places to look are:

• the declaration of the option using orderOf
• how it is defined
• how to override it from your configuration: security.pam.services.login.account.krb5.order = mkForce 1500; (forget about the mkForce, the orders might just be mkDefaulted in the end)

I wouldn't have wanted to re-implement the logic proposed here. As it turns out, it was available as a type, but it would have worked just as well with a function to call inside the apply argument of the option using it.

@rissson Did you plan to migrate the pam modules to a partial ordering? As things stand, you don't need partial ordering functionality and the much simpler and more efficient lib.sort can do the job for you.

Doesn't mean that orderOf doesn't work, but it may mean that we should rename this to partialOrderOf for clarity.

Did you plan to migrate the pam modules to a partial ordering?

No, indeed.

Doesn't mean that orderOf doesn't work, but it may mean that we should rename this to partialOrderOf for clarity.

Might be an idea.

I marked this as stale due to inactivity. → More info