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

YAML for config #343

Open
Tracked by #204
srid opened this issue Nov 12, 2024 · 8 comments · May be fixed by #352
Open
Tracked by #204

YAML for config #343

srid opened this issue Nov 12, 2024 · 8 comments · May be fixed by #352
Assignees

Comments

@srid
Copy link
Member

srid commented Nov 12, 2024

This would mitigate #339

Our entire omnix configuration (om.nix),
https://github.com/juspay/omnix/blob/main/nix/modules/flake-parts/om.nix

can be written as YAML as well (see below). This has the benefit of not needing to evaluate the whole flake just to get the omnix config. This would then make om develop faster.

We may still have to support Nix-based config (but that piggyback on YAML using builtins.fromYAML) for cases where we evaluate a non-local flake (even then we can catch the flake in local nix store before looking up YAML, falling back to Nix eval).

Blocked on #341 (after which doing this should be straightforward).

ci:
  default:
    omnix:
      dir: "."
      steps:
        # build:
        #   enable: false
        flake-check:
          enable: false  # Not necessary
        custom:
          om-show:
            type: "app"
            # name: "default"
            args:
              - "show"
              - "."
          binary-size-is-small:
            type: "app"
            name: "check-closure-size"
            systems:
              - "x86_64-linux"  # We have static binary for Linux only.
          omnix-source-is-buildable:
            type: "app"
            name: "omnix-source-is-buildable"
          cargo-tests:
            type: "devshell"
            command:
              - "just"
              - "cargo-test"
            systems:
              - "x86_64-linux"
              - "aarch64-darwin"  # Avoid emulated systems
          cargo-clippy:
            type: "devshell"
            command:
              - "just"
              - "clippy"
            systems:
              - "x86_64-linux"
              - "aarch64-darwin"  # Avoid emulated systems
          cargo-doc:
            type: "devshell"
            command:
              - "just"
              - "cargo-doc"
            systems:
              - "x86_64-linux"
              - "aarch64-darwin"  # Avoid emulated systems
    doc:
      dir: "doc"
    registry:
      dir: "crates/omnix-init/registry"
      steps:
        build:
          enable: false
        custom: {}
    cli-test-dep-cache:
      dir: "crates/omnix-cli/tests"
      steps:
        lockfile:
          enable: false
        flake_check:
          enable: false
        custom: {}
health:
  default:
    nix-version:
      min-required: "2.16.0"
    caches:
      required:
        - "https://om.cachix.org"
    direnv:
      required: true
develop:
  default:
    readme: |
      🍾 Welcome to the **omnix** project

      To run omnix,

      ```sh-session
      just watch <args>
      ```

      (Now, as you edit the Rust sources, the above will reload!)

      🍎🍎 Run 'just' to see more commands. See <https://nixos.asia/en/vscode> for IDE setup.
@srid srid changed the title YAML for config TOML for config Nov 12, 2024
@srid srid changed the title TOML for config YAML for config Nov 12, 2024
@srid
Copy link
Member Author

srid commented Nov 12, 2024

Oh well NixOS/nix#7340

@srid
Copy link
Member Author

srid commented Nov 12, 2024

even then we can catch the flake in local nix store before looking up YAML, falling back to Nix eval

So this is what we should do, nevermind fromYAML

image

@srid srid mentioned this issue Nov 13, 2024
15 tasks
@shivaraj-bh
Copy link
Member

What do we use to parse YAML in rust? I see three options:

@srid
Copy link
Member Author

srid commented Nov 15, 2024

I'd use serde-yaml for now.

A maintained fork may popup later (there's one already but not ready yet: cafkafk/serde-norway#17). There's also https://github.com/saphyr-rs/saphyr but saphyr-serde is not available yet. So in future we can switch to one of these that the community adopts.

@shivaraj-bh
Copy link
Member

Also, what's the problem with using json for configs instead?

@srid
Copy link
Member Author

srid commented Nov 15, 2024

Not a problem per se, but the UX of editing the config can suck a bit.

https://x.com/i/grok/share/eulnfhBsYG4zrqRX3U96yU9Q6

For eg., multiline-strings will look like this in JSON (forced to be single line, peppered with \n's):

image

TOML is another option, but nested attrs will look pretty verbose:

image


Do you think we should simply go with JSON instead?

@jayvdb
Copy link

jayvdb commented Nov 16, 2024

re YAML, more options are listed at rustsec/advisory-db#2132 . https://crates.io/crates/serde_yml doesnt use unsafe-libyaml - instead it uses a fork https://github.com/sebastienrousseau/libyml which is actively maintained. I had a problem using it to parse OpenAPI schemas , but it is probably fine with simpler schemas.

@shivaraj-bh
Copy link
Member

shivaraj-bh commented Nov 21, 2024

Basic benchmark of fetching OmConfig from om.nix vs its json equivalent. The benchmark was done using divan on an MBP M1 pro:

Timer precision: 41 ns
config             fastest       │ slowest       │ median        │ mean          │ samples │ iters
├─ from_flake_url  156.5 ms      │ 346.5 ms      │ 173.9 ms      │ 176.2 ms      │ 100     │ 100
├─ from_json       26.16 µs      │ 146.4 µs      │ 29.54 µs      │ 31.53 µs      │ 100     │ 100
╰─ from_yaml       110.4 µs      │ 478.3 µs      │ 115.3 µs      │ 125.5 µs      │ 100     │ 100

Edit: I’m not sure if we’d want to include these benchmarks in juspay/omnix, so they’re in my fork for now. See the implementation.

Edit: I’ll also be adding YAML to this soon.

Edit: The benchmark now includes YAML too

@shivaraj-bh shivaraj-bh linked a pull request Nov 25, 2024 that will close this issue
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 a pull request may close this issue.

3 participants