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

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

Thinking about this further I disagree. Flox has a real application in companies seeking to wrap their arms around legacy technologies, and seeing as we are autogenerating template for multiple shells I don't see why we wouldn't do that for all supported shells. It seems snobbish, particularly as direnv and Python venvs both support tcsh, and at a minimum removing this would break our Python environment tests. With that I'd like to retain this logic.

Can we resolve the conversation on that basis? Thanks.

I meant both - I'm not sure we need the tcsh support in language specific flox init, and I'm not sure we need the tcsh block in manifest.toml.md (although we should definitely say we have a profile.tcsh section).

We don't really document our init hooks at all, they're just recommendations. And I'm guessing it's an okay recommendation for most people to not add a tcsh hook.

If you look ahead we may eventually support other shells like dash, ksh, etc. In short we may support an arbitrary number of shells in the future, and I think it's reasonable to draw some line (probably based on popularity) as to which shells are the defaults for automatic setup, documentation, etc so as not to clutter up the documentation, templates, etc.

I also think it wouldn't be too hard to add a --shells flag to init to be specified along with --auto-setup.

I'm not sure we need the tcsh support in language specific flox init, and I'm not sure we need the tcsh block in manifest.toml.md (although we should definitely say we have a profile.tcsh section).

If you look ahead we may eventually support other shells like dash, ksh, etc. In short we may support an arbitrary number of shells in the future, and I think it's reasonable to draw some line (probably based on popularity) as to which shells are the defaults for automatic setup, documentation, etc so as not to clutter up the documentation, templates, etc.

from a product standpoint, I agree with this. it's about prominence. bash, zsh, fish should be more prominent than the others IMO.

removed example tcsh stanza from manifest.toml(5) man page with commit a89a716

when performing a flox init there is no sample [profile.tcsh] stanza created, and nor is (or was) there for any of the other [3] shells (this was a point of confusion as some thought there would be)

when performing a flox init --auto-setup for a python project specifically, manifest stanzas are still being autogenerated for each of the [4] supported shells, and tests/python.bats has been updated to test all 4 of these

in essence this is what it means for a shell to be fully supported, and until we develop a facility for only performing --auto-setup initialization for selected shells as Zach proposed above this is in my view the most correct behaviour

And just for the avoidance of doubt I'm not aware of any other user-facing documentation or auto-generated configuration that even mentions tcsh (or fish for that matter) - if I've missed one please let me know! Thanks.

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

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

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

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

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

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

cli/flox/src/commands/activate.rs

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

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

cli/flox-rust-sdk/src/models/manifest.rs

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

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>

s/flox_save_fish_prompt/flox_saved_fish_prompt/g

Matthew spotted that we were sourcing ~/.tcshrc twice, so created the activate:validate_hook_and_dotfile_sourcing test to confirm that we are sourcing all the expected files in exactly the right order, and only once, for all shells. Then used this test to fix the tcsh order of sourcing the various scripts and hooks. Took this opportunity to make consistent the STDERR messages coming out of the various dotfiles in the fish and tcsh cases, and also addressed problems with tests interfering with each other as they stomped over $HOME/.{zshrc,bashrc,...}. My approach there was to create an "*.extra" convention by which any test could _add_ to what the default dotfiles were doing, and then remove the "*.extra" file at the conclusion of the test.

When investigating the infinite source loop potential for tcsh I discovered that these tests were not actually eval'ing properly during the tests, so I went one by one to verify the contents of the *.extra dotfiles and corrected the syntax. The tests now run reliably.

review feedback

Provide more informative message and a hint when attempting to activate pre-1.0.5 environments with either of fish or tcsh.

Per review feedback.

Re-added tests for fish and tcsh lost in the post-1299 refactor.

Needed to update references to FLOX_SHELL following commit 0a91cf3.

In creating the activate:validate_hook_and_dotfile_sourcing test in the activate.bats suite I copied the shell invocation arguments from a previous test, but as it happens invoking bash with `-i` causes the following error messages to be emitted when running in CI: bash: cannot set terminal process group (28939): Inappropriate ioctl for device bash: no job control in this shell Removing -i from the bash invocation, as well as for the other shells. The zsh example is slightly different because it's designed not to complain about such things when invoked with `-i`, so I left it in place for consistency and so that the scripts and order being sourced did not change.

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 3 times, most recently 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

mkenigs mentioned this pull request May 27, 2024

[ feature request ] Support fish shell #1173

Open

mkenigs requested changes May 27, 2024

View reviewed changes

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

pkgdb/src/buildenv/assets/activate.d/tcsh Outdated Show resolved Hide resolved

pkgdb/src/buildenv/assets/activate.d/tcsh_home/.tcshrc Show resolved Hide resolved

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

mkenigs reviewed May 30, 2024

View reviewed changes

cli/flox-rust-sdk/src/models/manifest.rs Show resolved Hide resolved

limeytexan force-pushed the fish-and-tcsh branch 2 times, most recently from 2fbe274 to 84ea49a Compare May 31, 2024 12:40

limeytexan and others added 11 commits May 31, 2024 21:20

Apply suggestions from code review

e6c2b51

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>

Apply suggestions from code review

7fe2c10

s/flox_save_fish_prompt/flox_saved_fish_prompt/g

fix: only modify prompt if $FLOX_PROMPT_ENVIRONMENTS is set

ee39869

review feedback

fix: incorporate review feedback

0094e97

Provide more informative message and a hint when attempting to activate pre-1.0.5 environments with either of fish or tcsh.

fix: adding mention of profile.{fish,tcsh} to manifest comment block

a03f6f1

Per review feedback.

fix: added python.bats tests for fish and tcsh

67c370a

Re-added tests for fish and tcsh lost in the post-1299 refactor.

fix: updated fish & tcsh logic to reflect FLOX_SHELL leak fix

2be61ee

Needed to update references to FLOX_SHELL following commit 0a91cf3.

limeytexan force-pushed the fish-and-tcsh branch from 84ea49a to 2be61ee Compare May 31, 2024 20:20

limeytexan added 2 commits May 31, 2024 21:23

chore: removing example tcsh section from manifest-toml(5) man page

a89a716

mkenigs approved these changes May 31, 2024

View reviewed changes

mkenigs added this pull request to the merge queue May 31, 2024

github-merge-queue bot removed this pull request from the merge queue due to failed status checks May 31, 2024

limeytexan added this pull request to the merge queue Jun 2, 2024

Merged via the queue into main with commit ace82ca Jun 2, 2024
16 checks passed

limeytexan deleted the fish-and-tcsh branch June 2, 2024 18:07

zmitchell mentioned this pull request Jun 4, 2024

test: copy over init tests for catalog #1528

Open

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

limeytexan May 30, 2024

mkenigs May 30, 2024

zmitchell May 31, 2024

ghudgins May 31, 2024

limeytexan May 31, 2024 •

edited

ghudgins May 31, 2024

zmitchell commented May 25, 2024

add support for fish and tcsh shells #1347

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

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 May 31, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zmitchell commented May 25, 2024

limeytexan commented Apr 20, 2024 •

edited

limeytexan May 26, 2024 •

edited

limeytexan May 31, 2024 •

edited