Port over the NixOS fish module #50

cbarrett · 2017-10-07T02:14:34Z

This PR essentially replaces this file with the nixos fish module. The only difference between the two now is nuking the reference to environment.shells 🎉

The NixOS fish module has a number of nice features that we're gaining, such as more customizable initialization.

LnL7

Looks great!

LnL7 · 2017-10-07T10:01:39Z

modules/programs/fish.nix

+      set fish_function_path ${pkgs.fish-foreign-env}/share/fish-foreign-env/functions $__fish_datadir/functions
+
+      # For "reasons not remembered," setEnvironment does not set PATH, so do it here
+      set PATH ${replaceStrings [":"] [" "] config.environment.systemPath} 


I think it was something related to ordering, but I guess it's actually useful in this case. How is the separator handled on nixos?

Since setting the PATH happens inside fenv's temporary bash environment, the foreign-env plugin for Oh My Fish! handles converting the separator for us. This applies just as well to nix-darwin since we're also using fenv. :-)

Ah, interesting. I might change it to just set environment.variables.PATH at some point then.

LnL7 · 2017-10-07T10:05:14Z

modules/programs/fish.nix

+      set PATH ${replaceStrings [":"] [" "] config.environment.systemPath} 
+
+      # source the NixOS environment config
+      fenv source ${config.system.build.setEnvironment}


I inlined this in the other modules since nothing actually needed a separate file. But I can change this to a writeText and use the text attribute instead.

See a8b4770 and e8355cd, that should resolve most of differences with the nixos modules.

yurrriq · 2017-10-07T21:05:46Z

Thanks for this. I’m reading the diff on my phone and it looks like you’ve done away with variables in the config. I use that in system config. Is there another approach now?

therealpxc · 2017-10-09T17:31:58Z

modules/programs/fish.nix

-
-      ${concatStringsSep "\n" fishVariables}
+      # if we haven't sourced the general config, do it
+      if not set -q __fish_nixos_general_config_sourced


@LnL7 @cbarrett

do we care that references to NixOS have now ended up in the module? Would it be worth adding a PR to Nixpkgs which changes these to generic names, perhaps substituting nix for nixos and makes a note that this path is depended-on in nix-darwin and potentially other declarative configuration/module systems for Nix and not just NixOS?

Is it worth it just for the name change? Is it worth it to leave a note to Nixpkgs/NixOS developers so they know to at least ping people should that change?

Good point. I'm down with changing the names to nix_darwin instead of nixos. How do others feel?

therealpxc · 2017-10-09T17:36:29Z

modules/programs/fish.nix

+    environment.etc."fish/foreign-env/interactiveShellInit".text = cfge.interactiveShellInit;
+    environment.etc."fish/foreign-env/extraInit".text= cfge.extraInit;
+
+    environment.etc."fish/nixos-env-preinit.fish".text = ''


In the Fish package in Nixpkgs, Fish looks for this file, /etc/fish/nixos-env-preinit.fish. This is fine for us, but it means that we have to 'own' /etc/fish in nix-darwin. If we changed the Fish package to refer to /etc/static/fish/nixos-env-preinit.fish or something like that, this piece (and other pieces, if we changed them accordingly) would work even if nix-darwin can't or doesn't write directly to /etc/fish. Is that desirable? I don't think it would disrupt the way things work on NixOS.

tl;dr: Can we safely assume that /etc/static is where both nix-darwin and NixOS will write Fish config files to, and if so, should we change the modules (on both) to refer to those paths instead of the ones that live under /etc?

therealpxc · 2017-10-09T17:56:11Z

@yurrriq Good catch. I think maybe it would be best if we could use a global 'variables' attribute in config.environment and make sure that those variable changes are represented in setEnvironment or extraInit. (There might already be one; I haven't looked.)

I don't think that the foreign-env plugin currently applies aliases, but I think if we could enhance it to do so that would be ideal, because then we could put all of this stuff in generically as POSIX shell scripts and have fenv take care of all of it. Then setting variables can, for users, be independent of shell selection.

If this got through as it is now, you could set those variables with export directives inside shellInit or any of the other *ShellInit attributes in the your nix-darwin configuration.

I don't think you should have to, though. Being able to set aliases and variables directly, without worrying about the logic of when those things get sourced seems like it should be a feature. It looks like it's missing in the NixOS module, so I will seek to update that as well according to what we decide is a good idea here.

cbarrett · 2017-10-09T18:03:51Z

Thanks for all the comments! I'll take a look at addressing them tonight hopefully, sometime this week for sure.

therealpxc · 2017-10-10T15:28:43Z

Hey y'all. I just submitted a PR for Fish in Nixpkgs which makes it so that Fish installed via Nix takes care of NIxifying the environment automatically and transparently, regardless of whether you're using Fish on NixOS, Nix with GNU+Linux, Nix on macOS, or with nix-darwin.

It still needs more manual testing and probably other work (I think we should add some tests for initialization behavior as well), but it's ready for your comments. I'm using it with nix-darwin (a version similar to this PR (slightly modified and based on @LnL7 's recent updates) on this laptop and I'll be testing it on macOS without nix-darwin tonight, as well as on NixOS. I may not have a non-NixOS GNU+Linux machine online to test on until tomorrow.

It would impact this PR, if only to simplify it. The global shell configuration stuff is still new functionality, and we also still want to restore the functionality for setting variables and possibly add alias-setting support to fenv, so it is a complement to rather than replacement for these changes.

cbarrett · 2017-10-11T15:16:53Z

@therealpxc I agree that better alias support would be good. FWIW as of 2.5.0, fish seems to operate much closer to bash wrt. aliases. (e.g. typing alias will list all the aliases).

therealpxc · 2017-10-11T15:21:21Z

So I hadn't got back to this, but the way this is currently set up, does it actually translates all the system-wide aliases correctly so that fenv is unnecessary for that part? I think I mistakenly thought the aliases in the module were a Fish-specific thing.

cbarrett · 2017-10-12T00:04:46Z

Ah yeah, I forgot to bring back adding variables (thanks for the heads up @yurrriq) EDIT: Ah yeah nevermind we're using the normal setEnvironment stuff now, so that should all Just Work™

cbarrett · 2017-10-12T02:05:36Z

Looking into the test failure…

cbarrett · 2017-10-12T02:15:24Z

Looks like that test will have to be adjusted. Also pxc had an interesting idea for how to make much of this work on non-nix versions of fish. I have some local nix usage related issues / confusion I need to untangle as well. Putting [WIP] tag back on

yurrriq · 2017-10-12T03:51:08Z

I’ve amended my config so it’s moot for me, but @therealpxc and I have been working though reinstalling Nix and testing all this cleanly. I’ve borked my work machine now and it’s rebuilding the nix-darwin system manually (curl error 77 x a million) while I’m at a show... here’s hoping it’s all good when I get home / by tomorrow morning..

yurrriq · 2017-10-12T13:40:06Z

Update: Both my personal and work machines now have fully-functioning, fresh installs of Nix and nix-darwin and I can test out fish changes on Saturday.

therealpxc · 2017-10-12T16:30:22Z

(If you try to test these changes and my Nixpkgs changes at the same time, your system will be broken. Eventually we will make sure that the changes in Nixpkgs are met with whatever changes are needed in nix-darwin to make things work, but we're not letting one PR hold up the other for now.)

LnL7 · 2017-10-20T19:50:35Z

@yurrriq Have you had a chance to test this with your configuration, did anything break?

yurrriq · 2017-10-20T22:27:57Z

Last I checked it didn't break anything. FWIW I stopped using programs.fish.variables.

cbarrett · 2017-11-05T17:40:05Z

@LnL7 This should be ready to go now!

yurrriq · 2017-11-05T18:05:07Z

🎉 I’ll try this out again on my personal machine shortly.

yurrriq · 2017-11-16T07:35:55Z

This has potentially introduced an issue on my system:

set: Tried to change the read-only variable '_'

I'm trying to find the cause, but also doing a nix-channel --update now.

therealpxc · 2017-11-16T12:51:49Z

This is a cosmetic issue and a side effect of using fenv with the setEnvironment script. It's being addressed in our fenv patch in the fish-foreign-env package in Nixpkgs. I think Colin recently opened a PR for it.

…

On Thu, Nov 16, 2017, 12:35 AM Eric Bailey ***@***.***> wrote: This has potentially introduced an issue on my system: set: Tried to change the read-only variable '_' I'm trying to find the cause, but also doing a nix-channel --update now. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#50 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ADCJbolsLM1srd6R3CnNHQHQ5HzlkHl3ks5s2-XcgaJpZM4PxM1K> .

cbarrett · 2017-11-16T20:34:50Z

Correct. I've been busy with a class and have been neglecting Nix. I aim to make progress on this next week as I'm off from work.

LnL7 reviewed Oct 7, 2017

View reviewed changes

therealpxc reviewed Oct 9, 2017

View reviewed changes

therealpxc mentioned this pull request Oct 10, 2017

fish: always set up the basic Nix env during shell initialization NixOS/nixpkgs#30284

Closed

8 tasks

cbarrett force-pushed the master branch from 27b6eba to 9b289da Compare October 11, 2017 23:50

cbarrett changed the title ~~[WIP] Port over the NixOS fish module~~ Port over the NixOS fish module Oct 12, 2017

cbarrett changed the title ~~Port over the NixOS fish module~~ [WIP] Port over the NixOS fish module Oct 12, 2017

cbarrett added 3 commits October 15, 2017 23:26

Port over the NixOS fish module

a6cd3bb

nixos -> nix_darwin

1dd1f60

Set PATH since that's not being done by setEnvironment

a069111

cbarrett force-pushed the master branch from 3748aea to a069111 Compare October 16, 2017 03:42

Fix tests

ae37a8a

cbarrett changed the title ~~[WIP] Port over the NixOS fish module~~ Port over the NixOS fish module Nov 5, 2017

LnL7 approved these changes Nov 5, 2017

View reviewed changes

yurrriq approved these changes Nov 5, 2017

View reviewed changes

LnL7 merged commit 5d80f2c into LnL7:master Nov 5, 2017

LnL7 mentioned this pull request Nov 6, 2017

fish: Don't source the Nix configuration if it already has been sourced #48

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Port over the NixOS fish module #50

Port over the NixOS fish module #50

cbarrett commented Oct 7, 2017 •

edited

Loading

LnL7 left a comment

LnL7 Oct 7, 2017

therealpxc Oct 9, 2017

LnL7 Oct 9, 2017

LnL7 Oct 7, 2017

LnL7 Oct 7, 2017

yurrriq commented Oct 7, 2017

therealpxc Oct 9, 2017

cbarrett Oct 12, 2017

therealpxc Oct 9, 2017

therealpxc commented Oct 9, 2017

cbarrett commented Oct 9, 2017

therealpxc commented Oct 10, 2017 •

edited

Loading

cbarrett commented Oct 11, 2017

therealpxc commented Oct 11, 2017

cbarrett commented Oct 12, 2017 •

edited

Loading

cbarrett commented Oct 12, 2017

cbarrett commented Oct 12, 2017

yurrriq commented Oct 12, 2017 •

edited

Loading

yurrriq commented Oct 12, 2017

therealpxc commented Oct 12, 2017

LnL7 commented Oct 20, 2017

yurrriq commented Oct 20, 2017

cbarrett commented Nov 5, 2017

yurrriq commented Nov 5, 2017

yurrriq commented Nov 16, 2017

therealpxc commented Nov 16, 2017 via email

cbarrett commented Nov 16, 2017

Port over the NixOS fish module #50

Port over the NixOS fish module #50

Conversation

cbarrett commented Oct 7, 2017 • edited Loading

LnL7 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yurrriq commented Oct 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

therealpxc commented Oct 9, 2017

cbarrett commented Oct 9, 2017

therealpxc commented Oct 10, 2017 • edited Loading

cbarrett commented Oct 11, 2017

therealpxc commented Oct 11, 2017

cbarrett commented Oct 12, 2017 • edited Loading

cbarrett commented Oct 12, 2017

cbarrett commented Oct 12, 2017

yurrriq commented Oct 12, 2017 • edited Loading

yurrriq commented Oct 12, 2017

therealpxc commented Oct 12, 2017

LnL7 commented Oct 20, 2017

yurrriq commented Oct 20, 2017

cbarrett commented Nov 5, 2017

yurrriq commented Nov 5, 2017

yurrriq commented Nov 16, 2017

therealpxc commented Nov 16, 2017 via email

cbarrett commented Nov 16, 2017

cbarrett commented Oct 7, 2017 •

edited

Loading

therealpxc commented Oct 10, 2017 •

edited

Loading

cbarrett commented Oct 12, 2017 •

edited

Loading

yurrriq commented Oct 12, 2017 •

edited

Loading