Native fish environment with babelfish #270

kevingriffin · 2021-01-09T05:03:34Z

This is an alternative approach to #266

Adds the babelfish package for translating the environment to fish at build time, eliminating the need for and performance cost of fenv.

I'm not sure if we want to vendor the package, get it added upsteam in nixpkgs, or some other approach. Ideally nixOS users would receive a similar change to their module as well.

Fixes #269

toonn · 2021-01-09T20:43:03Z

This does seem to work fine for me. Doesn't really affect loading time of a shell started from a shell but spawning a new terminal has gone for ~8s to under 4s.

Note that I don't use POSIX variable substitutions in my environment so I can't vouch for that working.

kevingriffin · 2021-01-10T14:06:14Z

Thanks for having a look! I prefer this approach and opened PRs on nixpkgs to add the package and use it in the nixOS module.

bouk

Thank you for getting started on this, I already have something like this in my own nix config:

https://github.com/bouk/b/blob/5cfc6ec6a46d15fffef9c92788b719d07a1a950e/darwin.nix#L80-L117

For me it was a huge speed boost, shaving 3-4 seconds (!) off every new shell invocation

bouk · 2021-01-10T17:04:33Z

modules/programs/fish.nix

-        set fish_function_path ${pkgs.fish-foreign-env}/share/fish-foreign-env/functions $fish_function_path
-        fenv source /etc/fish/foreign-env/interactiveShellInit > /dev/null
-        set -e fish_function_path[1]
-


You're removing a bunch of the shell inits here, which will break gpg-agent for example.

I think we should translate all of the init files and save them as environment.etc."fish/interactiveShellInit.fish" etc. Then we also don't need system.build.setEnvironmentFish

@bouk I've tried implementing this approach. If you could have a look and give feedback, I'll port the same change to the nixOS module.

kevingriffin · 2021-01-11T07:30:06Z

I've made changes here and on the nixOS module to make this change optional, configured on the fish module. I also committed a change to the namespace of the foreign-env package to resolve the original issue for users who don't use the new babelfish-based generation option.

bouk · 2021-01-11T10:10:59Z

modules/programs/fish.nix

+        ${babelfish}/bin/babelfish < ${config.system.build.setEnvironment} > $out;
+      '';
+
+       etc."fish/shellInit".source = pkgs.runCommand "shellInit" {


fish files always have .fish at the end, so I think you should add that here too.

hauleth · 2021-01-16T00:06:07Z

modules/programs/fish.nix

+  else
+    ''
+      set fish_function_path ${pkgs.fishPlugins.foreign-env}/share/fish-foreign-env/functions $fish_function_path
+      fenv source ${path} > /dev/null


Do we still need fenv there as we are using babelfish?

We don't. In the version I've done for nixOS, this requirement is removed. I'll go ahead and port it back to nix-darwin in this branch.

LnL7

There are a few extra things to consider here compared to the NixOS module since this needs to be compatible with both 20.09 and unstable.

LnL7 · 2021-01-16T11:11:15Z

modules/programs/fish.nix

@@ -12,6 +12,23 @@ let
    mapAttrsFlatten (k: v: "alias ${k} '${v}'") cfg.shellAliases
  );

+  babelfish = pkgs.callPackage ../../pkgs/shells/fish/plugins/babelfish.nix { };


I don't really want to keep package definitions in this repo.

Since this isn't guaranteed to be available in nixpkgs (eg. 20.09) the best solution is to add a package option without a default, that way it's up to the user to either introduce the version they want into their configuration or if nixpkgs is recent enough just use pkgs.babelfish. The later can then also the default once the oldest supported release has babelfish available.

I agree. It was convenient to spike it, but I wouldn't want to vendor an old package in nix-darwin. I'll add a package option.

LnL7 · 2021-01-16T11:14:09Z

modules/programs/fish.nix

+    "source ${path}"
+  else
+    ''
+      set fish_function_path ${pkgs.fishPlugins.foreign-env}/share/fish-foreign-env/functions $fish_function_path


Similar to the babelfish package, this should still handle the case where fishPlugins isn't available in nixpkgs yet. That's what is causing the tests to fail. eg.

let fenv = pkgs.fish-foreign-env or pkgs.fishPlugins.foreign-env; in # ...

Ideally an alias would be added in nixpkgs however, just renaming stuff isn't really something projects outside of the repo should have deal with without some compatibility window.

That's a very clean way to do it. I'll incorporate that into this branch, along with the other changes (like selecting which method to use—fenv or babelfish—) from the nixOS module and push up a new version.

Configuration may be ran through fenv at shell start time (as previously) or translated to fish at build time with a specified babelfish package.

kevingriffin · 2021-01-17T01:32:32Z

@LnL7 Thanks for the review! I believe what I just pushed up addresses your comments. Let me know if there's anything else to improve.

kevingriffin · 2021-01-23T01:25:40Z

@LnL7 The nixOS module was merged with no further changes. Is there anything else to do to make this suitable for nix-darwin?

LnL7

With the extra compatibility changes this looks good to me. I'm not a fish user but people with more relevant knowledge should have already reviewed this on the NixOS side.

teehemkay · 2021-01-29T16:55:31Z

There's an issue where fish-foreign-env has now been aliased to throw "fish-foreign-env has been replaced with fishPlugins.foreign-env in Nixpkgs.

If works fine if the user chooses to enable babelfish.
But it results in an error if fish is enabled but babelfish is not used and Nixpkgs has already been upgraded to fishPlugins.foreign-env which unfortunately is the default configuration...

The test to bind fenv in the let expression should probably be inverted.
Maybe something like:

 fenv = if builtins.hasAttr "fishPlugins" pkgs then pkgs.fishPlugins.foreign-env else pkgs.fish-foreign-env;

toonn · 2021-01-29T17:09:33Z

modules/programs/fish.nix

+
+  envInteractiveShellInit = pkgs.writeText "interactiveShellInit" cfge.interactiveShellInit;
+
+  fenv = pkgs.fish-foreign-env or pkgs.fishPlugins.foreign-env;


I ran into the same problem because of the redirect at pkgs.fish-foreign-env, looks like flipping around the packages would indeed fix it.

Suggested change

fenv = pkgs.fish-foreign-env or pkgs.fishPlugins.foreign-env;

fenv = pkgs.fishPlugins.foreign-env or pkgs.fish-foreign-env;

teehemkay · 2021-01-29T17:13:44Z

Yep. I was beeing extra cautious (check before jumping) hence the if expression.

I've updated my previous comment to clarify when the error occurs.

kevingriffin · 2021-01-30T02:29:40Z

Thank you @teehemkay and @toonn for the testing. I opened a quick PR to integrate this change:

#283

kevingriffin mentioned this pull request Jan 9, 2021

Recent changes in fish-foreign-env (Nixpkgs) break /etc/fish/nixos-env-preinit.fish #269

Closed

kevingriffin force-pushed the fish-environment-babelfish branch from 87157b7 to dcb8389 Compare January 9, 2021 05:07

kevingriffin mentioned this pull request Jan 10, 2021

fish: native environment NixOS/nixpkgs#108947

Merged

10 tasks

kevingriffin mentioned this pull request Jan 10, 2021

fish: generate environment natively #266

Closed

bouk suggested changes Jan 10, 2021

View reviewed changes

kevingriffin force-pushed the fish-environment-babelfish branch from 61a859a to 3e7ba97 Compare January 11, 2021 06:44

bouk reviewed Jan 11, 2021

View reviewed changes

hauleth reviewed Jan 16, 2021

View reviewed changes

LnL7 requested changes Jan 16, 2021

View reviewed changes

fish: add babelfish translation option

867ef96

Configuration may be ran through fenv at shell start time (as previously) or translated to fish at build time with a specified babelfish package.

kevingriffin force-pushed the fish-environment-babelfish branch from c0e2b01 to 867ef96 Compare January 17, 2021 01:31

happysalada mentioned this pull request Jan 28, 2021

update fish-foreign-env package #280

Closed

LnL7 approved these changes Jan 28, 2021

View reviewed changes

LnL7 merged commit 32d49bc into LnL7:master Jan 28, 2021

toonn reviewed Jan 29, 2021

View reviewed changes

malob mentioned this pull request Jan 29, 2021

programs.nix-index: add common-not-found support for Fish #272

Closed

kevingriffin mentioned this pull request Jan 30, 2021

fish: fix fenv reference on 20.09 #283

Merged

r-burns mentioned this pull request Feb 5, 2021

fish: fix fenv function path #288

Merged

emilazy mentioned this pull request May 23, 2023

fish: use babelfish for hm-session-vars.sh nix-community/home-manager#4012

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Native fish environment with babelfish #270

Native fish environment with babelfish #270

kevingriffin commented Jan 9, 2021

toonn commented Jan 9, 2021

kevingriffin commented Jan 10, 2021

bouk left a comment

bouk Jan 10, 2021

kevingriffin Jan 11, 2021

kevingriffin commented Jan 11, 2021

bouk Jan 11, 2021

hauleth Jan 16, 2021

kevingriffin Jan 16, 2021

LnL7 left a comment

LnL7 Jan 16, 2021

kevingriffin Jan 17, 2021

LnL7 Jan 16, 2021

kevingriffin Jan 17, 2021

kevingriffin commented Jan 17, 2021

kevingriffin commented Jan 23, 2021

LnL7 left a comment

teehemkay commented Jan 29, 2021 •

edited

Loading

toonn Jan 29, 2021

teehemkay commented Jan 29, 2021 •

edited

Loading

kevingriffin commented Jan 30, 2021


		envInteractiveShellInit = pkgs.writeText "interactiveShellInit" cfge.interactiveShellInit;

		fenv = pkgs.fish-foreign-env or pkgs.fishPlugins.foreign-env;

	fenv = pkgs.fish-foreign-env or pkgs.fishPlugins.foreign-env;
	fenv = pkgs.fishPlugins.foreign-env or pkgs.fish-foreign-env;

Native fish environment with babelfish #270

Native fish environment with babelfish #270

Conversation

kevingriffin commented Jan 9, 2021

toonn commented Jan 9, 2021

kevingriffin commented Jan 10, 2021

bouk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevingriffin commented Jan 11, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

kevingriffin commented Jan 17, 2021

kevingriffin commented Jan 23, 2021

LnL7 left a comment

Choose a reason for hiding this comment

teehemkay commented Jan 29, 2021 • edited Loading

Choose a reason for hiding this comment

teehemkay commented Jan 29, 2021 • edited Loading

kevingriffin commented Jan 30, 2021

teehemkay commented Jan 29, 2021 •

edited

Loading

teehemkay commented Jan 29, 2021 •

edited

Loading