puma / puma-dev

A tool to manage rack apps in development with puma
BSD 3-Clause "New" or "Revised" License
1.74k stars 107 forks source link

Refactor env loading, appDir resolution, and exec shell #302

Closed nonrational closed 2 years ago

nonrational commented 2 years ago

Related to:

Changes

Binaries

https://github.com/puma/puma-dev/actions/runs/1798040084

cjlarose commented 2 years ago

I'm going to quickly restate the problem this PR is trying to solve to make sure I understand it correctly.

The current technique (master) for loading environment variables is this:

Problems with the current approach

The proposed solution is this:

This solution successfully gives folks the ability to change the shell that puma-dev uses when starting application subprocesses, so it definitely provides an option to people that was not previously available. It has two potential drawbacks, IMO

  1. If any of the env files are more complicated than just KEY=value declarations, the $SHELL will not be configured as the user expects.
  2. The env files will now be sourced using the specified $SHELL. I think this can cause problems for teams of devs that use different shells, since they’ll likely share the env files for their projects. I think it is simpler to standardize that all envfiles will be interpreted in bash, regardless of the user’s preferred shell. This is what the current approach does on master and is actually identical to tools like direnv.

I think there are two potential paths forward to consider, both of them just alternative ways for the user to specify the $SHELL for the top-level login shell.

  1. Instead of trying to build the env map in Go, just let bash do this work: start a new short-lived bash subprocess. In that process, just source all of the env files and print out the value of $SHELL. This way all env files are interpreted using bash (so they can do more complicated scripting stuff). Collect the output of this process in Go to determine the $SHELL to use for the actual application subprocess at the top level (in the login shell). In that subprocess, just do what puma-dev does currently (start a new bash shell, source the env files, and exec puma).
  2. Add a new puma-dev -install option to allow users to specify the shell they want puma to use when starting application subprocesses. We can pass the selected shell as a CLI argument to the puma-dev process when launched by the init process.

The other thing that's missing is probably just better documentation and better tools for debugging this kind of stuff. It's not super obvious what shells puma-dev is using, which ones are login shells, and consequently, which files need to be modified to make puma-dev happy (.bashrc, .bash_profile, .zshrc, etc).

nonrational commented 2 years ago

Instead of trying to build the env map in Go, just let bash do this work.

I think this has a lot of benefits. My original intent was to reduce the amount of shell code significantly, but you've convinced me that there's more value in letting shell handle env grooming, rather than restricting ourselves to dotenv-style configs.

This approach also preserves my original intent, namely to get us back into Go after reading the env, so we can build the puma CLI string in Go, rather than having to do it in pure shell, which is really ugly.

I do worry that now macOS has made zsh the default, it's confusing that we'll still force someone to run bash under the hood. And, as you say, it becomes doubly confusing to trace where the env actually comes from.

[What's] missing is probably just better documentation and better tools for debugging this kind of stuff

You're totally right. It's incredibly hard to figure out how all of these pieces interact. I'm hopeful that splitting out some of this behavior will make it easier to follow in the code, but will also look for opportunities for logging in the short-term.

So, I think the way forward involves a few changes and, potentially, splitting this PR in 2-3 pieces to keep things straight.

cjlarose commented 2 years ago

I do worry that now macOS has made zsh the default, it's confusing that we'll still force someone to run bash under the hood. And, as you say, it becomes doubly confusing to trace where the env actually comes from.

Yeah, I acknowledge that using bash for this case can be confusing, but I think as long as it is well-documented, it's worth it just to standardize the shell among devs. Even though bash isn't my favorite interactive shell, it's clear that it's ubiquitous for scripting/env tasks.

So, I think the way forward involves a few changes and, potentially, splitting this PR in 2-3 pieces to keep things straight.

I love this approach of breaking up the work into a few smaller PRs. Feel free to tag me for review for any. The list of steps you laid out seems great!