Skip to content

[direnv] copy bash, fish, zsh shell code from direnv #1156

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 2 commits into from
Jun 26, 2023
Merged

[direnv] copy bash, fish, zsh shell code from direnv #1156

merged 2 commits into from
Jun 26, 2023

Conversation

savil
Copy link
Collaborator

@savil savil commented Jun 14, 2023 •
edited
Loading

Summary

Overall Motivation
We've been having issues with the bin-wrappers, and want to explore an alternate approach. Bin-wrappers ensure that the shell environment is completely updated prior to invoking the binary, via running devbox shellenv if needed.

This alternate approach takes inspiration from direnv. It will seek to register a prompt hook in the shell. The hook will execute a shell function that will export the shell environment to update it, prior to displaying the shell prompt.

To punt on how to transition existing devbox commands, we're introducing devbox hook and devbox export as distinct commands, analogous to direnv hook and direnv export.

This PR's motivation

As part of implementing this, we'd like to make use of direnv's battle-tested shell code where we can. Direnv has a nice shell abstraction for handling the hook and environment-variables. We could adopt this abstraction to better support shell code we generate beyond this immediate use-case of prompt-hooks.

So, we:

  1. Copy bash, fish, zsh Shell code from direnv. Thank you 🙏
  2. Add ksh, posix, unknownSh structs. I filled in the struct functions as placeholders for now.

I call the package shenv since it handles the "shell environment". I stayed away from "shellenv" to avoid confusion with our existing "shellenv" command's operation.

How was it tested?

Have not tested. Subsequent PR will test and correct as needed.

@savil Graphite App
Copy link
Collaborator Author

savil commented Jun 14, 2023 •
edited
Loading

Current dependencies on/for this PR:

This comment was auto-generated by Graphite.

@savil savil mentioned this pull request Jun 16, 2023
Merged
@savil savil force-pushed the savil/direnv-inspired-shellenv branch 2 times, most recently from e24ad15 to 61a3bfd Compare June 16, 2023 23:45
@savil savil requested review from gcurtis and loreto June 17, 2023 00:52
@savil savil marked this pull request as ready for review June 17, 2023 00:52
loreto
loreto approved these changes Jun 20, 2023
View reviewed changes
Copy link
Contributor

@loreto loreto left a comment

Choose a reason for hiding this comment

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

LGTM although I am having trouble figuring out which files came from direnv, and which ones did you add.

Is ksh the only one you added? Why not bring over tcsh, elvish and gha as well?

As mentioned during standup ... direnv must support ksh, right? Is it possible we just need to map ksh to one of the pre-existing ones? Maybe the posix one? (Uncyclopedia says KornShell complies with POSIX.2)

Are you sure you don't want to also copy the env related files direnv has? Or at least the env struct along with GetEnv, Copy, ToGoEnv, ToShell, Fetch

I think I would also copy shell.go along with the DetectShell, it handles figuring out the shell from the path if necessary: DetectShell("-/usr/local/bin/bash")

@savil savil force-pushed the savil/direnv-inspired-shellenv branch from 61a3bfd to 751628e Compare June 26, 2023 16:03
@savil
Copy link
Collaborator Author

savil commented Jun 26, 2023 •
edited
Loading

LGTM although I am having trouble figuring out which files came from direnv, and which ones did you add.

The PR summary mentions:

So, we:

  1. Copy bash, fish, zsh Shell code from direnv. Thank you 🙏
  2. Add ksh, posix, unknownSh structs. I filled in the struct functions as placeholders for now.

Is ksh the only one you added?

see above

Why not bring over tcsh, elvish and gha as well?

Our current devbox shell doesn't support those. So, I was limiting scope for now. We can add them later if we need.

As mentioned during standup ... direnv must support ksh, right? Is it possible we just need to map ksh to one of the pre-existing ones? Maybe the posix one? (Uncyclopedia says KornShell complies with POSIX.2)

Yeah, I didn't get a chance to investigate that. Will do, and if needed will drop the ksh shell from this place.

Are you sure you don't want to also copy the env related files direnv has? Or at least the env struct along with GetEnv, Copy, ToGoEnv, ToShell, Fetch

For now, I was focussed purely on getting the hook and export commands working. In a follow up PR, I want to investigate whether we can do the "environment diffing" that direnv does, and will likely incorporate direnv's Env utilities once I know how we can use them.

I think I would also copy shell.go along with the DetectShell, it handles figuring out the shell from the path if necessary: DetectShell("-/usr/local/bin/bash")

Ah, I do this: the shenv.go code is copied from the shell.go code. I did modify DetectShell just a bit to expunge the figuring out shell from path, since I didn't see how it would be used in our integration. As needed, I can add it back (likely by using devbox hook $SHELL)

@savil savil merged commit fe1db1f into main Jun 26, 2023
@savil savil deleted the savil/direnv-inspired-shellenv branch June 26, 2023 18:14
savil added a commit that referenced this pull request Jun 26, 2023
@savil
[RFC][direnv-inspired] introduce export and hook commands, use in dev…
9e3638f 
…box shell, global and direnv (#1172)

## Summary

See #1156 for overall motivation.



**In this PR:**

1. Turn off adding bin-wrappers path to $PATH from `devbox shellenv`.
2. We add commands `devbox hook` and `devbox export`, analogous to
`direnv hook` and `direnv export`.
   - `export` is an alias of shellenv, and is hidden
- `hook` registers a shell prompt hook that wil call `eval $(devbox
export)`
3. During `devbox shell`, we add `eval $(devbox hook)` to the generated
shellrc file.
4. During `direnv`, the code is essentially unchanged except
calling`devbox export` instead of `devbox shellenv`.
5. For global, the users are currently expected to add `eval $(devbox
global hook <shell>)`

**RFC proposal to roll out:**
1. I don't think we need `devbox export`. We can continue with `devbox
shellenv`.
2. We can remove `devbox hook` entirely: in the generated shellrc, we
can inline the code of the hook that calls `devbox export`.
3. We can introduce `devbox global activate` to have the functionality
that `devbox global hook` has in this PR.
- `activate` is a generic word, enabling us to change its functionality
under-the-hood. It would save us the dilemma we have with this PR about
how to make users change their shellrc code from `devbox global
shellenv` to `devbox global hook`.
- We can make `devbox global shellenv` an alias of `devbox global
activate`. This would save users from needing to update their personal
shellrc files.

## How was it tested?

Added a feature flag: `DEVBOX_FEATURE_PROMPT_HOOK=1`.

Did some basic testing:
- project:
- `devbox shell` and then edited `devbox.json` to add a new package, and
saw the new package being installed prior to next prompt being
displayed.
- `devbox run -- iex -S mix` worked as expected, since we can turn off
the bin-wrappers.
- global: 
   - added `devbox global hook | source` to my fish shell.
   - `devbox global ls` worked.
   - `devbox global add <package>` worked as before.
   
**Note:** this PR adds the basic functionality, but the feature is not
yet robust. For a full list of pending scenarios that must work, please
refer to the (internal) notion doc.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@loreto loreto loreto approved these changes

@gcurtis gcurtis Awaiting requested review from gcurtis

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@savil @loreto