Skip to content

Conversation

@sellout
Copy link

@sellout sellout commented Oct 5, 2025

Description

  • Don’t require Linux – everything here works fine on Darwin, etc.;
  • add a nullable package option;
  • change the syntax for extraConfig (use MISC instead of XDG_MISC_DIR, the same way xdg-user-dirs-update works); and
  • add a setSessionVariables option, which defaults to true to maintain the current behavior.

The last two changes are conditionalized on 25.11, so in future XDG_MISC_DIR will be disallowed and setSessionVariables will default to false.

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.

  • If this PR adds a new module

    • Added myself as module maintainer. See example.
    • Generate a news entry. See News
    • Basic tests added. See Tests
  • If this PR adds an exciting new feature or contains a breaking change.

    • Generate a news entry. See News

@sellout sellout force-pushed the non-linux-xdg-user-dirs branch 2 times, most recently from d2f86e2 to 0342467 Compare October 5, 2025 05:39
@sellout sellout marked this pull request as ready for review October 5, 2025 05:42
@home-manager-ci home-manager-ci bot requested a review from pacien October 5, 2025 05:43
Copy link
Collaborator

@khaneliman khaneliman left a comment

Choose a reason for hiding this comment

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

Like the changes overall, but would be nice to split up the commits a little to separate thoughts.

  • null package support
  • darwin support
  • setSessionVariables option
  • directories/bindings transformation

Copy link
Contributor

@ambroisie ambroisie left a comment

Choose a reason for hiding this comment

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

If I understand correctly, we should move away from setting XDG_*_DIR as a shell session variable, and make it so every shell uses xdg-user-dirs instead?

I'm curious, why is xdg-user-dirs preferred anyway, given that home-manager owns the configuration and creates the directories?

@sellout
Copy link
Author

sellout commented Oct 6, 2025

If I understand correctly, we should move away from setting XDG_*_HOME as a shell session variable, and make it so every shell uses xdg-user-dirs instead?

No, XDG_*_DIR shouldn’t be set in shells at all. To find the “DESKTOP” directory, users should use xdg-user-dir DESKTOP instead of $XDG_DESKTOP_DIR. The docs and the idiosyncratic config format make this far from obvious, but they do say, “It […] sets up a config file in $(XDG_CONFIG_HOME)/user-dirs.dirs […] that applications can read to find these directories.”

I'm curious, why is xdg-user-dirs preferred anyway, given that home-manager owns the configuration and creates the directories?

You’re right, it won’t make much of a difference under Home Manager, because xdg-user-dirs-update is disabled, so these directories are less likely to be modified dynamically1. The bigger concern IMO is with writing portable code. If users have the expectation that XDG_*_DIR is set, they’re more likely to write programs that rely on the environment, which won’t necessarily work when running on some other system using xdg-user-dirs.

Another issue that arises with the environment variables is that a user may assume that

XDG_DESKTOP_DIR="$HOME/temp-desktop" gnome-mangle-desktop

will protect their Desktop directory from whatever mangling is about to occur, when it’s unlikely to have any effect.

Of course, having the vars set in the environment can be convenient. And the idiosyncratic format2 of user-dirs.dirs is intended to make it source-able without trampling on existing variables3 (for handling in shell scripts). But because of the potential sync & portability issues, I think users should explicitly make the decision to do so. If you don’t think that’s compelling, I’m cool with keeping the default as true, but I need the option so I can exclude the vars from my environment.

Footnotes

  1. Well, there is still some dynamicity – after home-manager switch, xdg-user-dir DESKTOP will receive the up-to-date value while $XDG_DESKTOP_DIR from any already-running shell or program will receive the pre-switched value.

  2. There’s already a comment in the Nix module that wonders why the values need to be quoted. It’s because it’s not actually a shell script. E.g., lines containing settings have to match the regex ^[ \t]*XDG_(.*)_DIR="(\$HOME)?/(.+)" (you can’t reference $XDG_CONFIG_DIR or things like that, only a magical $HOME token). It’s just misleadingly similar to a shell script.

  3. Except XDG_RUNTIME_DIR, which you’d think XDG might have been aware of.

@ambroisie
Copy link
Contributor

Thank you for the thorough answer, you're making some good points!

Everything here works on Darwin, etc.
It defaults to `true` to maintain the current behavior.

This is conditionalized on 25.11, so in future `setSessionVariables` will
default to `false`.
E.g., use `MISC` instead of `XDG_MISC_DIR`, the same way `xdg-user-dirs-update`
works.

This is conditionalized on 25.11, so in future `XDG_MISC_DIR` will be disallowed.
@sellout sellout force-pushed the non-linux-xdg-user-dirs branch from 0342467 to c3f3957 Compare October 8, 2025 04:44
Comment on lines +148 to +158
// (
if lib.versionOlder config.home.stateVersion "25.11" then
lib.mapAttrs' (
k:
let
name = lib.match "XDG_(.*)_DIR" k;
in
lib.nameValuePair (if name == null then k else lib.elemAt name 0)
) cfg.extraConfig
else
cfg.extraConfig
Copy link
Collaborator

@khaneliman khaneliman Oct 10, 2025

Choose a reason for hiding this comment

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

Could we make this a louder transition that gives people a chance to know about the effect through an apply function with a warning? That way, we can detect if the string matches the old format, perform the transform, and then warn about the deprecation.

Just gives us the ability to be a true deprecation and can eventually remove the extra logic / tech debt.

Copy link
Author

Choose a reason for hiding this comment

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

Sorry for letting this languish …

Does this mean the "25.11" conditionalization should be removed completely in favor of the warning?

And should the true/false conditionalization for setSessionVariables stay the way it is (since it’s more of a trivial switch), or should it also move to the warning/deprecation style?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't mind the stateVersion stuff as a way to prevent breaking existing configs. But, it would be nice to add a warning inside this function when we are going through the old behavior so people can update to the new behavior and we have the ability to remove this stateVersion logic in the future to keep the module simpler.

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