Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

himalaya: adjust module for v1.0.0-beta #4839

Merged
merged 1 commit into from
Feb 5, 2024

Conversation

soywod
Copy link
Contributor

@soywod soywod commented Jan 1, 2024

Description

Adjust module to match the v1.0.0-beta. I just open the PR as draft (waiting for NixOS/nixpkgs#278088).

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

@toastal

@github-actions github-actions bot added the mail label Jan 1, 2024
@soywod soywod changed the title himalaya: adjust module for v1.0.0-beta himalaya: adjust module for v1.0.0-beta Jan 1, 2024
Copy link
Contributor

@toastal toastal left a comment

Choose a reason for hiding this comment

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

Generally looks good (haven’t had a chance to run it). I didn’t expect watch to be added already. My only concern is those cuddled attrset braces with the if statement had me thrown for a loop & I think has a negative impacted on readability.

modules/programs/himalaya.nix Outdated Show resolved Hide resolved
modules/programs/himalaya.nix Outdated Show resolved Hide resolved
@soywod
Copy link
Contributor Author

soywod commented Jan 5, 2024

I didn’t expect watch to be added already

The watch (and notify) were already there before, I just refactored the code to match the new watch API which combines both previous watch (running shell commands) and previous notify (send system notifications).

I agree about the if, I will propose an alternative.

Thank you for your feedback!

@soywod soywod force-pushed the himalaya-v1.0.0-beta branch 3 times, most recently from da69fae to 5ce6c1b Compare January 28, 2024 08:59
@soywod soywod marked this pull request as ready for review January 28, 2024 08:59
@soywod soywod requested a review from toastal January 28, 2024 11:10
@toastal
Copy link
Contributor

toastal commented Jan 28, 2024

@soywod do you have a recent rebase from the default branch atop this?

@soywod
Copy link
Contributor Author

soywod commented Jan 28, 2024 via email

@toastal
Copy link
Contributor

toastal commented Jan 28, 2024 via email

@erooke
Copy link

erooke commented Jan 28, 2024

For whatever it's worth while trying to rebuild my configuration off of this branch I got the following error

error:
       … while calling the 'derivationStrict' builtin

         at /builtin/derivation.nix:9:12: (source not available)

       … while evaluating derivation 'home-manager-generation'
         whose name attribute is located at /nix/store/9s5qs4hni9fj88x79iw6im7amv7ghb76-source/pkgs/stdenv/generic/make-derivation.nix:352:7

       … while evaluating attribute 'buildCommand' of derivation 'home-manager-generation'

         at /nix/store/9s5qs4hni9fj88x79iw6im7amv7ghb76-source/pkgs/build-support/trivial-builders/default.nix:98:16:

           97|         enableParallelBuilding = true;
           98|         inherit buildCommand name;
             |                ^
           99|         passAsFile = [ "buildCommand" ]

       (stack trace truncated; use '--show-trace' to show the full trace)

       error: attribute 'accounts' missing

       at /nix/store/kgyb4snyic74z8jyybx1zclh9dk70q8n-source/modules/programs/himalaya.nix:68:33:

           67|         backend = "notmuch";
           68|         notmuch.database-path = config.accounts.email.maildirBasePath;
             |                                 ^
           69|       });

Here is a minimal config which seems to trigger the same breakage:

{...}: {
  home = {
    stateVersion = "24.05";
    username = "test";
    homeDirectory = /home/test;
  };

  accounts.email = {
    maildirBasePath = ".mail";

    accounts.test = {
      primary = true;
      address = "test@mail.com";
      userName = "test@mail.com";
      realName = "Test Testerton";

      notmuch.enable = true;
      himalaya.enable = true;
    };
  };
  programs.notmuch.enable = true;
  programs.himalaya.enable = true;
}

This was using the current nixpkgs-unstable and github:soywod/home-manager/himalaya-v1.0.0-beta. If needed I could supply a flake.lock and flake.nix but that felt like un-needed noise.

@soywod soywod force-pushed the himalaya-v1.0.0-beta branch 2 times, most recently from 357e987 to 5f8e067 Compare January 28, 2024 19:42
@soywod
Copy link
Contributor Author

soywod commented Jan 28, 2024 via email

@erooke
Copy link

erooke commented Jan 28, 2024

I pushed a fix, it should be good now!

Yup the home manager config now builds on my end! Himalaya seems unhappy with the generated configuration now though.

$ himalaya accounts list

Error: cannot parse config file at "/home/ethan/.config/himalaya/config.toml"

Caused by:
    TOML parse error at line 1, column 1
      |
    1 | [rooke]
      | ^
    unknown variant `notmuch`, expected one of `maildir`, `imap`, `smtp`, `sendmail`    

@soywod
Copy link
Contributor Author

soywod commented Jan 28, 2024 via email

@soywod
Copy link
Contributor Author

soywod commented Jan 28, 2024 via email

@erooke
Copy link

erooke commented Jan 28, 2024 via email

@soywod
Copy link
Contributor Author

soywod commented Jan 28, 2024

@rycee what is the best approach for this kind of situation?

@toastal
Copy link
Contributor

toastal commented Jan 29, 2024

I got the config to build but …

$ himalaya account list
Error: cannot parse config file at "$XDG_CONFIG_HOME/himalaya/config.toml"

Caused by:
    TOML parse error at line 1, column 1
      |
    1 | display-name = "toastal"
      | ^^^^^^^^^^^^^^^^^^^^^^^^
    invalid type: integer `46`, expected struct TomlAccountConfig

@soywod
Copy link
Contributor Author

soywod commented Jan 29, 2024 via email

@toastal
Copy link
Contributor

toastal commented Jan 29, 2024

There is no way to know the correct location of errors at the moment

Well that kinda sucks ha. DM‘d my config on Matrix.

@soywod
Copy link
Contributor Author

soywod commented Jan 29, 2024 via email

@soywod
Copy link
Contributor Author

soywod commented Jan 29, 2024

I cannot understand why .override does not work from the home manager module but work outside of it: error: attribute 'override' missing

https://github.com/nix-community/home-manager/actions/runs/7702476916/job/20990851373?pr=4839

Edit: somehow the derivation path points to a dummy one: /nix/store/ybh6rq30zqxp2kg0fapimf1rcgpy1vp5-dummy.drv, could it be because a mismatch between my local version of himalaya (beta.2) and the one available on nixpkgs-unstable (beta)? Let's wait for NixOS/nixpkgs#284358.

Copy link
Contributor

@toastal toastal left a comment

Choose a reason for hiding this comment

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

Wait or not, this does work for me now. Thanks for the help @soywod

Copy link
Contributor

@toastal toastal left a comment

Choose a reason for hiding this comment

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

Tested new changes.

@rycee
Copy link
Member

rycee commented Feb 5, 2024

All good to merge?

@toastal
Copy link
Contributor

toastal commented Feb 5, 2024

It works well enough for me & the beta2 has been merged into nixpkgs.

@rycee rycee merged commit 3c6f2dd into nix-community:master Feb 5, 2024
3 checks passed
@rycee
Copy link
Member

rycee commented Feb 5, 2024

Cool, thanks! Merged to master now 🙂

@soywod soywod deleted the himalaya-v1.0.0-beta branch February 5, 2024 23:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants