Skip to content
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.

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

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

Closed
wants to merge 9 commits into from

Conversation

nonrational
Copy link
Member

@nonrational nonrational commented Feb 5, 2022

Related to:

Changes

  • resolve symlinks (e.g. ~/.puma-dev/myapp) to absolute appDir to make relative Gemfile paths work
  • load full puma env before checking $SHELL and...
  • exec $SHELL to exec puma instead of always using bash
  • avoid passing arguments to puma (e.g. -C, -t, -w) if ENV vars aren't set
  • remove neko dependency
  • add zsh to build matrix

Binaries

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

@nonrational nonrational marked this pull request as draft February 8, 2022 19:51
@cjlarose
Copy link
Member

cjlarose commented Feb 10, 2022

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:

  • In Go, get the current environment using os.Environ()
    • Append THREADS, WORKERS, and CONFIG values to the environment variable list
  • Start a new login shell in a subprocess using $SHELL if set, /bin/bash otherwise. Pass in the computed environment.
    • In that shell, start a (non-login) bash shell
      • cd to the path of the app (not the destPath), basically just the symlink in ~/.puma-dev
      • source env files (~/.powconfig, .env, .powrc, .powenv, .pumaenv)
      • exec puma

Problems with the current approach

The proposed solution is this:

  • Use os.Environ to get environment variables for the current puma-dev process. Try to read all of the env files in Go (and merge in the values into the current environment). This is done, in part, to determine the correct $SHELL to use at the top level. If any of the env files are more complicated than just KEY=value, then this approach is aborted.
  • Start a new login shell using the $SHELL determined by reading all of environment variables (falling back to /bin/bash if unset)
    • In that shell, start a new non-login shell using the same $SHELL
      • cd, source env files, and exec puma.

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
Copy link
Member Author

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.

  • Resolve symlink'd application dir to allow relative paths (e.g. shared engines) to function as expected.
  • Add support for ~/.pumaconfig xor ~/.pumaenv with the eventual goal of deprecating config files referencing "pow"
  • Look for opportunities to add more logging when apps come up and read their envs
  • Re-work puma-dev app environment loading
    1. Source puma env files (e.g. .env, .pumaenv, ...) via shell.
      • Probably bash -l. Though, if i run zsh will we still pick up the right env e.g. asdf, rbenv, etc.? If we can't get the exact env in Go that we'll have when puma's running, its prob not worth it and we should just do everything in shell.
    2. Read the resulting env back into Go for shell selection and puma CLI args building
    3. Pass that env into the non-login shell process that boots puma.

@cjlarose
Copy link
Member

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants