emacs-twist / twist.nix

Build an entire Emacs configuration as a Nix package in a pure, reproducible way
GNU General Public License v3.0
78 stars 7 forks source link

Parse with file name rather than file contents #60

Closed LuigiPiucco closed 1 year ago

LuigiPiucco commented 1 year ago

I modified this repo for my own use-case, but it's a very small change and I think it's a generally useful one. Specifically, I'm trying to wrap DOOM Emacs in Nix, by replacing straight.el. DOOM configs have 3 files, and each has its meaning, it's not just package definitions, and package related pieces may come in a few different forms (builtin DOOM modules vs. user packages).

With the version prior, only the contents of each file in initFiles was passed to initParser, which made per-file behavior much harder to implement. By reading inside the parser with the name as argument, this becomes easier.

I also added a direnv shell with rnix-lsp, you can safely cherry-pick that one out if it isn't desired.

I thought this could help others, but if you think it won't, no worries, I'l try and maintain my changes separately.

Testing

I couldn't quite figure out how this is tested, what I ended up doing was building the test folder's flake.nix and running the generated emacs environment. That worked well, but it might not be sufficient to guarantee accuracy.

akirak commented 1 year ago

Thank you for sending this PR. At first sight, the PR looked fine, but I have reverted that. I'm sorry for that.

Let me think more about this. I have understood your problem, but I don't like the idea of making parseUsePackages and parseSetup have an extra step of reading a file. It is less modular.

How about passing initReader as an option instead of initParser? That is, it would be better if the user could build his/her own reader by composing builtins.readFile and one of the parsers that take string as an argument. This way, initFiles actually doesn't have to be a list of files, so it will be more flexible.

Regarding the addition of shell.nix, I am not sure if I want to do that. I want to keep the workflow consistent across multiple projects as much as possible, so I want to try it out for some time before adopting it into the most crucial (?) project for me. I will consider using rnix-lsp. Thanks for the suggestion.

I tested this project by building examples with the input overridden (as below), but it takes more time. I add discovered corner cases to init.el in this repository, so it should work for most packages. The test suite needs improvement.

nix flake lock --update-input twist --override-input twist github:LuigiPiucco/twist.nix
LuigiPiucco commented 1 year ago

Thank you for sending this PR. At first sight, the PR looked fine, but I have reverted that. I'm sorry for that.

No need to be sorry, discussing things is key to open-source, in my opinion.

How about passing initReader as an option instead of initParser? That is, it would be better if the user could build his/her own reader by composing builtins.readFile and one of the parsers that take string as an argument. This way, initFiles actually doesn't have to be a list of files, so it will be more flexible.

I think it would work as well. Just to check if I understood correctly, is it something like this?

# In "pkgs/emacs/default.nix"
{
  # ...
  initParser ? lib.parseUsePackages {},
  initReader ? file: initParser (builtins.readFile file),
  # ...
}:
# ...
      (map (file: initReader file))
# Default parsers remain as they were.

If so, I can make such a change, I just need to know if you prefer that I add a new commit or rebase and force-push to my fork.

Regarding the addition of shell.nix, I am not sure if I want to do that. I want to keep the workflow consistent across multiple projects as much as possible, so I want to try it out for some time before adopting it into the most crucial (?) project for me. I will consider using rnix-lsp. Thanks for the suggestion.

This was more for my own use, you can cherry-pick that one out (or, if you prefer, I can rebase without it). The case for having it defined per project is reproducibility: Anyone with direnv can have the proper environment, without having to install to the system globally each time.

akirak commented 1 year ago

is it something like this?

Yes, I was thinking of a similar one.

If so, I can make such a change, I just need to know if you prefer that I add a new commit or rebase and force-push to my fork.

Thanks. I don't mind either way, but force-push would be better.

Anyone with direnv can have the proper environment, without having to install to the system globally each time.

Yes, I understand the point and use direnv. I use flake.nix to manage environments for my personal projects, but it adds extra inputs to flake.lock, which is not good for libraries. I have also recently discovered devenv, so I haven't decided which one to use.

LuigiPiucco commented 1 year ago

Yes, I understand the point and use direnv. I use flake.nix to manage environments for my personal projects, but it adds extra inputs to flake.lock, which is not good for libraries. I have also recently discovered devenv, so I haven't decided which one to use.

About this, the way I did it does not add an input to the flake, because it's in a separate shell.nix rather than the devShell output of flake.nix. I did use builtins.getFlake as a shortcut, but since direnv's use nix uses nix-shell, not nix shell, it allows impurities and does not lock the input. Currently, the flake ref is just "nixpkgs", which means we get whatever the user has in their system. Upon further thought, though, "github:NixOS/nixpkgs" could also be used, since it doesn't lock as well. However I'm not sure if it rebuilds at every commit to nixpkgs. I will inspect that later.

Overall, I think what I'm going to do is:

  1. Rebase my changes to use initReader;
  2. Cherry-pick the direnv commit into a separate branch;
  3. Further analyze the issue separately, considering devenv as well;
  4. Make another PR/issue with the results.
LuigiPiucco commented 1 year ago

I was thinking of adding Doom to examples. Hope to see your Nix configuration soon! It will make my project useful to others.

Sure! I am going to share it, and having an example would definitely be good. I just don't know how soon that'll be; we're at the end of semester period here, which is always very time-consuming. Once I get it working and minimally useful, I'll open-source it.

akirak commented 1 year ago

I just don't know how soon that'll be; we're at the end of semester period here, which is always very time-consuming.

I don't mind that. I need to prepare user documentation for twist before it gets more public attention, but for some reasons I cannot work on it right now.

Thank you for your contribution!