Skip to content

Conversation

@adamslc
Copy link

@adamslc adamslc commented Dec 17, 2025

Description

Unfortunately pipsync will throw an error if it trys to write into a
nonexistant local storage directory. This patch ensures that all local
storage directories are present for accounts with pimsync enabled.

I have chosen to not gate this behind a config option since it is required in
order for pipsync to function correctly and it is a no op if the directories
already exist. But I could be convinced otherwise.

Also, I am a Nix newbie so feel free to critique my style or approach.

Checklist

  • Change is backwards compatible.

  • Code formatted with nix fmt or
    nix-shell -p treefmt nixfmt deadnix keep-sorted --run treefmt.

  • Code tested through nix run .#tests -- test-all or
    nix-shell --pure tests -A run.all.

  • 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.

Unfortunately pipsync will throw an error if it trys to write into a
nonexistant local storage directory. This patch ensures that all local
storage directories are present for accounts with pimsync enabled.
@teto
Copy link
Collaborator

teto commented Dec 17, 2025

havent had the time to upstream but instead of an activation script it is better to use tmpfiles (a systemd feature, you can check its usage in nixpkgs or hm).

systemd.user.tmpfiles.rules = [
    # Type Path                      Mode User Group Age         Argument
    # todo loop over the different calendars 
    "d ${config.accounts.calendar.basePath}/fastmail 0755 teto users"
  ];

@adamslc
Copy link
Author

adamslc commented Dec 21, 2025

Thanks for the suggestion. I have implemented that change and it appears to be working on my system.

@teto
Copy link
Collaborator

teto commented Dec 21, 2025

cool. CI fails with evaluation errors from this code apparently.

@adamslc
Copy link
Author

adamslc commented Dec 21, 2025

Looks like a bug in modules/misc/tmpfiles.nix where ${pkgs.systemd} is being expanded to @systemd@ instead of an absolute path. I don't understand Nix well enough to see why that would be the case.

cc: @bmrips @dawidsowa

@bmrips
Copy link
Contributor

bmrips commented Dec 21, 2025

Looks like a bug in modules/misc/tmpfiles.nix where ${pkgs.systemd} is being expanded to @systemd@ instead of an absolute path. I don't understand Nix well enough to see why that would be the case.

cc: @bmrips @dawidsowa

The test environment stubs packages as @<drv-name>@ to prevent them from being built. Hence, ${pkgs.systemd}/... = @systemd@/... is not an absolute path in the test environment and the type check fails. To prevent that, set test.stubs.systemd.outPath = null;.

@adamslc
Copy link
Author

adamslc commented Dec 22, 2025

@bmrips thanks for the explanation. With that fix, all tests pass on linux.

However, tmpfiles does not work on Darwin. As currently implemented, the directories are created on linux and nothing is done for Darwin. This is an improvement over the current behavior but it is unsatisfying that it does not fix the problem on all platforms. Thoughts?

systemd.user.tmpfiles.rules = tmpFileRules;
systemd.user.tmpfiles.rules = lib.optionals (
pkgs.stdenv.hostPlatform.system == lib.platforms.linux
) tmpFileRules;
Copy link
Contributor

Choose a reason for hiding this comment

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

Using pkgs.stdenv.hostPlatform.isLinux is the idiomatic way.

@bmrips
Copy link
Contributor

bmrips commented Dec 22, 2025

However, tmpfiles does not work on Darwin. As currently implemented, the directories are created on linux and nothing is done for Darwin. This is an improvement over the current behavior but it is unsatisfying that it does not fix the problem on all platforms. Thoughts?

For Darwin, you could still use the home.activation method since there is no equivalent for tmpfiles afaik.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants