A lot has happened since the first initial yarn dev diff
This is to update the dependencies as they have evolved since
then
Depends on D3865
Details
Details
- Reviewers
ashoat varun - Commits
- rCOMM526318477a08: Update nix dev shell
Run nix develop to create shell.
Run yarn cleaninstall to build all yarn projects
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
Comment Actions
Thank you for adding all of these!!
| nix/dev-shell.nix | ||
|---|---|---|
| 8 | I don't think we want to pull Flow from Nix. It's handled by yarn, and it's important the version matches what is in the various package.jsons. Instead of these, maybe can we set up a shell alias from flow to yarn flow? | |
| 12–13 | What are each of these for? | |
| 14 | I assume something needs to compile against this? What is it? | |
| 24 | I might be looking in the wrong place, but it seems like Nix's version of Arcanist is a little old. Is there a way to update it? | |
| nix/dev-shell.nix | ||
|---|---|---|
| 8 | I can remove it from nix. If it's part of a development flow, and people are expected to run yarn install, then the utility of having a pre-packaged version is much less important. | |
| 12–13 | python2 was added, because it's mentioned now in the dev docs python3 was added because one of the yarn builds requires it. | |
| 14 | I added it because of the mention: https://github.com/CommE2E/comm/blob/master/docs/dev_environment.md#python-2-apple-silicon-macs I can remove this | |
| 24 | I can update it upstream a bump. It's now only 5 days old https://github.com/NixOS/nixpkgs/pull/171040 | |
| nix/dev-shell.nix | ||
|---|---|---|
| 8 | Agree, let's remove it | |
| 12–13 | I'm pretty sure that (quite unfortunately) the script that needs Python 2 actually needs python to default to Python 2.... :( cc @atul | |
| 14 | Does this make it so that the buildscript from that sqlite3 npm package no longer needs to run? I don't think the sqlite3 npm package expects sqlite3 sources / binary anywhere, so unless this has some utility for us (eg. lets us avoid the annoying Python2-requiring buildscript) then we should probably leave it out for now. (Note that in our actual application we actually use a fork of sqlite3 called SQLCipher that supports encryption-at-rest – we only need sqlite3 as a transitive dependency for some dev tool) | |
| 24 | Thank you!! | |
Comment Actions
Back to you for more follow-ups – not all of my initial comments have been addressed (eg. Flow is still there)
Comment Actions
Remove flow
| nix/dev-shell.nix | ||
|---|---|---|
| 12–13 | That should be current behavior (nix-shell) [01:22:21] jon@jon-desktop /home/jon/comm/comm (joringer/add-nix-clean) $ python --version Python 2.7.18 (nix-shell) [01:23:40] jon@jon-desktop /home/jon/comm/comm (joringer/add-nix-clean) $ python3 --version Python 3.9.12 | |
| 14 | I believe I still needed it on m1. Linux I did not. But I would prefer not to do as much conditional logic, so linux can have a python2 interpreter :) | |
| nix/dev-shell.nix | ||
|---|---|---|
| 27 ↗ | (On Diff #12320) | Confused why we still have this, see my previous feedback |
| nix/dev-shell.nix | ||
|---|---|---|
| 27 ↗ | (On Diff #12320) | Yep, my bad. |
Comment Actions
Woohoo, we have a bunch more stuff in there now. @varun – can you take a look and see if we need another else for the Rust environment to work with nix develop?
Comment Actions
I think we need rustup so we can use other Rust tools besides cargo. It seems like this is a common way that people add Rust to their devShell. Thoughts?
Comment Actions
rustup allows you to switch between rust channels easily, and I can reflect the rust version needed with nix. I just used the latest stable, as that's generally enough unless you need a nightly toolchain.
Comment Actions
quick question inline, but lgtm!
| nix/dev-shell.nix | ||
|---|---|---|
| 36–37 ↗ | (On Diff #12566) | are these comments placeholders? |
| nix/dev-shell.nix | ||
|---|---|---|
| 36–37 ↗ | (On Diff #12566) | it's more or less to avoid people thinking that clang or gcc need to be explicitly mentioned |