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

pkgs.formats: Add HOCON format generator #280322

Merged
merged 6 commits into from
Feb 10, 2024

Conversation

h7x4
Copy link
Member

@h7x4 h7x4 commented Jan 11, 2024

Description of changes

Taking some of the lessons learnt from #246115, I've made a generator for the HOCON format. This generator aims to support (almost) all the features of the HOCON format, including things like substitutions from envvars, different kinds of include statements both for declarative and mutual includes, append statements, etc.

HOCON does not seem to have a definitive governing body, nor a complete formal specification, so I've chosen to base my implementation upon the best I could find: https://github.com/lightbend/config/blob/main/HOCON.md. This means that there's bound to be some compatibility issues with certain implementations (in fact, I've already stumbled upon one with the include types when I tried writing the validator using the most popular go implementation), but I think this is unavoidable to some degree.

Up until now, I've only found these three that are using the program, as well as a pending package. Most of them seem to have copied each others implementation, so backwards compatability can probably be combined:

  • nixos/modules/services/networking/jitsi-videobridge.nix
  • nixos/modules/services/networking/jicofo.nix
  • nixos/modules/services/networking/jibri/default.nix
  • nixos/modules/services/web-apps/tachidesk-server.nix (nixos/tachidesk-server: init at 0.7.0 #257733)

TODO:

  • write generator
  • write tests
  • replace existing implementations treewide
  • provide some sort of backwards compatibility or warning to smooth migration from the existing implementations
    (I think the only thing we'd need to handle would be __hocon_envvar and __hocon_unquoted_string)

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.05 Release Notes (or backporting 23.05 and 23.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Some tests that now use this generator either directly or indirectly:

nix build ".#pkgs.tests.pkgs-lib.hocon.comprehensive"
nix build ".#pkgs.tests.pkgs-lib.hocon.backwards-compatibility"
nix build ".#nixosTests.jibri" # This one is failing for seemingly unrelated reasons
nix build ".#nixosTests.jitsi-meet"
nix build ".#nixosTests.prometheus-exporters.jitsi"
nix build ".#nixosTests.suwayomi-server.with-auth"
nix build ".#nixosTests.suwayomi-server.without-auth"

Add a 👍 reaction to pull requests you find important.

@ckiee
Copy link
Member

ckiee commented Jan 12, 2024

hi, I have to focus my energy on other things so I won't be participating in this PR. best of luck!

@ckiee ckiee removed their request for review January 12, 2024 13:16
Copy link
Member

@roberth roberth left a comment

Choose a reason for hiding this comment

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

This looks well thought out, follows an established implementation strategy (see libconfig), has tests, and the Nix code looks good to me.
Although I don't have the opportunity to dive into the intricacies of HOCON, to "definitively" claim that all the details are good, I am confident that this code is maintainable.

@h7x4 h7x4 force-pushed the add-hocon-format-generator branch 3 times, most recently from 60db95e to badf956 Compare January 25, 2024 06:26
@h7x4 h7x4 marked this pull request as ready for review January 25, 2024 06:31
@h7x4 h7x4 requested a review from infinisil as a code owner January 25, 2024 06:31
@h7x4
Copy link
Member Author

h7x4 commented Jan 25, 2024

Okay, this should be completely ready now.

pings for maintainers of affected modules:

@h7x4
Copy link
Member Author

h7x4 commented Jan 25, 2024

@ofborg test jitsi-meet prometheus-exporters.jitsi suwayomi-server.with-auth suwayomi-server.without-auth

Copy link
Member

@infinisil infinisil left a comment

Choose a reason for hiding this comment

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

Overall this is looking great, I'm impressed by the code quality! Just got some minor suggestions, but I'm happy to merge

pkgs/pkgs-lib/formats/hocon/src/src/main.rs Outdated Show resolved Hide resolved
pkgs/pkgs-lib/formats/hocon/src/src/main.rs Outdated Show resolved Hide resolved
pkgs/pkgs-lib/formats/hocon/test/default.nix Outdated Show resolved Hide resolved
@h7x4 h7x4 force-pushed the add-hocon-format-generator branch from 3735717 to 421429f Compare February 9, 2024 17:17
@infinisil
Copy link
Member

Found the problem, pushed the fix :D

This is useful because the tests in `pkgs-lib` can mock out certain
`lib` functions like this using a `lib` overlay.
@h7x4 h7x4 force-pushed the add-hocon-format-generator branch from 04df819 to 7065951 Compare February 9, 2024 17:50
@h7x4
Copy link
Member Author

h7x4 commented Feb 9, 2024

Found the problem, pushed the fix :D

Cool! I've reordered and rebased the fixup to make more sense.

@infinisil
Copy link
Member

Looking good, let's merge!

@infinisil infinisil merged commit 11cd405 into NixOS:master Feb 10, 2024
20 checks passed
@infinisil
Copy link
Member

Forgot to mention that this should be documented! See #288163

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

Successfully merging this pull request may close these issues.

4 participants