vivid: add module

[arunoruto]

Description

Add module for https://github.com/sharkdp/vivid.
Previous PRs #2194 and #3705 addressed this, but never merged.

The themes and filetypes section were taken from #2194.
The shell integrations are used since the variable name LS_COLORS needs to be set to the output of the command vivid generate <theme>. One could utilize what #2194 used:

home.sessionVariables = mkIf (cfg.theme != null) {
    LS_COLORS = "$(${cfg.package}/bin/vivid generate ${cfg.theme})";
};

but this wouldn't work for fish and nushell.

Checklist

• Change is backwards compatible.

• Code formatted with ./format.

• Code tested through nix-shell --pure tests -A run.all or nix develop --ignore-environment .#all using Flakes.

• Test cases updated/added. See example.

• Commit messages are formatted like

  {component}: {description}
  
  {long description}
  

  See CONTRIBUTING for more information and recent commit messages for examples.

• If this PR adds a new module

  • Added myself as module maintainer. See example.

Maintainer CC

[arunoruto]

@folliehiyuki Hej, I just wanted to check if the PR is alright. It is my first in the hm repo, so feedback is always appreciated!

[folliehiyuki]

You can put a with lib; declaration here, so usages of lib afterward can be shorten.

[folliehiyuki]

Suggested change

	type = lib.types.nullOr lib.types.str;
	type = with types; nullOr str;

assuming that with lib; was declared above.

[folliehiyuki]

The same goes for the other options as well.

[arunoruto]

I have rewritten the module with with lib;.
I checked it multiple times, so there shouldn't be anything missing.

[folliehiyuki]

Can you also write a couple of assert tests for filetypes.yml and a sample theme file?

[arunoruto]

Can you also write a couple of assert tests for filetypes.yml

Do you have any examples on how to write those tests? I am not that experienced.

and a sample theme file?

In Nix and put it in the literalExample field?

[folliehiyuki]

You can look into https://github.com/nix-community/home-manager/tree/master/tests/modules/programs for referenced assert test cases and how to write them.

and a sample theme file?

In Nix and put it in the literalExample field?

I mean defining them in an assert test. You write the sample theme inside Nix and assert the generated theme file with expected value.

[arunoruto]

You can look into https://github.com/nix-community/home-manager/tree/master/tests/modules/programs for referenced assert test cases and how to write them.

and a sample theme file?

In Nix and put it in the literalExample field?

I mean defining them in an assert test. You write the sample theme inside Nix and assert the generated theme file with expected value.

I looked into the new ghostty PR and took that init as a base to design the tests (plus some other modules as you suggested!). All of the tests pass on my device, except the enable-shells.nix one. I guess it can't replace @vivid@ with the correct path? Or am I doing something wrong here... I was using @vivid@/bin/vivid instead of @vivid@/bin/dummy. Now all of my tests pass!

I also went a bit overboard with the config, but I wanted to see if it works correctly! Hit me up if I need to reduce it :)

I also fixed a small type in generating the LS_COLORS variable, should be more consistent now!

[arunoruto]

@folliehiyuki sorry for the tag, but I wanted finish this PR so it can be integrated until the 25.05 release.

[arunoruto]

@rycee Hi, sorry for the ping, but I wanted to ask if someone could review the PR in march/april :) maybe you know someone, who has time on their hands, since Hoang seems to be busy.

[sedlund]

im not a reviewer, but my thoughts:

this is a very simple program that generates static output based on the given theme name. and there is just a lot here to go over and maybe why there is a problem in getting a reviewer.

the testing seems overkill. the test should be specific to the implementation of the nixos module working, not every aspect of the program itself -- that should be in upstream.

I implement it like this:

sessionVariables =
  let
    lsColors = builtins.readFile (
      pkgs.runCommand "vivid-ls-colors" { } ''
        ${lib.getExe pkgs.vivid} generate dracula > $out
      ''
    );
  in
  {
    LS_COLORS = "${lsColors}";
  }

this allows nix build to run the vivid command to output LS_COLORS and stores it. it runs once on the build machine, not requiring the vivid binary to be copied to each host and to be run each time a login shell is executed.

also this uses sessionVariables so it is shell agnostic. you could do the same and get rid of the extra shell specific options.

hope this helps

[arunoruto]

@sedlund Thanks for the feedback!

What would you recommend to focus the testing on? I am still new in that regard to nixos and home-manager.

I just implemented the recommended changes using pkgs.runCommand and it worked, albeit needing a restart of the current user session. I implement the changes upstream when I am at home.

Just tried to do it at work and nuked my branch with a reset... But I should still have the files on my laptop at home! Afterwards I will reopen the PR or create a new one if I can't reopen this one!

[arunoruto]

Reopening it

[arunoruto]

Sorry for the spam!
I just recovered my branch and pushed the suggested changes :)

As for the tests, I would refactor them if you have some suggestions or even an example. Should I just make the files smaller, like a minimal working example, and use that as a test?

[sedlund]

Should I just make the files smaller, like a minimal working example, and use that as a test?

My thought is that the current test is only testing if lib.yaml.generate generates yaml correctly - which should be tested upstream.

I tested creating a small filetypes.yml and a theme that references a filetype that does not exist in the filetypes.yml and running:

vivid generate mytheme

this tests and verifies the filetypes.yml file and theme yml as a working set together and correctly tells me:

Error: Could not find style for category 'programming.source.cxx'

and exits 1

This would be output now during build with your recent changes - and I think the output would be more useful.

Nothing stands out to me what more could be tested.

[Anomalocaridid]

I implement it like this:

sessionVariables =
  let
    lsColors = builtins.readFile (
      pkgs.runCommand "vivid-ls-colors" { } ''
        ${lib.getExe pkgs.vivid} generate dracula > $out
      ''
    );
  in
  {
    LS_COLORS = "${lsColors}";
  }

this allows nix build to run the vivid command to output LS_COLORS and stores it. it runs once on the build machine, not requiring the vivid binary to be copied to each host and to be run each time a login shell is executed.

also this uses sessionVariables so it is shell agnostic. you could do the same and get rid of the extra shell specific options.

hope this helps

Not a reviewer either, but wouldn't this cause unnecessary import-from-derivation (IFD)? This would slow down evaluation and make the module unusable for people that have allow-import-from-derivation set to false.

[sedlund]

Interesting, was not aware. Although this happens currently here in a few spots already

It could be stored as an argument and executed everytime a build is ran if it is of concern. It doesn't seem like this would be in critical code path though 🤷‍♀️

[arunoruto]

I guess this means I will revert it back to the shell options?
This is usually how most shell programs import, like direnv with its init function. And vivid is implemented in rust, to the bottleneck shouldn't be there, I hope.
Either way, maybe the upstream project could implement a caching function, where the output could be stored in ~/.cache and loaded from there, if a theme exists.

I will also make a change to the testing suite, as proposed by @sedlund, triggering errors with faulty configs, instead of generating yaml configs.

[sedlund]

I really don't think IFD is an issue here at all. It is good to be aware of when doing it for large evals (package builds, etc).

But in this context it only blocks eval when the vivid derivation or the config file changes. The eval takes 3ms on my machine. The time it would take to distribute the binary to various machines would take far longer. So doing this is still preferred computationally and network wise.

I'd be happy to hear why this would not be true.

[sedlund]

deleted that suggestion, it was not correct

[stale]

Thank you for your contribution! I marked this pull request as stale due to inactivity. Please read the relevant sections below before commenting.

If you are the original author of the PR

• GitHub sometimes doesn't notify people who commented / reviewed a PR previously when you (force) push commits. If you have addressed the reviews you can officially ask for a review from those who commented to you or anyone else.
• If it is unfinished but you plan to finish it, please mark it as a draft.
• If you don't expect to work on it any time soon, please consider closing it with a short comment encouraging someone else to pick up your work.
• To get things rolling again, rebase the PR against the target branch and address valid comments.

If you are not the original author of the PR

• If you want to pick up the work on this PR, please create a new PR and indicate that it supercedes and closes this PR.

[tuxiqae]

Vivid support was added here #7772
Do we still need this PR open?

[arunoruto]

Vivid support was added here #7772 Do we still need this PR open?

Thanks for the update! I will close this PR now :)
P.S. maybe look at what the comments about testing in this PR, since it was the main reason it got delayed so far...

[tuxiqae]

FYI I took no part in #7772, I just noticed that it was merged and decided to notify you :)