{nixos,home,darwin}: add edit subcommand

[midischwarz12]

Sanity Checking

• I have updated the changelog as per my changes
• I have tested, and self-reviewed my code
• Style and consistency
  • I ran nix fmt to format my Nix code
  • I ran cargo fmt to format my Rust code
  • I have added appropriate docunentation to new code
  • My changes are consistent with the rest of the codebase
• Correctness
  • I ran cargo clippy and fixed any new linter warnings.
• If new changes are particularly complex:
  • My code includes comments in particularly complex areas to explain the
    logic
  • I have documented the motive for those changes in the PR body or commit
    description.
• Tested on platform(s)
  • x86_64-linux
  • aarch64-linux
  • x86_64-darwin
  • aarch64-darwin

---

Add a 👍 reaction to pull requests you find important.

[NotAShelf]

Should support non-flakes as well. When an installable is passed or if --file is used we'd like to edit it directly.

Also I think we should bail instead of trying nano when $EDITOR is unset. If user doesn't have EDITOR set, then it's unlikely that they have nano installed imo.

[NotAShelf]

Also we don't use conventional commits, please consider rewording to match the title.

[midischwarz12]

Should support non-flakes as well. When an installable is passed or if --file is used we'd like to edit it directly.

I intentionally left out support for a --file flag because at that point, it brings no advantage to just using their desired editor directly since you're already supplying the path.

Although, I can see the usefulness of wanting to automatically direct the editor to a designated flake location or non flake location such as /etc/nixos.

[NotAShelf]

@midischwarz12 I've moved this to the 4.2.0 milestone. Once again there is no rush, but if you could address my comments as discussed on Matrix I'd like to merge this before Monday and start thinking about 4.2.0

[midischwarz12]

I'll probably have time to get this out tomorrow as well. If not, it'll be done on Thursday for sure.

[midischwarz12]

@NotAShelf Feel free to take a look. Sorry for the long wait.

[NotAShelf]

Okay, this PR is less close to a merge than I had hoped. While I hate to provide negative reviews, there is no way I can merge this as is. Unfortunately I must leave my desktop shortly, but here is a quick list of thoughts:

1. The duplication of edit logic is unnecessary. They are so similar, there could be a generic helper that consolidates all three platform implementations and avoids the absurd +319 LoC addition. You can consider putting this in util, or in commands (not commands.rs; something like commands::edit)
2. I hope you do realise there is little to no value in editing a flake from the store directory. Not only is -o not supported for nix flake prefetch, but fetching it to thee Nix store and trying to edit it there (or the symlink) defeats the entire purpose of an edit subcommand. You can't edit the store, silly. Even if -o was supported you would be trying to edit a symlink... As I've shared with you befeore, the goal here was to edit the given flake directory (or a configuration.nix file), not a store copy of it. This should have been as easy as picking the correct variable (or its nix2 equivalent) from NH_FLAKE, NH_OS_FLAKE etc. and opening the directory with $EDITOR. Conveniently nix appears to have a nix edit command (for whatever reason) which you can use to get rid of some of th code. I also expect correct handling for when nano fails (which is likely). Also a warning when $EDITOR fails.
3. The blocking command implmentation and especially nix flake prefetch is an incredibly slow operation. Did you ever test this PR?

Since the goal of the edit subcommand is to provide backwards compatibility with nixos-rebuild edit, I would encourage you to review nixos-rebuild-ng and see how they handle the edit subcommand. Do note, however, that this PR needs a serious makeover for it to be mergable.

[midischwarz12]

Unfortunate, but respectfully, I think you're a bit confused for part of this.

1. We previously talked about delaying generic platform logic to a future PR. I kind of agreed since there's a crap ton of other stuff between all three platforms that needs to be made generic other than this edit feature. Moreover, I think that PR would be best made post 4.2.0 release and maybe better made in a follow up release like a patch update since it is solely a refactor without any level of feature change.
2. Let me break this down here:
   1. Yes, I understand that edit provides not so much value when it is a store path. However, this is the behavior of nix edit and nixos-rebuild edit for non-path flake attributes. Maybe I should parse the attribute and do the same as I am for Installable::File as that actually follows the path and opens it directly.
   2. Before I continue, yes, I've tested all of the os platform code. All Darwin and home implementations are untested as I don't use either. But I figured you don't mind since we previously stated that we don't test it anyways. Plus, they're nearly identical with only slightly discrepancies. Although, I'm really not sure why you're asking if I tested this when I marked this as tested. Did you test this and somehow failed? If so, what did you run that failed for you? Can you show the exact arguments you ran?
   3. No, -o and --outpath are both supported by nix flake prefetch and can be found in the manual. I've even tested this on the command line and in the this PR. It prefetches the flake and creates a symlink to result. I thought it would be reasonable to create an outpath since I need the store path after prefetching. Only other way I figured possible would be to parse stdout for the store path, but this isn't very future proof, easily susceptible to bitrot, and felt a bit hacky.
   4. For configuration.nix, you'd have to use --file because this is legacy and not a flake. Although, I guess I should try to determine if it is a flake or not when using --file or leave if only for legacy behavior. What do you think is more reasonable?
   5. Huh, I could have sworn nix edit only worked for packages and legacyPackages. Maybe there was some other reason I didn't like it. I'll get back to you if I remember.
   6. Ha, I think i undid too far at one point because I could have sworn I removed nano as the default editor in this change as previously discussed in this PR that it was not appropriate and should instead fail with a sensible error when EDITOR is unset. I'm not sure what happened, but I'll add that back in.
3. What are you referring to by "blocking command" implementation?

Yeah, I'll look into maybe reusing the existing commands again, parse the flake attribute for paths, and add again the sensible error for when EDITOR is unset.

[midischwarz12]

Ah, okay. I remember why I don't like nixos-rebuild edit now. This failed o my flake with:

error: cannot find positional information for <flake>

I think I was thinking that this fails on flakes which generate their nixosConfigurations output from some subsequent function. So for example, if you many hosts and have some sort of function which loops through the contents to automatically generate this (which is a very common pattern), it would fail.

However, I tried this on another flake which does not do this. And I am receiving the same error. So I'm not really sure why I'm getting this now.

I looked at nixos-rebuild-ng and apparently they just use nix edit but this led me to the definition of runCommand for my flake. So not very optimal either.

[midischwarz12]

I hate to look stupid here. But I just did some light poking around with nixos-rebuild edit. And it seems that it does not respect --flake at all. So I'd either have to go with nix edit somehow or completely come up with my own solution here.

[midischwarz12]

@NotAShelf This should be what you want while being backwards compatible. Feel free to test and merge.

[faukah]

Bunch of code duplication, but you guys already talked about that.