add support for fish and tcsh shells #1347

limeytexan · 2024-04-20T16:45:22Z

Proposed Changes

This PR adds support for the fish and tcsh shells based on the refactoring of flox activate performed in #1299.

Also removed posixIfThen and posixSetEnv C++ macros no longer required since merging PR#1299.

Release Notes

Added support for fish and tcsh shells.

cli/flox/doc/manifest.toml.md

The perils of creating our own quoting function is that we're destined to repeat lessons of the past, but of course the advantage is that we have more precise control of the outcome. This patch adds logic to similarly escape the backtick (`) character. (I rather suspect our next step will be to escape the single quote ('), followed by conditional logic to avoid escaping when within them.)

Following PR#1299 it was fairly straightforward to add support for the fish and tcsh shells. This patch adds this support along with tests that mirror those that already exist for bash and zsh. Also removed `posixIfThen` and `posixSetEnv` C++ macros no longer required since merging PR#1299.

mkenigs

I only made it through the Rust - I'll pick up with the pkgdb changes next week (that's where all the real logic is 😆 )

cli/flox/src/commands/init/mod.rs

cli/tests/activate.bats

mkenigs · 2024-05-25T01:12:35Z

cli/flox/src/commands/init/python.rs

+                indoc! {r#"
+                echo "Activating poetry virtual environment" >&2
+                source "$(poetry env info --path)/bin/activate.csh""#}
+                .to_string(),


question blocking: how prominent do we want tcsh to be in docs and init hooks? I'm not really sure how much it's used, so I'm not sure if most people will see it as boilerplate or actually helpful

I know at least two firms in which it's still used extensively, but regardless we should maintain a low bar for the addition of shell support (and it should be easier and better factored to involve modifications to fewer than 30 files in future!).

As for the prominence of its mention in the template, that's somewhat of an implementation detail; direnv supports 9 shells, and I agree at some point that we'll want to change the manifest template to be less verbose once there are that many to be listed.

But is there a reason you made the comment here in the python.rs file? This is one place where we definitely need to explicitly emit code for each of the supported shells - I can see your question pertaining to the template manifest.toml but not here?

Even in init hooks, we could just add hooks for the most popular shells - with 3 examples (bash, zsh, and fish) I don't think it would be too difficult for a tcsh user to figure out what to add in a tcsh hook.

@ghudgins @jennymahmoudi any thoughts?

agree, just fish is good

zmitchell · 2024-05-25T01:26:00Z

I'm about half-way done with my review, I'll probably try to finish on Monday

@mkenigs

Thanks @mkenigs for the eagle eye. Clearly did that in a batch of bulk changes and just missed it ... curious why that wasn't caught in tests but happy to move on ... Co-authored-by: Matthew Kenigsberg <matthew@floxdev.com>

mkenigs · 2024-05-27T22:34:02Z

pkgdb/src/buildenv/assets/activate.d/set-prompt.tcsh

+    set prompt = "$_flox $FLOX_SAVE_TCSH_PROMPT"
+endif
+
+unset _flox


suggestion blocking: when "${FLOX_PROMPT_ENVIRONMENTS:-}" == "", the prompt shouldn't be changed at all

mkenigs · 2024-05-27T22:37:14Z

pkgdb/src/buildenv/assets/activate.d/tcsh

+# these files ourselves prior to setting up the environment.
+foreach f ( /etc/csh.login /etc/csh.cshrc ~/.cshrc ~/.tcshrc )
+  if ( -f $f ) then
+    source $f


question blocking: could we hit the same infinite looping bug as with .bashrc?

mkenigs · 2024-05-27T22:40:48Z

pkgdb/src/buildenv/assets/activate.d/tcsh_home/.tcshrc

+
+# Bring in the Nix and Flox environment customizations.
+if ( $?FLOX_TCSH_INIT_SCRIPT ) then
+    source "$FLOX_TCSH_INIT_SCRIPT"


question blocking: looks like above you source e.g. $HOME/.cshrc, but then doesn't FLOX_TCSH_INIT_SCRIPT source ~/.cshrc again? Not sure if I'm reading that right

That's what it looks like to me

cli/flox/src/commands/init/mod.rs

zmitchell · 2024-05-25T01:09:27Z

cli/flox/src/commands/activate.rs

@@ -365,6 +365,12 @@ impl Activate {
                    .arg("--rcfile")
                    .arg(activation_path.join("activate").join("bash"));
            },
+            Shell::Fish(_) => {


Suggestion: it's probably worth suggesting to the user that they rebuild the environment to gain support for the new shells. If this is a remote environment that belongs to someone else then it's out of their hands, but otherwise they may not understand why we say we support fish, etc and then get this error message.

I expect there will be approximately zero people wanting to use fish with environments rendered before version 1.0.5, but I see your point. What if we update the error message to read "fish not supported with environments rendered before version 1.0.5; please update environment and try again" - does that work for you?

zmitchell · 2024-05-28T19:37:19Z

pkgdb/src/buildenv/assets/activate.d/tcsh_home/.tcshrc

+
+# Bring in the Nix and Flox environment customizations.
+if ( $?FLOX_TCSH_INIT_SCRIPT ) then
+    source "$FLOX_TCSH_INIT_SCRIPT"


That's what it looks like to me

pkgdb/src/buildenv/assets/activate.d/set-prompt.fish

s/flox_save_fish_prompt/flox_saved_fish_prompt/g

limeytexan force-pushed the fish-and-tcsh branch 2 times, most recently from d05824d to fa5fd87 Compare April 23, 2024 10:48

limeytexan force-pushed the issue-1187 branch 2 times, most recently from 3c3e633 to f9caf58 Compare April 23, 2024 11:51

limeytexan force-pushed the fish-and-tcsh branch 3 times, most recently from 1071e76 to 65e11a2 Compare April 23, 2024 16:48

limeytexan force-pushed the issue-1187 branch 4 times, most recently from 779a4b9 to 67a0e41 Compare April 30, 2024 17:18

Base automatically changed from issue-1187 to main April 30, 2024 17:20

limeytexan force-pushed the fish-and-tcsh branch 3 times, most recently from 3118022 to d4d4a0b Compare May 22, 2024 17:43

limeytexan marked this pull request as ready for review May 22, 2024 17:55

limeytexan requested review from mkenigs, zmitchell and ysndr May 22, 2024 17:58

mkenigs requested a review from dcarley May 22, 2024 18:55

limeytexan commented May 23, 2024

View reviewed changes

cli/flox/doc/manifest.toml.md Outdated Show resolved Hide resolved

limeytexan commented May 23, 2024

View reviewed changes

cli/flox/doc/manifest.toml.md Outdated Show resolved Hide resolved

limeytexan commented May 23, 2024

View reviewed changes

cli/flox/doc/manifest.toml.md Outdated Show resolved Hide resolved

limeytexan force-pushed the fish-and-tcsh branch 2 times, most recently from 7c03ba1 to 44b7b6d Compare May 23, 2024 12:34

limeytexan added 2 commits May 23, 2024 12:52

limeytexan force-pushed the fish-and-tcsh branch from 44b7b6d to b07aa0e Compare May 23, 2024 12:53

limeytexan mentioned this pull request May 24, 2024

Detect hook.script and print a deprecation message #1135

Open

mkenigs reviewed May 25, 2024

View reviewed changes

Apply suggestions from code review

4ee89a1

Thanks @mkenigs for the eagle eye. Clearly did that in a batch of bulk changes and just missed it ... curious why that wasn't caught in tests but happy to move on ... Co-authored-by: Matthew Kenigsberg <matthew@floxdev.com>

mkenigs mentioned this pull request May 27, 2024

[ feature request ] Support fish shell #1173

Open

mkenigs requested changes May 27, 2024

View reviewed changes

zmitchell reviewed May 28, 2024

View reviewed changes

limeytexan commented May 29, 2024

View reviewed changes

pkgdb/src/buildenv/assets/activate.d/set-prompt.fish Outdated Show resolved Hide resolved

limeytexan commented May 29, 2024

View reviewed changes

pkgdb/src/buildenv/assets/activate.d/set-prompt.fish Outdated Show resolved Hide resolved

Apply suggestions from code review

b1e456e

s/flox_save_fish_prompt/flox_saved_fish_prompt/g

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add support for fish and tcsh shells #1347

add support for fish and tcsh shells #1347

limeytexan commented Apr 20, 2024 •

edited

mkenigs left a comment

mkenigs May 25, 2024

limeytexan May 26, 2024 •

edited

mkenigs May 27, 2024

ghudgins May 28, 2024

zmitchell commented May 25, 2024

mkenigs May 27, 2024

mkenigs May 27, 2024

mkenigs May 27, 2024

zmitchell May 28, 2024

zmitchell May 25, 2024

limeytexan May 29, 2024

zmitchell May 28, 2024

add support for fish and tcsh shells #1347

Are you sure you want to change the base?

add support for fish and tcsh shells #1347

Conversation

limeytexan commented Apr 20, 2024 • edited

Proposed Changes

Release Notes

mkenigs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

limeytexan May 26, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zmitchell commented May 25, 2024

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

Choose a reason for hiding this comment

limeytexan commented Apr 20, 2024 •

edited

limeytexan May 26, 2024 •

edited