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

fix: Issue 947 set NIX_PROFILES for use-xdg-base-directories #962

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

bestlem
Copy link

@bestlem bestlem commented Jun 2, 2024

#947 -

Puts the directory that nHome Manager uses when xdg is enabled and use-xdg-base-directories is set (~/.local/state/nix/profile/bin) into NIX_PATHS and thus the paths in the shells.

HM uses this directory instead of ~/.nix-profiles in this case

@@ -159,6 +159,7 @@ in
# Use user, default and system profiles.
environment.profiles = mkMerge [
(mkOrder 800 [ "$HOME/.nix-profile" ])
(mkOrder 800 [ "\${XDG_STATE_HOME:-$HOME/.local/state}/nix/profile" ])
Copy link

@brendanzab brendanzab Aug 6, 2024

Choose a reason for hiding this comment

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

I was looking at this today and noticed that in NixOS/nixpkgs#260764 this was updated to remove the shell expansion:

    environment.profiles = [
      "$HOME/.nix-profile"
      "\${XDG_STATE_HOME}/nix/profile"
      "$HOME/.local/state/nix/profile"
      "/etc/profiles/per-user/$USER"
    ];

Not sure if a similar approach should be used here, at least for consistency? Might also interact with shells like fish… but not sure!

Copy link

@brendanzab brendanzab Aug 6, 2024

Choose a reason for hiding this comment

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

When trying to add these manually to environment.profiles, I also noticed that the ${XDG_STATE_HOME} escape interacts poorly with clo4’s workaround for fish paths #122 (comment). Instead I just used this, hard-coding "$XDG_STATE_HOME/nix/profile" without the braces:

    # FIXME: Needed to address bug where $PATH is not properly set for fish.
    #        See: https://github.com/LnL7/nix-darwin/issues/122#issuecomment-1659465635
    programs.fish.loginShellInit =
      let
        # This naive quoting is good enough in this case. There shouldn't be any
        # double quotes in the input string, and it needs to be double quoted in case
        # it contains a space (which is unlikely!)
        dquote = str: "\"${str}\"";

        # FIXME: Workaround for incorrect XDG paths https://github.com/LnL7/nix-darwin/issues/947
        #
        # Nix-darwin should set these here: https://github.com/LnL7/nix-darwin/blob/91010a5613ffd7ee23ee9263213157a1c422b705/modules/environment/default.nix#L159-L163
        # as is set for nixOS: https://github.com/Gerg-L/nixpkgs/blob/6e8bbb279bd525b516e49021e5cb976f0b1f79e4/nixos/modules/config/users-groups.nix#L764-L769
        # In the future we could use `config.environment.profiles` instead of hard-coding these.
        profiles = [
          "$HOME/.nix-profile"
          "$XDG_STATE_HOME/nix/profile"
          "$HOME/.local/state/nix/profile"
          "/etc/profiles/per-user/$USER"
        ];
      in ''
        fish_add_path --move --prepend --path ${lib.concatMapStringsSep " " (path: dquote "${path}/bin") profiles}
        set fish_user_paths $fish_user_paths
      '';

Not sure if a better solution to #122 would solve this… I don't know if NixOS’ environment.paths breaks fish users - I would have imagined that would be fixed by now if that was the case.

Copy link
Author

@bestlem bestlem Aug 6, 2024

Choose a reason for hiding this comment

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

No that won't work as what happens if $XDG_STATE_HOME is not defined. Covered in NixOS/nixpkgs#260764 (comment) NixOS/nixpkgs#260764 (comment) I suppose.

#122 issue is macOS specific and has to do with Apple's path_helper which is just broken.(It forces things to the end of the path so you cannot override macOS system binaries)

Also I think it is more complex as I use home-manager as well - for fish I just manually set nix paths after all bits of nix and fish have finished playing around.

This fix is for bash and zsh setup, how it works on fish I have not really tested as as you note it gets confusing.

The issue is that the XDG path if it exists is not on $PATH in current nix-darwin

Copy link
Author

Choose a reason for hiding this comment

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

My issues with not finishing is more not understanding how PRs work with the testing.

Copy link
Author

Choose a reason for hiding this comment

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

The nixes one looks incorrect to me

 environment.profiles = [
      "$HOME/.nix-profile"
      "\${XDG_STATE_HOME}/nix/profile"
      "$HOME/.local/state/nix/profile"
      "/etc/profiles/per-user/$USER"
    ];

Why do we have "$HOME/.local/state/nix/profile" as if using xdc then the second line is sufficient isn't it.
I will remove the conditional XDG_STATE_HOME though as we probably do not want to set it.

@brendanzab What is the difference between "$XDG_STATE_HOME/nix/profile" and "${XDG_STATE_HOME}/nix/profile" I think we need a non xdc user to test.

Choose a reason for hiding this comment

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

Ahh yeah, that sounds like it could be a problem, for people not using XDG.

The issue for me was that fish didn't like the bash-style expansion (i.e. ${XDG_STATE_HOME} or${XDG_STATE_HOME:-$HOME/.local/state}) in combination with clo4's workaround. Without that workaround maybe the path expansion would be handled before fish gets it? I'm not sure how environment.profiles gets provided to fish. Sorry if that is confusing… it's becoming sort of a complicated set of workarounds alas.

Copy link

@brendanzab brendanzab Aug 7, 2024

Choose a reason for hiding this comment

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

Feel free to ignore me at any rate, I think your original solution might be better… (I don't maintain this project, I was just trying to move my nix files out of my home directory), but yeah this might end up breaking people’s workarounds unexpectedly, hah.

Copy link

@brendanzab brendanzab Aug 7, 2024

Choose a reason for hiding this comment

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

#122 issue is macOS specific and has to do with Apple's path_helper which is just broken.(It forces things to the end of the path so you cannot override macOS system binaries)

Ahh, ended up finding https://gist.github.com/Linerre/f11ad4a6a934dcf01ee8415c9457e7b2, which described the problem really well.

Copy link
Author

Choose a reason for hiding this comment

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

As for fish my config does not seem to have a problem with the syntax here.
My nix-darwin config includes

 programs = {
      bash.enable = true;
      zsh.enable = true;
      fish = {
        enable = true;
        useBabelfish = true;
        # babelfishPackage = pkgs.babelfish;
        vendor = {
          completions.enable = true;
          config.enable = true;
          functions.enable = true;
        };
        # xonsh.enable = true;
      };
    };
    environment = {
      shells = with pkgs; [
        bash
        zsh
        fish
      ];

      # Install completions for system packages.
      pathsToLink = [
        "/share/bash-completion"
        "/share/fish"
        "/share/zsh"
      ];
    };

Copy link
Contributor

Choose a reason for hiding this comment

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

From #962 (comment)

Why do we have "$HOME/.local/state/nix/profile" as if using xdc then the second line is sufficient isn't it.

Unfortunately not. XDG specifies that if XDG_STATE_HOME isn’t set, then $HOME/.local/state should be used. On the NixOS side, it looks like ${XDG_STATE_HOME:-$HOME/.local/state} interacted poorly with PAM, and so replacing the parameter expansion with two separate paths (the first hopefully not existing if XDG_STATE_HOME isn’t set) seems like a reasonable workaround.

I have

environment.profiles = lib.mkOrder 801 [
  "$XDG_STATE_HOME/nix/profile"
  "$HOME/.local/state/nix/profile"
];

in my darwin-configuration.nix, and it is working fine (but I do have everything using XDG, so I can’t test the non-XDG situation).

don't set XDG_STATE_HOME if not set, rely on directory just not
existing as nixos does. (I copied the original nixos version which
has now changed)

The other fix is toi fix the order to see if tests work.
@bestlem
Copy link
Author

bestlem commented Aug 6, 2024

I hate POSIX shell.

I have now read all the nixes change and the shell manuals.

Do we have PAM for use under Darwin?

If not then the shell check on XDG_STATE_HOME is the correct way.

If not the nixes fix is a mess.

I think I can get my old way to run and see where an error is as the GitHub script does print out the PATH

But I do not see anime error message saying text X failed and what it compares,

@bestlem
Copy link
Author

bestlem commented Aug 21, 2024

On reviewing more, that means I tried to work it out from bash/zsh documentation rather than copying nixes. NB I use fish and manually set the path so did not notice the latest change as per nixes does not work.

/etc/profile is the first file bash reads on an interactive login shell.

Apple's provided version runs path_helper and then sources /etc/bashrc

This means that XDG_STATE_HOME is not set before the changes here are read.

In zsh easier to follow /etc/zshenv loads the variables here and again is before ~/.zshenv so XDG* are not set.

So I think the change in nixos breaks things. But I have no Linux setup to test.

Even the fix I proposed and was the previous one in nixos only works by convention. That being that XDG_STATE_HOME is in ~/.local/state - If for example I wanted it in ~/Library/Application Support/xdgstate then it would fail.

@sellout
Copy link
Contributor

sellout commented Sep 17, 2024

This means that XDG_STATE_HOME is not set before the changes here are read.

Hrmm, looking at the code matches what you’re saying, but I’m not seeing /nix/profile in my PATH or NIX_PROFILES, so it seems like the XDG_STATE_HOME is set sufficiently early somehow. (Granted, my XDG_STATE_HOME is set to the standard place, and duplicates are removed in both cases, so it’s not that obvious, but I’m pretty sure that nothing is filtering non-existent paths from PATH)

Even the fix I proposed and was the previous one in nixos only works by convention. That being that XDG_STATE_HOME is in ~/.local/state - If for example I wanted it in ~/Library/Application Support/xdgstate then it would fail.

Yeah, this would be bad.

I have been thinking about setting my XDG_*_HOME to more Mac-specific places, so maybe this is the time to try it …

@bestlem
Copy link
Author

bestlem commented Sep 17, 2024

I think what is the issue is mixing what home-manager sets up and what nixos/nix-darwin sets.

If running home-manager the hm_session_vars.sh file sets the XDG_ variables after the code here is run.
This is set in ~/.profile that is read by ~/.bash* files. So if as under most Unixes the login shell is read then these are set early. But macOS does not do that.

Another way out is never to run home-manager on its own and always via nix-darwin and with useUserPackages = true then everything is in "/etc/profiles/per-user/$USER" and so the PATHS in ~ have nothing in them.

Or the latter might depend on frameworks - I now use Snowfallorg/lib and the binaries end up in ${XDG_STATE_HOME}/nix/profile I had a previous incomplete one that copied things around so that HM just used the /etc paths.

@bestlem
Copy link
Author

bestlem commented Sep 18, 2024

As for no /nix/profile you won't get it unless you are using some version of this PR.

nix-darwin justi does not enter any line in PATH for the XDG path - which was the simple thing I tried to fox by copying nixes

But I could not get the test to pass then nixes changed and then I actually looked at the issue and gone down too many rabbit holes.

@sellout
Copy link
Contributor

sellout commented Sep 18, 2024

As for no /nix/profile you won't get it unless you are using some version of this PR.

Yeah, see my earlier #962 (comment) – I am using the NixOS approach to this problem.

But I understand this is in some pretty fragile generated shell stuff (and repurposed for other shells, and PAM, etc.) so fingers crossed that there’s something that works in the general case.

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