Skip to content

Conversation

ipince
Copy link
Contributor

@ipince ipince commented Feb 22, 2023

Summary

Adds support for fish shell.

When running devbox shell:
We write the post-initialization steps into a "shellrc" file (see shellrc_fish.tmpl). I attempted to reuse the existing shellrc.tmpl, but the differences in syntax between fish and other shells were large enough that I had to separate them. Next, we invoke fish -C <shellrc>. This will make fish use the user's existing config first, and then run whatever is in <shellrc>, which is similar to what we do for non-fish shells.

When running devbox run:

  • If unified env is ON, then we invoke sh -c <script> just like we do for every other shell.
  • If unified env is OFF, then we'll invoke fish -C <shellrc> -c <script>. However, note that Enable unified env by default #650 already enables unified env by default, so this branch is mainly here just in case something goes wrong with unified env and we need to revert.

Caveats:

  • We don't check any of the syntax in devbox.json's init hook or scripts. If an init hook uses fish-specific syntax, then it will work for users using fish, but it won't for users using a different shell. Similarly for scripts. Conversely, if an init hook uses POSIX syntax that fish doesn't support (e.g. FOO=bar), then the init hook will fail for fish users.
  • Any plugins that use init hooks should write them in such a way that they work for both fish and non-fish. So far, it seems only pip and ruby use init hooks, and they're written in a compatible way (AFAIK).

How was it tested?

I tested that both devbox shell and devbox run worked with and without unified env. I verified that PATH was being set, user's config is being run (for shell only), init hooks are being run, history file is set (shell only), prompt is set (shell only), and scripts are written and run.
To test with fish, it's as easy as invoking devbox as such:

SHELL=fish devbox shell

@ipince ipince requested review from gcurtis, loreto and Lagoja February 22, 2023 19:27
@ipince
Copy link
Contributor Author

ipince commented Feb 22, 2023

Fixes #652

@Lagoja
Copy link
Contributor

Lagoja commented Feb 23, 2023

Looks like this works as expected from my Fish testing, so I'll approve it. Someone else should do code review though

Copy link
Collaborator

@gcurtis gcurtis left a comment

Choose a reason for hiding this comment

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

Nice! LGTM

}

func fishConfig() string {
xdg := os.Getenv("XDG_CONFIG_HOME")
Copy link
Collaborator

Choose a reason for hiding this comment

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

We also have ./internal/xdg.ConfigDir now.

@ipince ipince merged commit 2f25b12 into main Feb 23, 2023
@ipince ipince deleted the rodrigo/fish-support branch February 23, 2023 17:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants