search

nix-community / home-manager

Manage a user environment using Nix [maintainer=@rycee]
https://nix-community.github.io/home-manager/
MIT License
6.28k stars 1.68k forks source link

starship settings require ordered attrsets #2519

Open arcnmx opened 2 years ago

arcnmx commented 2 years ago

Is there an existing issue for this?

Issue description

Starship uses an IndexMap for its module settings that use dictionaries/attrsets so that they can be processed in declaration order. This doesn't translate well to nix, which doesn't really have ordered attrsets - and so certain settings are impossible to set properly via programs.starship.settings..

System information

host os: `Linux 5.15.4, NixOS, 22.05 (Quokka)`
version: `nix-env (Nix) 2.3.16`
berbiche commented 2 years ago

Hi,

Do you have an example setting that cannot be set currently with Home Manager?

arcnmx commented 2 years ago

Specifically - because starship requires that values be set in the order they should be processed - you cannot set...

[directory.substitutions]
"/a/b" = "b"
"/a" = "a"

because in nix, sets are always sorted by keys, so...

{
  directory.substitutions = {
    "/a/b" = "b";
    "/a" = "a";
  };
}

... results in an incorrect configuration:

[directory.substitutions]
"/a" = "a"
"/a/b" = "b"

and the module has no extraConfig that could be used to get around this limitation :<

rycee commented 2 years ago

Pretty unfortunate 🙁 May be worth adding an extraConfig of type.lines to cope with such situations.

rcerc commented 2 years ago

@arcnmx, @berbiche and @rycee, could format generators for DAGs be written, solving this issue for other modules as well? A toJSON function for DAGs would have to be implemented (see https://github.com/NixOS/nixpkgs/blob/master/pkgs/pkgs-lib/formats.nix), but that seems to be everything (other than maintaining the generators :slightly_smiling_face:).

Technically, JSON keys do not have an order, but remarshal (used by the generators) can preserve the order in its input JSON with the -p or --preserve-key-order flag.

stale[bot] commented 2 years ago

Thank you for your contribution! I marked this issue as stale due to inactivity. Please be considerate of people watching this issue and receiving notifications before commenting 'I have this issue too'. We welcome additional information that will help resolve this issue. Please read the relevant sections below before commenting.

If you are the original author of the issue

* If this is resolved, please consider closing it so that the maintainers know not to focus on this. * If this might still be an issue, but you are not interested in promoting its resolution, please consider closing it while encouraging others to take over and reopen an issue if they care enough. * If you know how to solve the issue, please consider submitting a Pull Request that addresses this issue.

If you are not the original author of the issue

* If you are also experiencing this issue, please add details of your situation to help with the debugging process. * If you know how to solve the issue, please consider submitting a Pull Request that addresses this issue.

Memorandum on closing issues

Don't be afraid to manually close an issue, even if it holds valuable information. Closed issues stay in the system for people to search, read, cross-reference, or even reopen – nothing is lost! Closing obsolete issues is an important way to help maintainers focus their time and effort.

arcnmx commented 2 years ago

Enjoy my horrible implementation of programs.starship.extraConfig, meaniebot.

(fwiw, a DAG approach would be quite neat)

rycee commented 2 years ago

I'm thinking that perhaps something like

{
  directory.substitutions = literalExpression ''
    "/a/b" = "b"
    "/a" = "a"
  '';
}

may be interesting?

arcnmx commented 2 years ago

eh a DAG is what I'd call interesting, that's a more boring approach :stuck_out_tongue: but sure, it could solve this!

(implementation might be a little complicated, plus unlike JSON there isn't necessarily such thing as a ubiquitous TOML literal value since the syntax is somewhat context-sensitive, you might have to disambiguate by saying literalTableKeyValues or something, but it can be hacked together regardless - and really depends on how much you want to integrate with the conversion process or just collect the non-standard literals to be appended after conversion of all other keys)

rcerc commented 2 years ago

I wrote a toJson function for DAGs and some functions that use remarshal to convert DAGs to files of other formats (e.g. TOML and YAML). The code is here. A problem I encountered was finding the right place for the to<format>File functions because I didn't think they should be placed in modules/lib, as they depend on the Nix package set. I extended the set in modules/default.nix and other files it appears in with another one in modules/pkgs/default.nix that includes the format generators. However, I am not sure if there is a more idiomatic way to avoid keeping these functions in modules/lib, so I was hoping to hear your thoughts on this.

stale[bot] commented 2 years ago

Thank you for your contribution! I marked this issue as stale due to inactivity. Please be considerate of people watching this issue and receiving notifications before commenting 'I have this issue too'. We welcome additional information that will help resolve this issue. Please read the relevant sections below before commenting.

If you are the original author of the issue

* If this is resolved, please consider closing it so that the maintainers know not to focus on this. * If this might still be an issue, but you are not interested in promoting its resolution, please consider closing it while encouraging others to take over and reopen an issue if they care enough. * If you know how to solve the issue, please consider submitting a Pull Request that addresses this issue.

If you are not the original author of the issue

* If you are also experiencing this issue, please add details of your situation to help with the debugging process. * If you know how to solve the issue, please consider submitting a Pull Request that addresses this issue.

Memorandum on closing issues

Don't be afraid to manually close an issue, even if it holds valuable information. Closed issues stay in the system for people to search, read, cross-reference, or even reopen – nothing is lost! Closing obsolete issues is an important way to help maintainers focus their time and effort.

rcerc commented 2 years ago

I am not sure if TOML name-value pairs are meant to convey an order according to the specification, but (if it is not bad practice to depend on this order) supporting the conversion from a DAG to a TOML file would probably be helpful in cases like these. Otherwise, do you think it would better to close this issue and go with the extra string configuration option?

peterhoeg commented 1 year ago

The string options are not great - starship isn't the only piece of software that expects a certain ordering, so having proper support for that solves a whole bunch of issues.

stale[bot] commented 1 year ago

Thank you for your contribution! I marked this issue as stale due to inactivity. Please be considerate of people watching this issue and receiving notifications before commenting 'I have this issue too'. We welcome additional information that will help resolve this issue. Please read the relevant sections below before commenting.

If you are the original author of the issue

* If this is resolved, please consider closing it so that the maintainers know not to focus on this. * If this might still be an issue, but you are not interested in promoting its resolution, please consider closing it while encouraging others to take over and reopen an issue if they care enough. * If you know how to solve the issue, please consider submitting a Pull Request that addresses this issue.

If you are not the original author of the issue

* If you are also experiencing this issue, please add details of your situation to help with the debugging process. * If you know how to solve the issue, please consider submitting a Pull Request that addresses this issue.

Memorandum on closing issues

Don't be afraid to manually close an issue, even if it holds valuable information. Closed issues stay in the system for people to search, read, cross-reference, or even reopen – nothing is lost! Closing obsolete issues is an important way to help maintainers focus their time and effort.

arcnmx commented 1 year ago

https://github.com/nix-community/home-manager/issues/2519#issuecomment-1086876577 is still a very intriguing solution, meaniebot

rcerc commented 1 year ago

I'm currently working on tidying (and repairing) the code I suggested to extend Nixpkgs in Home Manager so that we don't eventually run into problems with it, and I'm trying to not break compatibility (too badly, if at all necessary). I can file a draft pull request once I get somewhere though.

rcerc commented 1 year ago

Is #3481 a feasible change?

sersorrel commented 1 year ago

this seems like it's going to be a substantially bigger problem in the future: per https://github.com/greshake/i3status-rust/blob/master/NEWS.md#i3status-rust-0307, "In the future block = "..." will be required to be the first field of block configs", i.e. my current configuration (with things like alert = ... first rather than block = ...) would be invalid.

NobbZ commented 9 months ago

From my understanding, TOML by itself demands that the maps are unordered.

This is not an issue with HM, this is an issue with whatever expects the keys to be ordered.

pluiedev commented 7 months ago

From my understanding, TOML by itself demands that the maps are unordered.

This is not an issue with HM, this is an issue with whatever expects the keys to be ordered.

You're actually right! (TIL) — https://toml.io/en/v1.0.0#table

Under that, and until the next header or EOF, are the key/values of that table. Key/value pairs within tables are not guaranteed to be in any specific order.

I wonder how feasible it is to change how it works from starship's side though EDIT: Just made an issue upstream: starship/starship#5572