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

nix_rs: Inherit stderr while running nix as a child process #222

Closed
wants to merge 25 commits into from

Conversation

shivaraj-bh
Copy link
Member

@shivaraj-bh shivaraj-bh commented Aug 16, 2024

resolves #209

We also use flake-schemas in this PR to fetch the om config from the flake output instead of using nix eval (since inheriting stderr means it's no longer captured for om to process). An additional, unintended benefit of using flake-schemas is caching the output of nix flake show.

Changes in nix CLI

shivaraj-bh/nix@1d23c1e

Demo

❯ om show github:nammayatri/nammayatri
🐚 nix --extra-experimental-features 'nix-command flakes' show-config --json
warning: unknown setting 'upgrade-nix-store-path-url'
🐚 /nix/store/n02w2ybg9fc78grzz9i2aj49q3rysp7m-nix-2.24.0pre20240801_af10904/bin/nix flake show --legacy --allow-import-from-derivation --json --default-flake-schemas /nix/store/xzalq6mcw0ahyaccab6k98dbx3ll53y6-source github:nammayatri/nammayatri
warning: unknown experimental feature 'repl-flake'
[0.6 MiB DL] downloading 'https://github.com/nammayatri/nammayatri/archive/a3e472efe24e731268d20a31f75a1519ded50220.tar.gz'^C
[0.6 MiB DL]

TODO

  • Update nix_rs changelog
  • Update history.md
  • flake-schemas as local flake (for omSchema) while importing majority of schemas from DetSys/flake-schemas
  • move find_qualified_attr_in_flake_output to FlakeOutputs
  • simplify FlakeOutputs serialization… use untagged for flattening
  • refactor(DRY) in nix/flake-schemas/flake.nix

crates/nix_rs/src/command.rs Outdated Show resolved Hide resolved
@shivaraj-bh shivaraj-bh marked this pull request as draft August 16, 2024 19:40
@shivaraj-bh shivaraj-bh changed the title nix_rs: Stream stdout of nix to om nix_rs: Inherit stderr while running nix as a child process Aug 21, 2024
@shivaraj-bh
Copy link
Member Author

Upon inheriting stderr of child process (to display progress of underlying nix command), we can no longer capture it, breaking nix_eval_attr.

Do we need nix_eval_attr to rely on stderr to conclude that an attribute is missing? Could we use flake-schemas here?

Making flake-schemas recognise om flake output isn’t that hard: shivaraj-bh/flake-schemas@037434d

@shivaraj-bh
Copy link
Member Author

Oh no, It doesn’t:

"om": {
    "doc": "Configuration for `omnix` CLI.\n",
    "output": {
      "children": {
        "ci": {
          "leaf": true
        },
        "health": {
          "leaf": true
        }
      }
    }
  },

I will have to recursively mkChildren to include the entire configuration in the schema.

@shivaraj-bh
Copy link
Member Author

Upon recursively creating these children, it seems to sorta work:

"om": {
    "doc": "Configuration for `omnix` CLI.\n",
    "output": {
      "children": {
        "ci": {
          "children": {
            "default": {
              "children": {
                "cli-test-dep-cache": {
                  "children": {
                    "dir": {
                      "leaf": true
                    }
                  }
                },
                "doc": {
                  "children": {
                    "dir": {
                      "leaf": true
                    }
                  }
                },
                "flakreate-registry": {
                  "children": {
                    "dir": {
                      "leaf": true
                    }
                  }
                },
                "omnix": {
                  "children": {
                    "dir": {
                      "leaf": true
                    }
                  }
                }
              }
            }
          }
        },

But if you notice, along with “leaf” we don’t have the attribute mentioning the “value” of the leaf node, even though it is added in the schema:

{
  let
    omSchema = {
      version = 1;
      doc = ''
        Configuration for `omnix` CLI.
      '';
      inventory = output:
      let
        recurse = prefix: attrs: self.lib.mkChildren (builtins.mapAttrs
          (attrName: attrs:
            if (builtins.typeOf attrs) != "set" then
              {
                value = attrs;
                what = "omnix config";
              }
            else
              recurse (prefix + attrName + ".") attrs
          )
          attrs);
      in
        recurse "" output;
    };
  in
}

@shivaraj-bh
Copy link
Member Author

That is an easy fix in nix, see: shivaraj-bh/nix@1d23c1e

Note to self: Mention this use case in flake-schemas PR along with examples.

@shivaraj-bh
Copy link
Member Author

voila

"om": {
    "doc": "Configuration for `omnix` CLI.\n",
    "output": {
      "children": {
        "ci": {
          "children": {
            "default": {
              "children": {
                "cli-test-dep-cache": {
                  "children": {
                    "dir": {
                      "leaf": true,
                      "value": "crates/omnix-cli/tests",
                      "what": "omnix config"
                    }
                  }
                },
                "doc": {
                  "children": {
                    "dir": {
                      "leaf": true,
                      "value": "doc",
                      "what": "omnix config"
                    }
                  }
                },
                "flakreate-registry": {
                  "children": {
                    "dir": {
                      "leaf": true,
                      "value": "crates/flakreate/registry",
                      "what": "omnix config"
                    }
                  }
                },
                "omnix": {
                  "children": {
                    "dir": {
                      "leaf": true,
                      "value": ".",
                      "what": "omnix config"
                    }
                  }
                }
              }
            }
          }
        },

@srid
Copy link
Member

srid commented Aug 21, 2024

See also #211 (omnix flake-module)

We also want to support the scenario of there not being any om configuration in the flake (so as to default back to the Default::default value).

@srid
Copy link
Member

srid commented Aug 21, 2024

Is flake-schemas really relevant here? Consider the maintenance burden we would be taking on. Keeping all 3 places -- Rust types, flake-parts module, and flake-schema -- in sync.

@shivaraj-bh
Copy link
Member Author

shivaraj-bh commented Aug 21, 2024

Is flake-schemas really relevant here? Consider the maintenance burden we would be taking on. Keeping all 3 places -- Rust types, flake-parts module, and flake-schema -- in sync.

Rust types

#214 should solve the burden of maintaining Rust types?

flake-parts module

Aren’t we maintaining the flake-parts module in omnix itself?

flake-schema

That would require maintaining a fork until the upstream merge happens, I can keep this up-to-date, now that I am slowly getting familiarised with the nix codebase.

@shivaraj-bh
Copy link
Member Author

I did try to find different ways to not rely on the stderr output, but unless there is a way to print flake output as json, I don’t see anything better than using flake-schemas.

@srid
Copy link
Member

srid commented Aug 21, 2024

Feel free to explore the flake-schemas route. I'd be curious to see how it looks, in the end. Right now, I don't have a clear of idea of what is entailed.

That would require maintaining a fork until the upstream merge happens, I can keep this up-to-date, now that I am slowly getting familiarised with the nix codebase.

Can't the flake-schema live in this repo itself? I thought flake-schemas are meant to be decentralized, anyway (i.e., downstream flakes can specify their own schemas).

@srid
Copy link
Member

srid commented Aug 21, 2024

Rust types

#214 should solve the burden of maintaining Rust types?

For om show, yes.

Not necessarily for stuff like #216


Here's an idea: keep Rust types as canonical source of truth, and then generate the Nix expression necessary for creating flake-parts module and flake-schema from it ... in the same vein as https://hackage.haskell.org/package/purescript-bridge

@shivaraj-bh
Copy link
Member Author

This PR changes a bunch of things, to make the review simpler here are the primary changes:

  • flake configs are no longer fetched using nix eval, making nix_eval_attr, nix_eval_qualified_attr and omnix-common::OmConfig::from_flake_url redundant, should we still support them along with supporting missing_attribute check via nix eval?
  • Leaf enum has been removed for simplicity while defining custom serializer for FlakeOutputs

@shivaraj-bh shivaraj-bh marked this pull request as ready for review September 2, 2024 09:10
@srid
Copy link
Member

srid commented Sep 2, 2024

(Had to merge from main because I just wanted to try running it on some flakes..)

@srid
Copy link
Member

srid commented Sep 2, 2024

Functionally, it works quite well:

image

@shivaraj-bh
Copy link
Member Author

@srid added a couple of TODO’s to the description, feel free to add more if I missed any

@shivaraj-bh
Copy link
Member Author

Update the usage example in show.md

@srid do you mean to update the example itself? i.e to show something with less of “N/A” or to show the progress of “nix flake show” due to inherited stderr?

@srid
Copy link
Member

srid commented Sep 3, 2024

Former. Less of N/A.

@srid
Copy link
Member

srid commented Sep 3, 2024

Update the usage example in show.md

Nevermind; I thought we are doing om show on this repo itself, but this is happening on nixos-config.

@srid

This comment was marked as outdated.

@shivaraj-bh

This comment was marked as outdated.

@srid

This comment was marked as outdated.

@shivaraj-bh

This comment was marked as outdated.

@srid
Copy link
Member

srid commented Sep 3, 2024

Huh, ignore my last screenshot. I think I ran it using om from my nixos-config. Here's the latest:

image

(This was run from the omnix-init branch, but that one is up to date with main).

@shivaraj-bh
Copy link
Member Author

Apart from Refactoring flake-schemas/flake.nix, this PR is almost ready. @srid should we bother keeping eval.rs, now that it isn’t being used anywhere?

@srid
Copy link
Member

srid commented Sep 3, 2024

We can get rid of eval.rs. By the way, I don't understand the Filtered output types much. But I'll take a look a bit later.

(You can also drop by huddle to pair on it)

@shivaraj-bh shivaraj-bh marked this pull request as draft September 3, 2024 23:27
@srid srid closed this Oct 7, 2024
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 this pull request may close these issues.

om show: Display progress
2 participants