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

add hosts module arg #164

Merged
merged 1 commit into from
Mar 16, 2021
Merged

add hosts module arg #164

merged 1 commit into from
Mar 16, 2021

Conversation

Pacman99
Copy link
Member

@Pacman99 Pacman99 commented Mar 14, 2021

should help with #163. Fixes #169
But either way this could be generally useful. I have one use case of setting up a minecraft bungeecord proxy with servers on different hosts.

@nrdxp
Copy link
Collaborator

nrdxp commented Mar 14, 2021

This looks fairly benign, perhaps a usage example in the docs would be pertinent

hosts/default.nix Outdated Show resolved Hide resolved
@Pacman99
Copy link
Member Author

Agreed a usage example would be nice. I'm not sure where to put one. This change could be useful for profiles, modules, and hosts. But I guess hosts would still be the best place to include it.

@nrdxp
Copy link
Collaborator

nrdxp commented Mar 14, 2021

Actually, I was wanting to add a section to the docs, something like Common Usecases or something to that effect. Perhaps it would be best there.

@Pacman99
Copy link
Member Author

Yeah that would be perfect

@Pacman99
Copy link
Member Author

If we do add multiPkgs in the other PR, we could document both as special arguments available to hosts.

@Pacman99 Pacman99 force-pushed the export-hosts branch 4 times, most recently from d2358f7 to 24505ab Compare March 15, 2021 15:58
@Pacman99
Copy link
Member Author

switched to use _module.args based on comment in nixpkgs/lib/modules.nix. hosts arg should never be used in an imports line - evaluated modules can't be imported again,.

@Pacman99 Pacman99 changed the title add hosts specialArg to allow for modules to reference other host's configuration add hosts module argument to allow for modules to reference other host's configuration Mar 15, 2021
@Pacman99 Pacman99 force-pushed the export-hosts branch 2 times, most recently from 215b3c9 to 0144da6 Compare March 15, 2021 16:54
@Pacman99 Pacman99 changed the title add hosts module argument to allow for modules to reference other host's configuration expose self and inputs to lib and hosts Mar 16, 2021
@Pacman99
Copy link
Member Author

Pacman99 commented Mar 16, 2021

I increased the scope of this PR to cover #169. Now lib functions can access self and any input. And inputs is passed as a special arg, since that could be used in an imports statement. Self is passed as a module arg since I can't see a reason to why it would be part of an imports statement.

@blaggacao
Copy link
Contributor

blaggacao commented Mar 16, 2021

LGTM

I think it might be still too early for a smdocumented usage example. As I understand it, this idea came up towards a possible way reacting to another hosts configuration. That is mostly it's identity (hostname, ip, systemd machine id, — maybe static link local ipv6?)

I think you need to rebase?

@Pacman99
Copy link
Member Author

Yeah exactly, although with self and inputs I think this PR has more value. It is now an alternative method of passing inputs to your hosts instead of extern. So for mobile-nixos you can't use the extern modules section, because you can't add the pinephone module to all hosts. So using the inputs specialArg would be better. You can also use self.packages for cross compiling.

I don't know much about wireguard, but I would imagine that you might need to know information about other hosts to create the config for each host.

@blaggacao
Copy link
Contributor

I carved out the self part in #189 so that we can reason about inputs and hosts independently and also to unblock #168

@Pacman99 Pacman99 changed the title expose self and inputs to lib and hosts Pass all inputs to ./lib and ./hosts and add hosts module arg Mar 16, 2021
@Pacman99
Copy link
Member Author

I think either way we're blocked due to the ci issues. And I think this PR is pretty much ready to merge.

@Pacman99 Pacman99 changed the title Pass all inputs to ./lib and ./hosts and add hosts module arg Pass self and all inputs to ./lib and ./hosts and add hosts module arg Mar 16, 2021
flake.nix Outdated
@@ -57,7 +57,7 @@
overlay = import ./pkgs;
overlays = lib.pathsToImportedAttrs (lib.pathsIn ./overlays);

lib = import ./lib { inherit nixos pkgs; };
lib = import ./lib (inputs // { inherit nixos pkgs self; });
Copy link
Member Author

@Pacman99 Pacman99 Mar 16, 2021

Choose a reason for hiding this comment

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

I was debating over doing this or just making it available as inputs. I prefer this way so a lib function could just do something like { nix-darwin, ...}: and get access to nix-darwin.
The alternative would be { inputs, ... }: inputs.nix-darwin.lib.darwinSystem

Copy link
Member Author

@Pacman99 Pacman99 Mar 16, 2021

Choose a reason for hiding this comment

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

And also a lib function could do args@{ ... }: and remove pkgs self and lib to get access to all inputs. This is probably only useful for #190

Copy link
Contributor

@blaggacao blaggacao Mar 16, 2021

Choose a reason for hiding this comment

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

Maybe let's keep it name spaced as inputs. Similar to self this is well-known self-explaining top level flake glossary.

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 personally prefer this method because it feels more in alignment with the callPackage paradigm - or in this case callLib. You specify the things you need for your function and you get those inputs. If we just pass inputs itself, then you take everything, which is more similar to the module system.
But I'm ok with either, they both get the job done.

Copy link
Contributor

Choose a reason for hiding this comment

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

hm, for the sake of consistency, we probably shouldn't import it differently than how we import it with ./hosts, though.

Copy link
Member Author

Choose a reason for hiding this comment

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

hm, for the sake of consistency, we probably shouldn't import it differently than how we import it with ./hosts, though.

But do we really need consistency between lib and hosts? they are two different things.

@nrdxp Which one would you prefer? I think once decide how to pass inputs to lib, this should be ready.

Copy link
Contributor

@blaggacao blaggacao Mar 16, 2021

Choose a reason for hiding this comment

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

nix.registry = mapAttrs (n: v: { flake = ${v}; }) inputs; look clearer, indeed 😉

No need to be aware if some additional unwanted attributes sneak in and cause nasty side effects in unexpected places.

(over: nix.registry = mapAttrs (n: v: { flake = ${v}; }) (removeAttrs inputs [ "self" "pkgs" "lib" ]);)

@Pacman99 Pacman99 changed the title Pass self and all inputs to ./lib and ./hosts and add hosts module arg Pass all inputs to ./lib and ./hosts and add hosts module arg Mar 16, 2021
@Pacman99
Copy link
Member Author

So this PR now only covers passing inputs to lib and hosts. And it adds the hosts module arg as a convenience.

@blaggacao
Copy link
Contributor

Do we actually want to pass inputs to ./hosts? see #190 (comment)

@Pacman99
Copy link
Member Author

Pacman99 commented Mar 16, 2021

My reasoning is theres no harm in doing so. And it helps with flakes containing nixosModules that you only want in one host.

And extern doesn't cover all use cases with inputs and having inputs part of the module system accounts for pretty much any use case.

@blaggacao
Copy link
Contributor

blaggacao commented Mar 16, 2021

And it helps with flakes containing nixosModules that you only want in one host.

Is this an issue despite lazy evaluation? (sorry my ignorance)

And extern doesn't cover all use cases with inputs

Did you have any specific use case in mind?

My reasoning is theres no harm in doing so.

Well, it dilutes the interface. I don't think that's good on such a general basis which affects ./profiles and ./modules downstream. Or at least we should have a clear picture of what our interfaces are here.

I mean all of a sudden, we have to ways to accomplish the same. This is a recipe for trouble.

EDIT: Until we have our interfaces sorted out, I'm leaving the ./hosts parts out for now in #191

@Pacman99
Copy link
Member Author

I removed that part. I realize its a pretty niche use case and also I can just do it manually in extern with an inherit inputs;.

@Pacman99 Pacman99 changed the title Pass all inputs to ./lib and ./hosts and add hosts module arg Pass all inputs to ./lib and add hosts module arg Mar 16, 2021
@Pacman99 Pacman99 changed the title Pass all inputs to ./lib and add hosts module arg add hosts module arg Mar 16, 2021
@blaggacao
Copy link
Contributor

Thank you @Pacman99 for all this inspiration and thought fodder.

Interaction / discussion has it all! ❤️

@Pacman99
Copy link
Member Author

Thank you @Pacman99 for all this inspiration and thought fodder.
Interaction / discussion has it all! heart

Indeed it does! Thank you too!

@Pacman99
Copy link
Member Author

Also this is back to its original state, just adds hosts module arg. So I think its ready to merge.

@nrdxp
Copy link
Collaborator

nrdxp commented Mar 16, 2021

bors delegate+

@bors
Copy link
Contributor

bors bot commented Mar 16, 2021

✌️ Pacman99 can now approve this pull request. To approve and merge a pull request, simply reply with bors r+. More detailed instructions are available here.

@Pacman99
Copy link
Member Author

bors r+

@bors
Copy link
Contributor

bors bot commented Mar 16, 2021

👎 Rejected by too few approved reviews

@nrdxp nrdxp self-requested a review March 16, 2021 23:56
Copy link
Collaborator

@nrdxp nrdxp left a comment

Choose a reason for hiding this comment

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

bors r+

@bors
Copy link
Contributor

bors bot commented Mar 16, 2021

Build succeeded:

@bors bors bot merged commit c2cd3e7 into divnix:core Mar 16, 2021
@Pacman99 Pacman99 deleted the export-hosts branch March 17, 2021 00:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pass 'self' to lib
3 participants