Skip to content

aerc: add assertion to limit per-account extraConfig to UI config#4196

Merged
ncfavier merged 2 commits intonix-community:masterfrom
dryya:aerc-fix
Jul 14, 2023
Merged

aerc: add assertion to limit per-account extraConfig to UI config#4196
ncfavier merged 2 commits intonix-community:masterfrom
dryya:aerc-fix

Conversation

@dryya
Copy link
Copy Markdown
Contributor

@dryya dryya commented Jul 4, 2023

Description

The aerc configuration file aerc.conf can contain 10 sections, but only the UI section supports what the aerc manual calls contextual configuration. This works by appending to the section heading either :account=name or :folder=bar. It's supported in config.accounts.email.accounts.<name>.aerc.extraConfig.*.*, as the aerc-accounts module applies a
function to add <name> to section headings.
However, this means home-manager will generate files like:

[general:account=default]
log-file=...

and the options will not be recognized by aerc.

This change only adds the account name to the UI section by adding a attrsets.filterAttrs check to mkAccountConfig. This could be confusing if a user only wants to apply their configuration to one account, and a possible alternative could be to change this option to something like config.accounts.email.accounts.<name>.aerc.uiConfig.*.
The error message about setting unsafe-accounts-conf = true has also been updated, to clarify it should be set
in the global programs.aerc.extraConfig section.

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

@github-actions github-actions bot added the mail HM email accounts, thunderbird, alot, notmuch, msmtp, meli... label Jul 4, 2023
Copy link
Copy Markdown
Member

@ncfavier ncfavier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know much about aerc, but it seems to me that if the only per-account configuration allowed is ui, then we should restrict the per-account extraConfig to that. Otherwise the other settings will basically end up in the global extraConfig, possibly causing conflicts.

So I would either add an assertion, or make the type of the per-account extraConfig a submodule with just an ui option.

Comment thread modules/programs/aerc-accounts.nix Outdated
@ncfavier
Copy link
Copy Markdown
Member

ncfavier commented Jul 4, 2023

For example:

extraConfig = mkOption {
  type = types.submodule {
    options.ui = mkOption {
      type = confSection;
      description = "Per-account UI configuration...";
    };
  };

@dryya dryya force-pushed the aerc-fix branch 3 times, most recently from 21b73e9 to 2f97933 Compare July 7, 2023 02:21
@ncfavier
Copy link
Copy Markdown
Member

ncfavier commented Jul 7, 2023

So it seems like we need a default value for both extraConfig and extraConfig.ui. Setting both to {} means that the per-account ui section will always be generated, which may or may not be a problem.

As alternatives we could make it default to null and only generate it if it's != null, or fall back to the assertion I mentioned.

The aerc configuration file `aerc.conf` can contain 10 different
sections, but only the UI section supports what the aerc manual calls
contextual configuration. This works by appending to the section heading
either `:account=name` or `:folder=bar`.

The aerc-accounts module, however, applied `mkAccountConfig` to each
section heading declared in
`config.accounts.email.accounts.<name>.aerc.extraConfig.*`. This means
home-manager will generate files with `[general:account=default]` and
the options will not be recognized by aerc.

To address this, and since it doesn't make sense for other sections to
only be under a single account's scope, an assertion has been added
to confirm that only sectons that support contextual config (i.e.,
only the UI section) is declared.

This also addresses confusions like declaring
`accounts.email.accounts.*.aerc.extraConfig.general.unsafe-accounts-conf
= true` and triggering a warning message because
`programs.aerc.extraConfig.general.unsafe-accounts-conf` was unset.

This commit also updated documentation throughout the aerc modules to
be in line with this change, and fixed minor typos/formatting therein.

Co-authored-by: Genevieve <genevieve@sunlashed.garden>
@dryya
Copy link
Copy Markdown
Contributor Author

dryya commented Jul 7, 2023

OK, I've changed accounts.email.accounts.*.aerc.extraConfigback to typeconfSections` for consistency/parsimony, and added an assertion to make sure it only has a UI section. The assertion is a little ugly so let me know if it's OK or not.

I also think that as of before this PR, it was technically possible (although it seems unlikely) for a user to have something like accounts.email.accounts.mail.aerc.extraConfig."ui:account=mail" = { sort = "from"; }; in their config. After this PR, they would get an assertion. Should I mention in the error that specifying the account is not necessary and should be removed, or is there another way to deal with this?

(PS: I appreciate you guiding me through this process!)

@dryya
Copy link
Copy Markdown
Contributor Author

dryya commented Jul 7, 2023

I ran the test suite again and I think one could also fix the issue and keep it as a submodule by doing something like

    accountsExtraConfig = mapAttrs accounts.mkAccountConfig
      # only write this section if there actually is per-account config
      (attrsets.filterAttrs
        (_: v: v.aerc.extraConfig.ui != { })
        # could add || v.aerc.extraConfig.bla != {} in future
        aerc-accounts);

without needing any assertions. (Defaulting to null causes some issues because the INI generator wants to recurse through the values). But then, there would just be this odd-one-out section, which doesn't feel great.

@ncfavier
Copy link
Copy Markdown
Member

ncfavier commented Jul 8, 2023

Silently discarding non-ui sections doesn't seem great

Comment thread modules/programs/aerc-accounts.nix Outdated
Comment thread modules/programs/aerc.nix Outdated
@dryya dryya changed the title aerc: don't rewrite section headings in per-account extraConfig aerc: add assertion to limit per-account extraConfig to UI config Jul 8, 2023
This commit adds a test case to check both the warning on unset
`unsafe-accounts-conf = true` when aerc accounts are configured
with Nix, and the new assertion when per-account configuration
contains unsupported subsections (i.e. general).

It also fixes minor formatting issues and typos.
@dryya dryya requested a review from ncfavier July 14, 2023 16:59
@ncfavier ncfavier merged commit bec87d5 into nix-community:master Jul 14, 2023
@dryya dryya deleted the aerc-fix branch July 15, 2023 17:32
antholeole pushed a commit to antholeole/home-manager that referenced this pull request Jul 24, 2023
…x-community#4196)

* aerc: fix per-account extraConfig section names

The aerc configuration file `aerc.conf` can contain 10 different
sections, but only the UI section supports what the aerc manual calls
contextual configuration. This works by appending to the section heading
either `:account=name` or `:folder=bar`.

The aerc-accounts module, however, applied `mkAccountConfig` to each
section heading declared in
`config.accounts.email.accounts.<name>.aerc.extraConfig.*`. This means
home-manager will generate files with `[general:account=default]` and
the options will not be recognized by aerc.

To address this, and since it doesn't make sense for other sections to
only be under a single account's scope, an assertion has been added
to confirm that only sectons that support contextual config (i.e.,
only the UI section) is declared.

This also addresses confusions like declaring
`accounts.email.accounts.*.aerc.extraConfig.general.unsafe-accounts-conf
= true` and triggering a warning message because
`programs.aerc.extraConfig.general.unsafe-accounts-conf` was unset.

This commit also updated documentation throughout the aerc modules to
be in line with this change, and fixed minor typos/formatting therein.

Co-authored-by: Genevieve <genevieve@sunlashed.garden>

* aerc: make assertion plaintext and add test case

This commit adds a test case to check both the warning on unset
`unsafe-accounts-conf = true` when aerc accounts are configured
with Nix, and the new assertion when per-account configuration
contains unsupported subsections (i.e. general).

It also fixes minor formatting issues and typos.
SimSaladin added a commit to SimSaladin/home-manager that referenced this pull request Aug 26, 2023
* upstream/master: (352 commits)
  flake.lock: Update
  qcal: add module
  bat: generate cache file in XDG cache home
  nixos: only refer to `users.users` if needed
  home-cursor: remove IFD when linking icon directories
  flake.lock: Update
  home-cursor: improve icon compatibility
  fix(qt): allow theming for apps started by systemd (nix-community#4349)
  flake.lock: Update
  fcitx5: add `fcitx5-config-qt` to test stub
  pqiv: only run tests on Linux platforms
  pqiv: add module
  Translate using Weblate (German)
  xplr: add module
  browserpass: support librewolf
  Translate using Weblate (Romanian)
  Translate using Weblate (Portuguese (Brazil))
  hyprland: remove xwayland.hidpi (nix-community#4302)
  home-manager: handle profile list in Nix >2.17
  lf: simplify option validation (nix-community#4334)
  programs.khal: uncomment locale config (nix-community#4319)
  kanshi: support adaptive sync (nix-community#4328)
  mu: add package option (nix-community#4325)
  modules: types.string throws error now (nix-community#4324)
  taffybar: Avoid restarting too quickly (nix-community#4316)
  aerc: do not use smtp-starttls (nix-community#4272)
  zsh: Add `zsh.history.ignoreAllDups` config option (nix-community#4248)
  jujutsu: fix zsh completion (nix-community#4305)
  Translate using Weblate (Korean)
  Translate using Weblate (Romanian)
  Translate using Weblate (German)
  flake.lock: Update
  network-manager-applet: remove `--sm-disable` flag
  himalaya: update a link to ticket
  nushell: deprecation of let-env (nix-community#4292)
  man: fix caches generation in cross-compiled system (nix-community#4294)
  fish: fix session vars build in cross-compiled system (nix-community#4293)
  Update translation files
  Translate using Weblate (Chinese (Simplified))
  gh: option to enable helper for additional hosts (nix-community#4288)
  home-manager: rework news command
  home-manager: skip manual in uninstall configuration
  gnome-terminal: add assertion on profile names
  hyprland: use `toKeyValue`'s `indent` argument (nix-community#4274)
  gpg: fix typo (nix-community#4277)
  firefox: make package nullable (nix-community#4113)
  git-sync: add news entry for darwin
  tmate: don't generate empty config file (nix-community#4271)
  gh-dash: add module
  git-sync: add darwin support
  flake.lock: Update
  hyprland: prioritize variables and beziers (nix-community#4263)
  hyprland: add module
  script-directory: fix documentation link (nix-community#4258)
  Translate using Weblate (Indonesian)
  Translate using Weblate (Ukrainian)
  Translate using Weblate (Spanish)
  Translate using Weblate (Swedish)
  docs: hide `_module.*` from NixOS/nix-darwin docs
  jujutsu: update for Jujutsu 0.8.0 (nix-community#4250)
  ripgrep: don't set env. variable if no config (nix-community#4254)
  docs: update for Markdown migration
  treewide: remove now-redundant `lib.mdDoc` calls
  docs: clean up after Markdown migration
  flake: remove temporary nixpkgs pin
  treewide: convert all option docs to Markdown
  treewide: adjust some DocBook for conversion
  treewide: `mkAliasOption` -> `mkAliasOptionMD`
  treewide: `mkPackageOption` -> `mkPackageOptionMD`
  treewide: convert options with tables to Markdown
  treewide: convert options with lists to Markdown
  treewide: convert custom `enable` docs to Markdown
  treewide: convert parameterized docs to Markdown
  treewide: manually convert some docs to Markdown
  treewide: fix `mkEnableOption` arguments
  treewide: add missing option descriptions
  docs: use `nixosOptionsDoc`
  flake: temporarily pin nixpkgs version
  version: add `isReleaseBranch`
  manual: add test
  Update translation files
  flake.lock: Update
  home-manager: remove Home Manager default paths
  imapnotify: Use JSON type for extraConfig (nix-community#4238)
  Translate using Weblate (French)
  zsh: fix custom syntax highlighting styles (nix-community#4236)
  i3-sway: allow arbitrary floating modifier (nix-community#4229)
  i3-sway: multiple outputs (nix-community#4223)
  home-cursor.nix: enable gtk module when enabling gtk config generation (nix-community#4144)
  aerc: add assertion to limit per-account extraConfig to UI config (nix-community#4196)
  flake.lock: Update
  ssh-agent: add assertion and fix news entry (nix-community#4210)
  imapnotify: move test
  imapnotify: use direct nix store path for config
  flake.lock: Update
  nushell: add login.nu configuration option
  himalaya: fix notmuch backend
  swayosd: add module
  pyenv: add module
  darcs: add module
  tests: some minor cleanups
  goimapnotify: remove test dependency on notmuch
  unison: Allow using same option multiple times (nix-community#4208)
  imapnotify: Add launchd agent (nix-community#3291)
  i3: remove deprecated example in i3.config.startup (nix-community#4201)
  antidote: static file move to /tmp
  Translate using Weblate (Indonesian)
  PULL_REQUEST_TEMPLATE.md: add maintainer cc section (nix-community#4193)
  programs.khal
  vdirsyncer: synchronize metadata as well (nix-community#4167)
  i18n: Use glibcLocales from NixOS if possible (nix-community#2333) (nix-community#4177)
  xfconf: remove properties with null values (nix-community#4192)
  home-manager: Use `path:` URI type for flake default (nix-community#3646)
  ci: autolabel calendars PRs (nix-community#4165)
  flake.lock: Update (nix-community#4161)
  news: fix typo in programs.zsh.antidote entry
  ssh-agent: init module (nix-community#4178)
  chromium: fix `commandLineArgs` to use the user specified package (nix-community#4175)
  sxhkd: allow usage of derivations as keybind commands (nix-community#4169)
  README: Remove the pills (nix-community#4181)
  lsd: use -A instead of -a in aliases (nix-community#4173)
  kakoune: add defaultEditor option
  docs: update link to allowed users setting (nix-community#4176)
  zsh: allow setting custom syntax highlighting styles (nix-community#4122)
  flake: add formatter (nix-community#3620)
  broot: fix test (nix-community#4170)
  antidote: fix .dot path
  ci: build manual and push to home-manager.dev
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

mail HM email accounts, thunderbird, alot, notmuch, msmtp, meli...

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants