Details

Reviewers

tomek
• karol
• max
• jim
varun
ashoat
atul

Commits

rCOMM5dfdee963432: Add intial nix yarn dev env

Summary

Add initial nix dev environment for yarn workflows

Test Plan

Install nix and flakes (out of scope for this PR, part of D2921)
Run nix develop
Do workflows which require yarn

Diff Detail

Repository

rCOMM Comm

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

• jon added inline comments.Mar 21 2022, 5:40 AM

nix/overlay.nix
5 ↗	(On Diff #9874)	`final` references the package set after all changes, which allows use to refer to the `androidDevEnv` created above.

Harbormaster completed remote builds in B7506: Diff 10563.Mar 21 2022, 5:41 AM

Harbormaster completed remote builds in B7507: Diff 10564.Mar 21 2022, 5:45 AM

Tested it (by running yarn dev in native directory) and it works nice, but it would be better if we could just run nix develop instead of nix develop --extra-experimental-features nix-command --extra-experimental-features flakes. Assigning @jimpo as a blocking reviewer as he had some questions

but it would be better if we could just run nix develop instead of nix develop --extra-experimental-features nix-command --extra-experimental-features flakes

Maybe a crude approach, but we could add something like yarn nix-dev to package.json as an alias for nix develop --extra-experimental-features nix-command --extra-experimental-features flakes?

Maybe a crude approach, but we could add something like yarn nix-dev to package.json as an alias for nix develop --extra-experimental-features nix-command --extra-experimental-features flakes?

You just need to put "experimental-features = nix-command flakes" in your nix.conf, which should be documented somewhere. Or, put NIX_CONFIG in the envrc.

Last question here is whether we're OK merging this without documentation, or whether I'm just missing where the documentation is? Probably a question for @ashoat.

atul resigned from this revision.Apr 5 2022, 9:20 AM

Last question here is whether we're OK merging this without documentation, or whether I'm just missing where the documentation is? Probably a question for @ashoat.

The documentation is in D2921, which needs revision from @jonringer-comm

In D3274#99283, @varun wrote:

Last question here is whether we're OK merging this without documentation, or whether I'm just missing where the documentation is? Probably a question for @ashoat.

The documentation is in D2921, which needs revision from @jonringer-comm

Nvm, I'm going to commandeer that diff

Remove nix's flow, fix NixOS support

Nix's flow is more up-to-date, and will likely cause
work to bump flow version each time nix sees an update

You just need to put "experimental-features = nix-command flakes" in your nix.conf, which should be documented somewhere. Or, put NIX_CONFIG in the envrc.

This is documented in https://phabricator.ashoat.com/D2921

iOS and Android scenarios are not yet taken care of, as they are quite a bit more complex than the yarn workflows.

Harbormaster completed remote builds in B7917: Diff 11099.Apr 6 2022, 7:35 AM

Removing merge conflicts with D2921, which should be a dependency of this one

ashoat added a parent revision: Restricted Differential Revision.Apr 6 2022, 1:14 PM

Pulling out the Android changes

ashoat added a child revision: D3640: Add Android to Nix.Apr 6 2022, 1:24 PM

ashoat added inline comments.Apr 6 2022, 1:25 PM

.envrc
1 ↗	(On Diff #11099)	Is this a directive to the reader, or a description of what happens?
1–11 ↗	(On Diff #11099)	What's this file? What's the format... it looks like bash, but I'm not really sure?
2 ↗	(On Diff #11099)	Is `watch_file` a bash command?
11 ↗	(On Diff #11099)	Is this `\|\|` saying "if the stuff in the `{ }` block fails, only then should we run `use nix`"?
docs/dev_environment.md
5 ↗	(On Diff #11099)	It appears that you've made the same edit in two diffs. These diffs should be in a stack (ie. in the same branch), and so it should impossible for this to happen. It seems pretty clear that these two diffs are not in the same branch. You also haven't specified any dependency between the two

Harbormaster failed remote builds in B7938: Diff 11125!Apr 6 2022, 1:28 PM

Harbormaster completed remote builds in B7937: Diff 11124.Apr 6 2022, 2:02 PM

• jim accepted this revision.Apr 7 2022, 12:52 PM

This revision is now accepted and ready to land.Apr 7 2022, 12:52 PM

Add initial nix yarn development shell

Harbormaster completed remote builds in B8002: Diff 11207.Apr 7 2022, 11:07 PM

Prune accidental mention of android

Harbormaster completed remote builds in B8003: Diff 11208.Apr 7 2022, 11:13 PM

• jon retitled this revision from Add intial nix dev env to Add intial nix yarn dev env.Apr 7 2022, 11:20 PM

• jon edited the summary of this revision. (Show Details)

• jon edited the test plan for this revision. (Show Details)

ashoat added a reviewer: varun.Apr 8 2022, 8:20 AM

This revision now requires review to proceed.Apr 8 2022, 8:20 AM

Setting @varun as a blocking reviewer because he's taking the lead on reviewing Nix stuff and I think we should have at least one full-time person who understands the nuances of the Haskell-like syntax that .nix files use. I'm having a hard time reading through / grokking these files and think it's important that at least one reviewer here is able to understand what is being written

ashoat added inline comments.Apr 8 2022, 2:27 PM

flake.nix
21–23 ↗	(On Diff #11208)	Why do we define `overlays` again when it's already defined on line 14?
32 ↗	(On Diff #11208)	What does the parantheses mean?

small nit in addition to ashoat's questions

nix/overlay.nix
1 ↗	(On Diff #11208)	"refers to"

This revision now requires changes to proceed.Apr 13 2022, 12:34 PM

Improve wording of comments in overlay.nix

Inherit overlays from earlier in let block and avoid redefining value

• jon mentioned this in D3792: Improve wording of comments in overlay.nix.Apr 20 2022, 12:41 PM

Harbormaster completed remote builds in B8360: Diff 11671.Apr 20 2022, 12:42 PM

• jon added a child revision: D3784: Make script shebangs POSIX friendly.Apr 20 2022, 12:45 PM

Harbormaster completed remote builds in B8361: Diff 11672.Apr 20 2022, 12:46 PM

sorry, just a few more q's -- i think this is really close

flake.nix
29 ↗	(On Diff #11672)	still unclear what the parentheses around `legacyPackages` means. can you clarify?
33–34 ↗	(On Diff #11672)	From the NixOS wiki: Default overlay, consumed by other flakes `overlay = final: prev: { };` Same idea as overlay but a list or attrset of them. `overlays = {};` Can you clarify why we need both `overlays` and `overlay`? Didn't really understand that. Also can we not just inherit `overlays` from the outer scope?
nix/dev-shell.nix
11 ↗	(On Diff #11672)	why is this a recursive set?

This revision now requires changes to proceed.Apr 27 2022, 10:17 AM

• jon added inline comments.Apr 27 2022, 10:44 AM

flake.nix
29 ↗	(On Diff #11672)	This is syntax sugar for `devShell = legacyPackages.devShell`. It can be extended to pull out many packages as well. E.g. `inherit (legacyPackages) devShell tunnelbroker;`
33–34 ↗	(On Diff #11672)	We don't need to expose overlays yet as we only have one, this is more or less about completeness. I can remove if it's confusing, but in the future we may want to expose more than one overlay (or more likely, another overlay which we might use).
nix/dev-shell.nix
11 ↗	(On Diff #11672)	Most likely muscle memory, this doesn't need it. However, I did prune this a bit for this diff, and may have required rec before the changes and I forgot to remove the rec. From an implications standpoint, this shouldn't really affect much, generally recursive sets are a little slower (~1.5x slower) to evaluate, but shouldn't be noticeable as this attr set is quite small

varun added inline comments.Apr 27 2022, 11:18 AM

flake.nix
29 ↗	(On Diff #11672)	got it, thanks for explaining
33–34 ↗	(On Diff #11672)	I think it's fine to keep it. Thanks for explaining
nix/dev-shell.nix
11 ↗	(On Diff #11672)	i think we should remove it since none of the attributes refer to one another. what do you think?

• jon marked 2 inline comments as done.Apr 27 2022, 2:17 PM

• jon added inline comments.

nix/dev-shell.nix
11 ↗	(On Diff #11672)	Sure

• jon marked 11 inline comments as done.Apr 27 2022, 2:19 PM

• jon added inline comments.

flake.nix
21–23 ↗	(On Diff #11208)	over looked this, fixed.

• jon marked an inline comment as done.Apr 27 2022, 2:20 PM

• jon requested review of this revision.Apr 27 2022, 2:22 PM

@jonringer-comm why did you re-request review here? You still need to strip the rec.

The only scenario you should ever use the "Re-request review" feature in is if you want to send the diff back to your reviewers' queue without any changes

This revision now requires changes to proceed.Apr 27 2022, 2:51 PM

Remove unused rec