pkgs.formats: Add libconfig format generator#246115
Conversation
47f5b87 to
de61b18
Compare
|
@ofborg eval |
de61b18 to
453626c
Compare
0a85ef2 to
f535080
Compare
ckiee
left a comment
There was a problem hiding this comment.
Looks good, but the commit prefix should probably be pkgs-lib or formats.libconfig, there's no precedent I can see for pkgs.formats.* in the git log.
|
Seems like people are either using |
f535080 to
66c96ee
Compare
|
It should still be tested to see if it works well against logiops and sslh. |
roberth
left a comment
There was a problem hiding this comment.
Some suggestions that should make it work with a cross-compiling Nixpkgs.
66c96ee to
5793a35
Compare
|
Builds successfully for me on |
|
@roberth can you finish your review? I'd like to get this merged because it's blocking other PRs. |
fe443a5 to
b9850de
Compare
There was a problem hiding this comment.
Does the library support pretty printing?
Easily readable files are not technically required, but if you do need to open one of these, it would be a lot nicer with at least a few more line breaks. Troubleshooting is annoying enough as is ;)
(but only do it if the library can do it for you)
There was a problem hiding this comment.
libconfigs config_write_file function does seem to pretty print by default. I think this would be easy to do in the rust program as well, if we want to avoid piping stuff through several binaries? Not keen on doing FFI.
Would it be okay to add as an argument to the format options, like pkgs.formats.libconfig { pretty = true; }?
There was a problem hiding this comment.
A pretty option would ~2x the testing and make the diff even bigger. I'm in favor of hardcoding it for pretty output.
(We are not using a library to serialize to libconfig, only validating against libconfig.)
b9850de to
89378d9
Compare
|
Can we get this done before the NixOS 23.11 freeze, please? |
89378d9 to
5c5e2f3
Compare
|
I think that's a good cause to skip implementing the It shouldn't be hard to implement later, in the Rust serializer. The (self-imposed!) deadline is Oct 30. Upon pulling & rebasing onto the latest nixos-unstable I was met with the [libconfig] validation ok
installing
[…]
running tests
--- /nix/store/s9fpgmdz5zyabk4wh69vpky01qcdf2yg-expected.txt 1970-01-01 00:00:01.000000000 +0000
+++ /nix/store/kj4x8rdxk27bxcshy6c7hn7r0d6nb4br-libconfig-test.cfg 1970-01-01 00:00:01.000000000 +0000
@@ -1,6 +1,6 @@
array1d=[1, 5, 2];array2d=([1, 2], [2, 1]);list1d=(1, "mixed!", 5, 2);list2d=(1, (1, 1.2, "foo"), ("bar", 1.2, 1));nasty_string="\"@
\\ ^*bf
0\";'''$";nested={attrset={has={a={integer={value=100;};};};};};simple_top_level_attr="1.0";some_floaty=29.95;weirderTypes={
-@include "/nix/store/jcp2wambykcd4nrhd720zg5j1aygn6jh-libconfig-test-include"
+@include "/nix/store/dz01aq1prsxdh0sxj801q2jk97rng2zm-libconfig-test-include"
array_of_ints=[0732, 0xa3, 1234];bigint=9223372036854775807;float=0.0012;hex=0x1fc3;list_of_weird_types=(3.141592654, 9223372036854775807, 0x1fc3, 027, 1.2e-32, 1.0);octal=027;pi=3.141592654;};
[…]
error: building '/nix/store/c9rhjk5vgwa0326hl9hv0p3c3a7cz86d-pkgs.formats.libconfig-test-comprehensive.drv' failedAlso, looking at how quickly #244835 got merged is dismaying when compared to this one, just one review with no nits and it didn't even pass through staging for any stray NixOS tests as far as I can see. EDIT: I don't think #244835 is actually related anymore, it didn't replace trivial-builders. Oops. |
5c5e2f3 to
92f8d52
Compare
|
@roberth I think I'll pass over the prettification for now, if you don't mind. Is there anything more needed for merge? |
92f8d52 to
aa44c5e
Compare
|
@roberth friendly ping |
Co-authored-by: ckie <25263210+ckiee@users.noreply.github.com> Signed-off-by: h7x4 <h7x4@nani.wtf>
Co-authored-by: ckie <25263210+ckiee@users.noreply.github.com> Signed-off-by: h7x4 <h7x4@nani.wtf>
aa44c5e to
5707d01
Compare
|
Thank you @h7x4! |
|
Thank you! |
|
EDIT: this test-set blocks |
|
Anyway, fixed in #264581. |
Description of changes
This is a new attempt at #208747, with the wider goal of solving the obstruction of #167388.
I've addressed some of the comments in the earlier PR, specifically the ones about not generating it in nix, and about the number formats. This implementation exports the attrset as JSON, and converts it using a relatively small rust program. It supports include directives, and special types like hex, octal and scientific notation floats. I've (ab)used the fact that libconfig doesn't allow their setting names to start with
_(see<name>), to ensure the expressiveness stays the same.One point that might be a bit iffy, is the way the generator automatically determines whether something should be a list or an array. It reduces the expressive power a little bit, as it's now impossible to create lists of equal-typed values. I do not think it should be a problem, but it should probably be discussed.There exists a test here:
nix-build -A pkgs.tests.pkgs-lib.libconfig.comprehensiveThings done
sandbox = trueset innix.conf? (See Nix manual)nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)