[Nix] Use vanilla direnv
ClosedPublic
Actions

Authored by • jon on Jan 11 2023, 10:55 AM.

Details

Reviewers

atul
ashoat
varun

Commits

rCOMMb854a1697b98: [Nix] Use vanilla direnv

Summary

Sourcing nix_direnv seems to cause some potential
issues on machines with older versions of bash. Instead, use
a script which has some fallback behavior to determine the
desired action.

This also preserves the parent shell (e.g. zsh, fish, or bash)

Test Plan

Tested on intel mac, using zsh and bash.

Install direnv, e.g. brew install direnv
Then run

# from parent directory
cd <comm root>

Diff Detail

Repository

rCOMM Comm

Branch

jonringer/direnv

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

• jon created this revision.Jan 11 2023, 10:55 AM

Herald added a subscriber: tomek. · View Herald TranscriptJan 11 2023, 10:55 AM

Harbormaster completed remote builds in B15259: Diff 20805.Jan 11 2023, 11:07 AM

• jon requested review of this revision.Jan 11 2023, 11:07 AM

ashoat requested changes to this revision.Jan 11 2023, 11:39 AM

ashoat added inline comments.

.envrc
1	Given this has bash code in it, do we need a shebang at the start or something? Not super familiar with the `.envrc` file and its format
6	This file is only going to work with bash now that it's using `[[`, right? Will this cause problems for people using other shells? Related: ENG-2698
6–12	This logic is kind of "magic" and confusing to read. The confusing parts are that operator precedence isn't obvious, and the fact that the different parts here are conditionals that control whether to execute the subsequent parts Instead, can we make the conditionals explicit with eg. `if` statements? What do you think?

This revision now requires changes to proceed.Jan 11 2023, 11:39 AM

Strip out all unneeded defaulting logic

• jon marked 2 inline comments as done.Jan 12 2023, 11:05 AM

• jon added inline comments.

.envrc
1	No, this is meant to be sourced. Not executed.
6	`[[` seems to be supported by bash 3+, zsh, and fish (at least, i was able to use direnv with each of these shells). I can use `[ "$(type -t use_flake)" -eq "function" ]` if needed.
6–12	Sure, let me try
6–12	Seems that the `[[ $(type -t use_flake) == function ]] && use flake` was only necessary when direnv was first rolling out flake support (this is now 3-4 years ago), so no longer relevant. # shell gc root dir mkdir -p "$(direnv_layout_dir)" eval "$(nix print-dev-env --profile $(direnv_layout_dir)/flake-profile)" is imitating `use flake` by hand-rolling a similar workflow (now obsolete) `use nix` reads in a `shell.nix`, which we don't use as that's been replaced by flakes + `nix develop` (also obsolete). Since `./scripts/install_nix.sh` will set up nix + flakes, we can just use `use flake`.

• jon marked 2 inline comments as done.Jan 12 2023, 11:06 AM

• jon added inline comments.

.envrc
6	`[[` is no longer relevant after removing logic for non-flake workflows.

Harbormaster completed remote builds in B15317: Diff 20892.Jan 12 2023, 11:16 AM

Patched this into my local environment and cding into comm seems to continue working as expected (yarn cleaninstall, yarn dev in a few of the workspaces, etc)

.envrc
2–3 ↗	(On Diff #20892)	Just curious why we need to watch both `flake.nix` and `flake.lock`? I'd assume that any changes to `flake.nix` would be reflected in `flake.lock`?

• jon marked an inline comment as done.Jan 12 2023, 2:04 PM

• jon added inline comments.

.envrc
2–3 ↗	(On Diff #20892)	We need both. `flake.lock` contains the exact checkout of nixpkgs. Without it, if I were to update the nixpkgs pin, then people won't get the new packages from nixpkgs.

Whoa this is lovely and clean now

This revision is now accepted and ready to land.Jan 12 2023, 3:02 PM

atul added inline comments.Jan 12 2023, 3:26 PM

.envrc
2–3 ↗	(On Diff #20892)	Was wondering if we could watch just `flake.lock`

• jon marked 2 inline comments as done.Jan 17 2023, 7:02 AM

• jon added inline comments.

.envrc
2–3 ↗	(On Diff #20892)	If we are unlikely to change `nix/overlay.nix` or `flake.nix`. I just want to be absolutely certain, and I think it's a low bar to reference the other two files