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

Configuration file formats for JSON, INI, YAML and TOML #75584

Merged
merged 6 commits into from Aug 2, 2020

Conversation

@Infinisil
Copy link
Member

@Infinisil Infinisil commented Dec 12, 2019

Motivation for this change

This is a main part of the implementation of NixOS/rfcs#42 (see https://github.com/Infinisil/rfcs/blob/config-option/rfcs/0042-config-option.md#part-1-1).

Since that part of the RFC is fairly uncontroversial, I think we can merge this before the RFC is eventually accepted. This allows people to use such types in their NixOS options already.

Ping @Profpatsch @rycee @aanderse @kampka

Things done
  • Tested that these types and generator combinations indeed can be used in modules.
  • Write some tests
  • Wrote documentation on how to use them..

Here's a simple example module though:

{ lib, config, pkgs, ... }:
let
  format = pkgs.formats.json {};
in {
  options.settings = lib.mkOption {
    type = format.type;
    default = {};
  };

  config = {
    settings.foo = 10;
    settings.bar.baz = "Hello";
    environment.etc.test.source = format.generate "config.json" config.settings;
  };
}
@Infinisil
Copy link
Member Author

@Infinisil Infinisil commented Dec 12, 2019

Potentially/Eventually these formats might be implemented using lazyAttrsOf (or what becomes of it) from #70138, such that self-dependent assignments of them will be possible. Edit: This is now done in this PR :)

@aanderse
Copy link
Contributor

@aanderse aanderse commented Dec 15, 2019

@Infinisil awesome stuff!

lib/settings-formats.nix Outdated Show resolved Hide resolved
lib/settings-formats.nix Outdated Show resolved Hide resolved
@Infinisil
Copy link
Member Author

@Infinisil Infinisil commented Dec 22, 2019

(This still needs a bit of docs probably and a history cleanup at least btw, before anybody merges)

@Infinisil Infinisil mentioned this pull request Jan 1, 2020
0 of 10 tasks complete
@Infinisil Infinisil force-pushed the Infinisil:settings-formats branch from e5661da to 3974642 Jan 9, 2020
@Infinisil
Copy link
Member Author

@Infinisil Infinisil commented Jan 9, 2020

I've now rebased this ontop of #70138 to implement types where values can depend on other values. E.g. this previously threw an infinite recursion error, but now it doesn't anymore:

{ lib, config, ... }:
let format = lib.formats.json;
in {
  options.settings = lib.mkOption {
    type = format.type;
    default = {};
  };

  config = {
    settings.foo = true;
    settings.bar = lib.mkIf config.settings.foo "Foo is true!";

    environment.etc.test.text = format.generate config.settings;
  };
}

Due to how the implementation works, it's however a bit more cumbersome if you aren't sure whether an attribute exists at all. This is how that can now be handled gracefully:

{
  # To make sure the `.name` attribute exists
  settings.name = format.empty;
  # To handle the case of `.name` being empty
  settings.greeting = "Hello, ${format.get "nobody" config.settings.name}!";
}

This will generate { greeting = "Hello, nobody!"; }

Alternatively if you don't mind name being set:

{
  settings.name = lib.mkDefault "nobody";
  settings.greeting = "Hello, ${config.settings.name}!";
}

Which will generate { name = "nobody"; greeting = "Hello, nobody!"; }

@Infinisil Infinisil changed the title Configuration file formats for JSON and INI Configuration file formats for JSON, INI and YAML Jan 9, 2020
@Infinisil Infinisil force-pushed the Infinisil:settings-formats branch from 3974642 to 3501efa Jan 10, 2020
samueldr added a commit to samueldr-wip/mobile-nixos-wip that referenced this pull request Jan 13, 2020
samueldr added a commit to samueldr-wip/mobile-nixos-wip that referenced this pull request Jan 13, 2020
samueldr added a commit to samueldr-wip/mobile-nixos-wip that referenced this pull request Jan 13, 2020
samueldr added a commit to samueldr-wip/mobile-nixos-wip that referenced this pull request Jan 14, 2020
@Profpatsch Profpatsch self-requested a review Jan 15, 2020

empty = { _nixos_empty_json_value =
throw ("You tried to access an option value of type json that is empty. "
+ "Use `lib.formats.json.get` on the value before using it.");

This comment has been minimized.

@Infinisil

Infinisil Jan 15, 2020
Author Member

It might be a good idea to only have a single empty value for all

@Infinisil
Copy link
Member Author

@Infinisil Infinisil commented Jan 15, 2020

On IRC @adisbladis brought to my attention that to support more formats like TOML, we could use builtins.toJSON and convert that to toml with a build-time tool, meaning the generated strings won't be available to nix at eval time anymore and the generate functions have the signature generate :: Value -> Derivation. This doesn't seem like a problem: Nobody should need the raw Nix strings anyways, and this will speed up evaluation too.

Copy link
Member

@Profpatsch Profpatsch left a comment

Haven’t reviewed in-depth, but if other people are okay with it LGTM

@Profpatsch Profpatsch dismissed their stale review Jul 27, 2020

seems to be resolved

@aanderse aanderse mentioned this pull request Jul 27, 2020
2 of 10 tasks complete
@Infinisil
Copy link
Member Author

@Infinisil Infinisil commented Jul 29, 2020

I think this PR is in a good state now. Unless somebody has any improvement suggestions or concerns I'll merge it in its current state this weekend!

@domenkozar
Copy link
Member

@domenkozar domenkozar commented Jul 29, 2020

I'd review it if it was written with terminal length in mind, but currently it's not feasible via github ui.

Copy link
Contributor

@roberth roberth left a comment

It should be noted in the docs that some of these functions may write to the store, which is not safe when Nix is handling secrets. This is in contrast to builtins.toJSON etc, which operate entirely in memory.

lib/generators.nix Show resolved Hide resolved
# they depend on some packages. This notably is *not* for supporting package
# building, instead pkgs/build-support is the place for that.
{ lib, pkgs }: {
# setting format types and generators. These would fit in lib/types.nix, but

This comment has been minimized.

@roberth

roberth Jul 29, 2020
Contributor

Suggested change
# setting format types and generators. These would fit in lib/types.nix, but
# setting format types and generators. These do not fit in lib/types.nix, because
@@ -0,0 +1,11 @@
# pkgs-lib is for things that should be in lib, but can't beceause

This comment has been minimized.

@roberth

roberth Jul 29, 2020
Contributor

Suggested change
# pkgs-lib is for things that should be in lib, but can't beceause
# pkgs-lib is for functions that can't be in lib, because
@@ -75,6 +75,11 @@ let
inherit (self) runtimeShell;
};

pkgsLib = self: super: import ../pkgs-lib {

This comment has been minimized.

@roberth

roberth Jul 29, 2020
Contributor

An overlay is unnecessary because the same could be achieved by putting formats in all-packages.nix.

Categorizing functions separately from packages is beyond the scope of this change. It can be discussed in a separate issue/pr, although I doubt its usefulness.

This comment has been minimized.

@Infinisil

Infinisil Jul 29, 2020
Author Member

Doesn't have to be just functions though, can be arbitrary values, and other functions like fetchgit wouldn't fit in a pkgs-lib. I guess it won't be a problem to just put formats in all-packages.nix, but imo having a separation between "things needed to create packages" and "things that just happen to depend on pkgs" would make sense. Could still introduce that in the future though

This comment has been minimized.

@Infinisil

Infinisil Jul 29, 2020
Author Member

Not using an overlay anymore now

This comment has been minimized.

@roberth

roberth Jul 29, 2020
Contributor

All of formats, fetchgit, dockerTools depend on Nixpkgs, take arguments, produce derivations, are maintained and released in the same way. What makes formats so architecturally different that it needs to be treated differently?

imo having a separation between "things needed to create packages" and "things that just happen to depend on pkgs" would make sense. Could still introduce that in the future though

If only one attribute is separate it doesn't make sense though. For it to make sense it must be applied to the whole of Nixpkgs and it must have practical benefits. The prior can be done in the future and I'm skeptical about the benefits, but would love to be proven wrong in that same future.

For now please follow the example of, say, dockerTools.

This comment has been minimized.

@roberth

roberth Jul 29, 2020
Contributor

Looks like my view of the PR was outdated (maybe caused by the current service disruption?) anyway, thanks for removing the overlay. Currently build-support has the role of pkgs-lib so you may want to move the files to build-support/formats. I don't really like that name either so I'm going to leave this change up to you to decide.

This comment has been minimized.

@Infinisil

Infinisil Jul 29, 2020
Author Member

The difference between e.g. fetchgit and formats.json is that fetchgit is needed for building many of the pkgs.* derivations, whereas formats.json doesn't have any use for any pkgs.*. Instead formats are mostly just useful for NixOS modules (really, the module system in general, so it wouldn't fit in nixos/lib)

This is also why I don't think build-support would make sense, because formats.json doesn't support any package building.

@worldofpeace
Copy link
Member

@worldofpeace worldofpeace commented Jul 29, 2020

I'd review it if it was written with terminal length in mind, but currently it's not feasible via github ui.

this is useful https://github.com/StylishThemes/GitHub-code-wrap

@rycee
Copy link
Member

@rycee rycee commented Jul 29, 2020

An alternative that doesn't need any installations is to use the "Split" diff option. For whatever reason, it seems to wrap lines while "Unified" doesn't.
github-split

@Infinisil
Copy link
Member Author

@Infinisil Infinisil commented Jul 29, 2020

It should be noted in the docs that some of these functions may write to the store, which is not safe when Nix is handling secrets. This is in contrast to builtins.toJSON etc, which operate entirely in memory.

For NixOS modules at least it doesn't make much of a difference, because the only way for a string (which toJSON returns) to influence a system build is by putting it in a derivation. With the interface this PR adds, this step is just done a little earlier. But yeah it can't hurt to point this out, I'll add a note

Infinisil added 5 commits Dec 12, 2019
@Infinisil Infinisil force-pushed the Infinisil:settings-formats branch from 76e8e4a to 83b1688 Jul 29, 2020
@Infinisil
Copy link
Member Author

@Infinisil Infinisil commented Jul 31, 2020

@roberth Looking good now?

@roberth
roberth approved these changes Aug 2, 2020
@roberth roberth merged commit 150bf4f into NixOS:master Aug 2, 2020
15 checks passed
15 checks passed
tests
Details
Evaluation Performance Report Evaluator Performance Report
Details
grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-maintainers matching changed paths to changed attrs...
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-darwin nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="83b1688"; rev="83b16885f526d6ab7b39e98159e2b48024f3238c"; } ./pkgs/t
Details
grahamcofborg-eval-lib-tests nix-build --arg pkgs import ./. {} ./lib/tests/release.nix
Details
grahamcofborg-eval-nixos nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="83b1688"; rev="83b16885f526d6ab7b39e98159e2b48024f3238c"; } ./nixos/
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="83b1688"; rev="83b16885f526d6ab7b39e98159e2b48024f3238c"; } ./nixos/
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="83b1688"; rev="83b16885f526d6ab7b39e98159e2b48024f3238c"; } ./nixos/
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="83b1688"; rev="83b16885f526d6ab7b39e98159e2b48024f3238c"; } ./pkgs/t
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="83b1688"; rev="83b16885f526d6ab7b39e98159e2b48024f3238c"; } ./pkgs/t
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="83b1688"; rev="83b16885f526d6ab7b39e98159e2b48024f3238c"; } ./pkgs/t
Details
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
grahamcofborg-eval-package-list-no-aliases nix-env -qa --json --file . --arg config { allowAliases = false; }
Details
@Ericson2314
Copy link
Member

@Ericson2314 Ericson2314 commented Aug 2, 2020

I wonder if pkgs-lib should be pkgs/lib to match nixos/lib.

@Infinisil
Copy link
Member Author

@Infinisil Infinisil commented Aug 2, 2020

Awesome, thanks for merging!

@Ericson2314 I intentionally didn't name it lib because it could easily be confused with pkgs.lib.

@Infinisil Infinisil deleted the Infinisil:settings-formats branch Aug 2, 2020
@Infinisil Infinisil mentioned this pull request Aug 3, 2020
3 of 9 tasks complete
@Infinisil Infinisil mentioned this pull request Aug 22, 2020
4 of 10 tasks complete
@midchildan midchildan mentioned this pull request Sep 1, 2020
4 of 9 tasks complete
@Infinisil Infinisil mentioned this pull request Sep 3, 2020
10 of 17 tasks complete
vcunat referenced this pull request Oct 19, 2020
Foremost, the message was discarding double quotes on one side of the
diff, which was super-confusing to me, as I thought that the format
convertor broke that when in fact only whitespace was changed.

I thought I'd cat the files, but then... switching to `diff -u` seemed
self-sufficient.  It felt sufficiently non-controversial to push
directly, but certainly feel free to improve further.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.