Details
- Reviewers
• abosh varun atul - Group Reviewers
Restricted Owners Package (Owns No Changed Paths) - Commits
- rCOMM1991d5d3b4ce: [Nix] Auto start redis on nix develop
# If you have redis setup through homebrew, then run: brew services stop redis nix develop
Should see:
View Redis Logs: tail -f $HOME/Library/Caches/Redis/logs Kill Redis server: pkill redis
inspect logs:
tail -f $HOME/Library/Caches/Redis/logs
Diff Detail
- Repository
- rCOMM Comm
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
Sweet, followed the Test Plan and it looks like it worked as expected!
atul@atuls-MacBook-Pro comm % nix develop Starting Redis View Redis Logs: tail -f /Users/atul/Library/Caches/Redis/logs Kill Redis server: pkill redis Existing MariaDB instance found outside of nix environment Please stop existing services and attempt 'nix develop' again Welcome to Comm dev environment! :)
That said, I'm not that familiar with nix yet so I'll resign as reviewer and let eg @varun take a closer look at that stuff.
| nix/redis-up-mac.nix | ||
|---|---|---|
| 12–16 ↗ | (On Diff #15770) | Have a hunch that folks on the team are going to ask if we can separate this into separate .sh file or somehow run shellcheck on the contents... might be worth leaving a comment here to preempt that? |
| 25–42 ↗ | (On Diff #15770) | Have a hunch that folks on the team are going to ask if we can separate this into separate .sh file or somehow run shellcheck on the contents... might be worth leaving a comment here to preempt that? |
Have a hunch that folks on the team are going to ask if we can separate this into separate .sh file or somehow run shellcheck on the contents... might be worth leaving a comment here to preempt that?
This seems to be a problem with a lot of nix work. I really wish there was a way we could separate all shell scripting into a sh file or somehow verify/lint it beforehand, no matter how small the code is. It makes it easier to review and adds another layer of testing.
| nix/redis-up-mac.nix | ||
|---|---|---|
| 15 ↗ | (On Diff #15770) | Double quote. A bit confused why writeShellApplication didn't pick up on this if it runs it through ShellCheck. {F144261} |
| 15–16 ↗ | (On Diff #15770) | This can probably be on the same line. |
| 20 ↗ | (On Diff #15770) | @atul technically writeShellApplication runs all the code through ShellCheck. But it still is hard to review shell scripting in a string versus in a .sh file w/ highlighting, etc. Plus the ShellCheck CI is something we can review the logs for, etc. |
| 31 ↗ | (On Diff #15770) | I know you're trying to execute the script from the parent directory (with the ..), but ShellCheck is complaining (online version): If we used the CI, we would be all on the same version of ShellCheck and this could be easier to debug. |
| 34 ↗ | (On Diff #15770) | Once again, why is writeShellApplication not picking up on this? Even if nix paths can't have spaces in them, it's still standard practice to double quote variables in bash. |
| nix/redis-up-mac.nix | ||
|---|---|---|
| 12–16 ↗ | (On Diff #15770) | I separated as much as I could in the other file. If I did more, then I would just have doubled work where I set some bash variables with nix store paths, then have to assert that I was passed them on the other .sh side.
writeShellApplication runs shellcheck when it "builds" the scripts for nix develop. So you would see nix develop fail if it didn't pass. |
| 15 ↗ | (On Diff #15770) | because writeShellApplication runs the checks after nix has substitute the values, so ${redis}/bin/redis-server will expand to the nix store path (e.g. /nix/store/<hash>-redis/bin/redis-server) so shellcheck just sees an absolute path. The confusion is that nix string interpolation looks like bash variable substitution. |
| 15–16 ↗ | (On Diff #15770) | I think this is kind of subjective, but if we do ever has to pass arguments, then it's kind of nice to have one line per "configuration" setting. |
| 25–42 ↗ | (On Diff #15770) | I've separated out a lot of the "hairy" bash into separate files, all that is left is usually setting some variables, or executing/sourcing scripts. And those are much less prone to getting hit with the barrage of bash's weird subtleties |
| 31 ↗ | (On Diff #15770) | This is another nix-ism. But path literals in nix resolve the path, add that file to the nix store, then substitute the value with the nix store path. In this case, it's saying "go up from the nix directory, where this expression lives, go into the script directory and add the start_comm_daemon.sh to the nix store, now reference that file for the nix logic". This "nixification" of the script allows for nix to execute the redis-up command even when outside of the comm directory. (e.g. cd $HOME && nix run 'github:commE2E/comm?refjonringer/web-services#redis-up) $ nix build .#redis-up $ cat ./result/bin/redis-up #!/nix/store/wns6kkf93d55703pssdwz2dj0ws5pfvf-bash-5.1-p16/bin/bash set -o errexit set -o nounset set -o pipefail export PATH=":$PATH" # so use XDG conventions and hope $HOME doesn't have a space. REDIS_CACHE_DIR=${XDG_CACHE_HOME:-$HOME/Library/Caches}/Redis REDIS_PIDFILE="$REDIS_CACHE_DIR"/redis.pid mkdir -p "$REDIS_CACHE_DIR" "/nix/store/b5prjzmy2lshrl2dq6bdbf2hld9km8sw-start_comm_daemon.sh" \ redis \ Redis \ /nix/store/i5z1rbw4wfgqva6d60dl6fkdpb73igxd-redis-init/bin/redis-init \ "$REDIS_PIDFILE" echo "View Redis Logs: tail -f $REDIS_CACHE_DIR/logs" >&2 echo "Kill Redis server: pkill redis" >&2 |
| 34 ↗ | (On Diff #15770) | because it's an unambiquous absolute path when shellcheck takes a look at it. But I'll quote since it doesn't hurt. /nix/store/i5z1rbw4wfgqva6d60dl6fkdpb73igxd-redis-init/bin/redis-init \ |