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

Check if options exist before setting them #226

Closed
wants to merge 1 commit into from

Conversation

Pacman99
Copy link
Member

@Pacman99 Pacman99 commented Apr 1, 2021

mkHosts is not very portable since it relies on nixos options. Which means in the future it can't be used on nix-darwin or robotnix. So here I just wrapped every option thats set to first check if it exists.

I would also like to set the precedent for future integrations added into devos to be made optional with similar methods:

  • Don't add a package unless it exists, ex: pkgs ? deploy-rs
  • Don't set an option unless it exists, ex: options ? home-manager. This one is tricky, it can't be done with mkIf. You have to use a combination of mkMerge and optionalAttrs, the former is usually necessary when your setting other things and the same module and you want to set an option that might not exist. See changes in devosSystem and mkHosts for examples.
  • Don't export an output that depends on a specific input, unless the input exists: ex: inputs ? deploy
  • Don't test relevant outputs unless they exist, ex: self ? homeConfigurations. Not sure what to do about this one though, most tests are tightly integrated with devos.


system.configurationRevision = lib.mkIf (self ? rev) self.rev;
}
(lib.optionalAttrs (options ? home-manager) {
Copy link
Member Author

Choose a reason for hiding this comment

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

This can't just be // optionalAttrs ..., that causes an infinite recursion error.

Copy link
Member Author

Choose a reason for hiding this comment

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

@blaggacao
Copy link
Contributor

blaggacao commented Apr 10, 2021

Does process on gytis-ivaskevicius/flake-utils-plus#18 feed any information back on this?

@Pacman99
Copy link
Member Author

I actually did that PR with knowledge I gained from working on this.

@blaggacao
Copy link
Contributor

blaggacao commented Apr 10, 2021

Just realized, once we implement config generators in a lib, that lib ought to bring its own dependencies. The use of deploy-rs or home-manager will then be optional be the way the user interfaces with that lib's api. Should we close this?

@Pacman99
Copy link
Member Author

I think the code in mkHosts, is relevant, where we set the home-manager defaults if the option exists. And there are a couple other things, that are useful, for example deploy might not be in the users packages, so we don't want to add it to shell, unless it exists.

@nrdxp
Copy link
Collaborator

nrdxp commented Apr 10, 2021

Is this still an issue/needed?

@blaggacao
Copy link
Contributor

blaggacao commented Apr 10, 2021

We should put it on-hold until we have devos lib and clarity of which dependency can be expected where, we might else get dead code out of this while refactoring.

@Pacman99
Copy link
Member Author

Yeah most of this is unnecessary with the move to pkgs-lib. I think we have now forced lib to depend on deploy-rs.
But I do want to keep this PR for the goal of making home-manager to be optional. I think my other goal for this PR was to have a demonstration of how integrations can be optional to encourage it in the future.

@Pacman99 Pacman99 changed the title Make home-manager and deploy-rs integrations optional Check if options exist before setting them Apr 11, 2021
@blaggacao blaggacao changed the base branch from core to develop April 22, 2021 00:03
@Pacman99 Pacman99 marked this pull request as draft April 24, 2021 06:50
@Pacman99
Copy link
Member Author

Pacman99 commented Apr 24, 2021

planning to rework mkHosts, so I might include these changes in that PR. But we'll see where it goes.

@Pacman99 Pacman99 closed this Apr 26, 2021
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.

3 participants