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

Configurable derivations #6583

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from
Draft

Conversation

edolstra
Copy link
Member

WIP

To show all the options of a configuration:

# nix describe github:edolstra/configurables-test#nixosConfigurations2.test

To build a configuration:

# nix build github:edolstra/configurables-test#nixosConfigurations2.test

To override the host name from the command line:

# nix build github:edolstra/configurables-test#nixosConfigurations2.test --argstr networking.hostName xyzzy

To override the system type from the command line:

# nix build github:edolstra/configurables-test#nixosConfigurations2.test ---argstr nixpkgs.system i686-linux

@roberth
Copy link
Member

roberth commented May 28, 2022

Intriguing :)

Mutually exclusive or mix-in?

I think the usability would be better of configurability was a mix-in interface rather than an entirely different type.
That way, expression can use the default configuration as usual. This is especially true if the wiring of the configuration happens in the flake fixpoint, so that self.* is always the configured thing.

Overriding other parts of the flake

It'd also be more powerful to have this at the level of the flake fixpoint. This would allow the user to override a package in packages while deploying a nixosConfigurations.

A bonus feature would be to make these configurations first class, so they can be interpreted by the frameworks that build on Flakes, allowing the configuration of framework-specific concepts. This could be done by adding, say, self.configurationOverrides.<attrName>.<configurableItem> = <configuredValue>. Frameworks can declare that all possible <attrName>s are validated and handled by the framework by setting outputs.nixConfig.checkConfigurationOverridesAttrNames = false (or similar; maybe not nixConfig?), as this is already within their capability to do, without bothering the user.

nixosConfigurations meaning

I've always understood the nixosConfigurations as the final "real-world" configurations that are ready to be applied. Your example violates this understanding, making nixosConfigurations more like nixosModules.
We'd have:

  • nixosModules: services, image logic, profiles (I'd call them archetypes), complete but not instantiated system configs
  • nixosConfigurations: instantiated real systems, instantiated but not completely configured "real" systems

Configuring NixOS after the fact

While I'm not super happy about the fact that the nixosConfigurations are now almost conceptually redundant (if it wasn't for the choice of NixOS entrypoint (which source, which lib.nixosSystem-equivalent), I should point out a recent addition that will make your configurability a bit cleaner.

In the upcoming release (and recent backport to 21.11) it is possible to do

nixosConfigurations.foo = lib.nixosSystem { ..... };
nixosConfigurations.foo-with-hello =
  self.nixosConfigurations.foo.extendModules { modules = [
    { environment.systemPackages = [ pkgs.hello ]; }
  ]; };

This cleanly copies the effect of all lib.nixosSystem parameters to the new instance. That also fixes the little problem with specialArgs for you.

@edolstra
Copy link
Member Author

A bit off-topic, but I feel that .extendModules perpetuates the .override anti-pattern, where you have to call something (in this case a NixOS configuration) just to be able to override it.

It'd also be more powerful to have this at the level of the flake fixpoint. This would allow the user to override a package in packages while deploying a nixosConfigurations.

I think this would require a change to the flakes spec, since it would mean that the flake outputs attribute has to be a configurable (i.e. implement an interface that allows its configuration options to be discoverable and settable). But it should be possible to do in a backwards-compatible way.

@roberth
Copy link
Member

roberth commented May 30, 2022

A bit off-topic, but I feel that .extendModules perpetuates the .override anti-pattern, where you have to call something (in this case a NixOS configuration) just to be able to override it.

In this case it is slightly better. It exists on a rather cheap value that does not require any options to be computed; not even imports to be resolved. Now this is going to look like a party trick, but it actually demonstrates that this extension mechanism is lazier than the usual .override ones. The crux: without a value for later, the module fixpoint can not be computed, yet we don't get an error, so it has not been computed before producing the final configuration.

nix-repl> ((lib.evalModules { prefix = []; modules = [ ({ later, ... }: { imports = [ later ]; }) ]; }).extendModules { specialArgs.later = { }; }).config
{ }

The reason why this mechanism is lazy is that unlike most overrides, the returned set off attribute names does not depend on the actual values. It is always { _module, config, options, extendModules, type } so the only cost of overriding here is producing the other 4 thunks; next to nothing.

At first, I only intended extendModules for internal use in specialisations and such (through module arguments), but the surprisingly negligible cost made me reconsider that. In fact, this is the cheaper one, as when you use it through module arguments, you're by definition duplicating the configuration, as is required for such features as specialisations.

But it should be possible to do in a backwards-compatible way.

While backcompat is generally nice, this should be no issue as flakes are experimental. I'm all for it though.

OT: A solution for #3843 can probably also be backwards compatible, but after accumulating a bunch of backcompat, it seems like a good idea to clear most of that out, before finalizing the RFC that specifies the first non-experimental flakes release.

@roberth
Copy link
Member

roberth commented May 30, 2022

We've talked about generalizing this from output attributes to all outputs, but it could be generalized further to encompass the inputs as well. Doing so will solve NixOS/nixpkgs#166220, or I guess this PR already does that, but only for the specific case of the nixpkgs flake; not yet other flakes that depend on nixpkgs.

At that point, we might consider setting system with the same means, thereby solving #3843 in a way, although such a solution would suggest being able to work with only a single system though, at least when ignoring the ability to invoke the good old nixpkgs function outside the flake paradigm. This could be a bit of a problem for dependency flakes that expose data (such as docs or a website) which isn't buildable everywhere, but can be used everywhere by virtue of being data (which is in a binary cache, presumably). There are other uses cases for cross-system dependencies as well, such as for natively built VM images.

@edolstra
Copy link
Member Author

While backcompat is generally nice, this should be no issue as flakes are experimental. I'm all for it though.

Flake configurability is more of a flakes 2.0 feature. It shouldn't delay Nix 3.0.

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/tweag-nix-dev-update-31/19481/1

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/nixos-configuration-nix-with-argument/19653/4

@domenkozar
Copy link
Member

It would be cool if it accepted a JSON, as a file or a string.


auto type = cursor->getAttr(state->sType)->getString();

if (type != "configurable")
Copy link
Member

Choose a reason for hiding this comment

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

Maybe it's overloading the concept, but it seems like you might expect nix describe to also work on flakes.

Copy link
Member

@shlevy shlevy left a comment

Choose a reason for hiding this comment

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

Overall looks pretty good! The one big picture question I have, referenced in a few of my comments: Why limit configurability to derivations? Why not configurable apps, configurable NixOS configurations, etc.?

I may pick this PR up and open a PR against your repo bringing it up to date with master and completing the work (documentation, testing, etc.)

for (const auto & [n, s] : enumerate(ss)) {
if (n + 1 == ss.size()) {
if (pos->children.find(s) != pos->children.end())
throw Error("definition of '%s' clashes with a previous definition", state->symbols[attr.name]);
Copy link
Member

Choose a reason for hiding this comment

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

Many tools with non-repeatable flags have the convention that the later can be used to override the former. Is there a reason we went with this approach instead?

}
}

std::function<Value * (const ValueTree & tree)> buildAttrs;
Copy link
Member

Choose a reason for hiding this comment

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

Might be worth a comment that this declared here for recursive calling, it surprised me.


ValueTree tree;

if (cmd) {
Copy link
Member

Choose a reason for hiding this comment

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

If we want other things besides drvs/installables to be configurable, we should probably extract everything from here to line 713 into a separate function that can be used for other interfaces.

auto vArgs = buildAttrs(tree);

auto vRes = state->allocValue();
state->callFunction(build, *vArgs, *vRes, noPos);
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't the pos be wherever we found attr?

}

if (outputsToInstall.empty())
outputsToInstall.insert("out");
else if (type == "configurable") {
Copy link
Member

Choose a reason for hiding this comment

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

If we want other things besides drvs/installables to be configurable, should we allow type to be some kind of structured value? E.g. [ "configurable" "derivation" ] or { configurable = true; configured = "derivation"; } or something?

The alternative is to dynamically resolve this, i.e. check type after resolving the configurable.

@stale stale bot removed stale labels Aug 14, 2023
@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/experimental-does-not-mean-unstable-detsyss-perspective-on-nix-flakes/32703/2

@ggPeti
Copy link
Member

ggPeti commented Jan 17, 2024

made a workaround

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants