added default.nix for building reproducible dev environment#3479
Conversation
|
Thanks for adding Nix support — this is a nice convenience for contributors who use Nix. Two issues to fix before this is ready:
The build script runs: ${node}/bin/node node_modules/esbuild/install.jsBut this install script was removed in older esbuild versions. Since v0.11+, esbuild ships its platform binary via optional peer packages (e.g. Hardcoded ${node}/bin/node ../napi-postinstall/lib/cli.js unrs-resolver 1.11.1 checkThe version (
cd node_modules/unrs-resolver
UNRS_VERSION=$(${node}/bin/node -e "process.stdout.write(require('./package.json').version)")
${node}/bin/node ../napi-postinstall/lib/cli.js unrs-resolver "$UNRS_VERSION" check
) |
Review notes
The (
cd node_modules/unrs-resolver
UNRS_VERSION=$(${node}/bin/node -e "process.stdout.write(require('./package.json').version)")
${node}/bin/node ../napi-postinstall/lib/cli.js unrs-resolver "$UNRS_VERSION" check
)
One improvement would be to derive the list of packages needing The |
Code Review
|
Code Review
|
|
I think the added complexity of path traversal for something that's only used in one place isn't worth it. We can fix it later if it becomes a problem. As for my little mkBuildableShell utility, I'm not planning to delete it and thought we'd prefer having just one nix file in the OGS project. If I ever do delete it, we could switch to mkShell or find a fork/backup of the (simple) code somewhere. |
|
In else
echo 'Linking node_modules to Nix store...'
rm ./node_modules # ← fails if node_modules doesn't exist
ln -sf ${node-modules}/node_modules ./node_modules
fiChange to |
ReviewOverall the approach is well-structured — pinned hashes for nixpkgs and the offline cache ensure reproducibility, and the Two issues worth addressing: 1. Unquoted The shell script uses Before: After: 2. External dependency on a personal, non-nixpkgs repository
Options to mitigate: copy the implementation inline into |
c9b0b94 to
6ed3f05
Compare
|
With That's actually the more correct location (the project's No other issues found in the current version — the previous |
|
I think the latest comment by Claude is wrong. That's what it says in the docs but not how it actually works out. My change should not be a change for non-nix users. Someone else should try and confirm. PS not knowing really anything about his, I'd support putting .vite at the project level for everyone. Shouldn't node_modules be reserved for storing node modules, not random data? But I wanted this PR to be a minimal change. |
|
Interesting, can you update the CONTRIBUTING.md to layout how this is used? How does this compare to something like docker in terms of spinning up a quick environment on linux/mac/windows ? |
|
Thanks for contributing this! Two issues worth addressing before merge: 1. Dependency on a personal GitHub repository (
A safer option would be to either inline the utility (since it's described as "simple"), or fork it under the 2. cacheDir: path.resolve(__dirname, process.env.OGS_LOCAL_VITE_CACHE ? ".vite" : "node_modules/.vite"),When To avoid the unintended side effect, only override ...(process.env.OGS_LOCAL_VITE_CACHE ? { cacheDir: path.resolve(__dirname, ".vite") } : {}), |
|
Okay, I added a bit of description. Nix has a similar goal to Docker - specifying exactly how to build and install everything, including each of its dependencies. With Nix however you are not in any kind of virtualized environment, it's just accomplished using normal environment variables like PATH and by installing each dependency in a unique location. So you can use Nix applications on your system like normal programs. You can even get NixOS, which applies this method to the entire OS. I think Yarn achieves something similar, in a more limited scope, using the hashes in yarn.lock to lock down exact versions of all the JavaScript packages. But Nix is general and it also goes all the way down. The instructions for building the specific version of NodeJS are referenced in there, as well as the exact compiler versions, Bash, etc., with no requirement for a binary blob at any point. |
|
I'm familiar with Nix as a full install or for a base image for vms or docker instances, but I haven't seen used for a development environment per-say. What's the proposed flow here? Say a contributor comes along, they're running Mac OsX, what do they do to get up and running quickly? |
|
If it's someone who is already familiar with Nix and has it installed, it should be trivially easy: just However that's still a very niche case and for a typical person, say on macOS, I'm hesitant to suggest it since maybe your normal CONTRIBUTING instructions are already good enough. But if that has been a pain point, and assuming the Nix setup on this branch does actually work on macOS, this might offer an easier path:
By the way, as an example of how it can be useful, last time I was running things in this code base, we used Speaking of more complicated things, I started looking at the e2e-tests and ran into this: Looks like that's another thing to add to the Nix config if we can figure out how to make it happen ahead of time. It's a bit troublesome that it seems to want to do a network download - is there a way to just point it at a pre-installed system chromium? |
|
Ok, so this is strictly for those running Nix. My main concern is maintaining it in this case. Neither I nor any regular contributors use Nix so I worry this is going to rot over time and no one will notice until someone comes along and spends a few hours banging their head against outdated instructions. So I think to get this merged we need to address that, I'm not sure if we can do that with a github action to add a build test or not, but if we can that would be great. Further we need some way of updating that default.nix to track things as they change, that's not manually editing and updating stuff - I see a bunch of sha hashes for versions and whatnot, so if we get updating that automated and some way of testing this so it's regularly exercised I think we're good to go. |
|
For automated testing, you could run e.g. nix-shell --command 'yarn test'which will break if someone updates yarn.lock without updating the hash correspondingly, or if the project starts to require a newer version of node/yarn than what's included presently (looks like I got v24.14.0/1.22.22). But it's fine to just leave this alone until there's more interest. I can keep using and improving it no matter what, and it's easy for others to find in the search. Maybe one of the regulars will get curious eventually, try it out, and see the benefits. |
|
When I say automated test I mean hooking it up into the CI, so github actions in our case. That might be very possible as you can specify base images and whatnot so it might just be a matter of selecting a nix base image and then running your above command. |
|
This pull request has been marked stale and will be closed soon without further activity. Please update the PR or respond to this comment if you're still interested in working on this. |
|
Updated it for the latest yarn.lock and made the update process a little smoother. |
|
With cacheDir: path.resolve(__dirname, process.env.OGS_LOCAL_VITE_CACHE ? ".vite" : "node_modules/.vite"),resolves to The relocation is arguably more sensible (the project's If the goal is to minimize impact on non-Nix users, only override ...(process.env.OGS_LOCAL_VITE_CACHE ? { cacheDir: path.resolve(__dirname, ".vite") } : {}), |
|
Nix string interpolation bug in default.nix Line 69 uses Ruby-style interpolation syntax instead of Nix syntax: The name field reads: online-go-offline-cache-#{newYarnLockHash} In Nix, string interpolation uses dollar-brace syntax, not hash-brace. The hash is never substituted, so the derivation name is the literal string containing the characters hash-open-brace-newYarnLockHash-close-brace. The newYarnLockHash binding computed on the line above is therefore unused dead code. Correct Nix syntax: name = "online-go-offline-cache-${newYarnLockHash}"; |
|
The nativeBuildInputs = [
pkgs.nodejs # default — likely Node 22 in nixpkgs-25.11, not Node 24
pkgs.yarn
pkgs.fixup-yarn-lock
napi-postinstall-alias
];This puts a different Fix: replace |
|
This pull request has been marked stale and will be closed soon without further activity. Please update the PR or respond to this comment if you're still interested in working on this. |
This PR proposes adding a
default.nixto the top level, using the Nix build system to define a precise dev environment. This can be useful for developers familiar with Nix and also casual/infrequent contributors since (after Nix is installed) they only have to run one command to enter a dev shell:No more reading docs to figure out exactly what you need to install (and what versions) to get it going! I don't know much about NodeJS environments so for me at least this satisfies a desire to keep them as encapsulated and well-defined as possible.
It works like this:
fetchYarnDepsfunction downloads the packages specified inyarn.lockand stores them in a cache folder in the Nix store. This cache is locked with a hash that will need to be updated from time to time, whenyarn.lockandpackage.jsonchange.node_modulesfolder in--offlinemode, using yarn.lock, package.json, and the cache as inputs.node_modulesinto the right place.The
node_modulesdirectory is read-only, which is a benefit since it's not possible for files in there to be corrupted. This required a change to Vite configuration to make it save its cache to the top level.viteinstead of there. I made this configurable with an environment variable, so it still defaults to the standard location.