-
-
Notifications
You must be signed in to change notification settings - Fork 10
feat: make use of Nix files to set up dev env #22
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
Kicked off a build to see if the image built from this branch works https://github.com/joyeecheung/devcontainer/actions/runs/20376852332 (when it's done it'll be published to https://hub.docker.com/r/joyeecheung/node-devcontainer) |
Co-authored-by: Joyee Cheung <joyeec9h3@gmail.com>
|
|
||
| # Setting up direnv for the local clone | ||
| ARG USE_SHARED_LIBS=false | ||
| # As much as possible, we want to use the defaults set in shell.nix so the DX is consistent for users of devcontainers and users of Nix. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel that we still need to figure out how to add extra configuration flags to this instead of saying "Don't" - at some point the CI use case is bound to diverge from the local development use case e.g. in terms of whether to compile with -g - might not be very critical in the CI, or may be a bit excessive, but it's critical for local debugging - it would be better for the devcontainer to ship the build cache with the debug symbols, otherwise the image is quite limited for the development use case. Can we split that with different arguments, so that we can have different images built for different use cases?
Also, after this lands, is there way to test build configurations without landing a change in the upstream first? I feel that if changing the image building process requires changing the upstream, it could end up creating a circular dependency that would be difficult to break if this image is going to be used in the upstream for tests somehow, which is the primary reason why we are trying to unify the two. Can we update the flow to allow a bit more flexibility, say, instead of always pulling from the upstream and creating a dependency in the building process, we save a copy of those scripts here and add a daily workflow to update them if necessary? That way to break up the circular dependency for testing, we can just update one of the copy in a PR, and sync up later after they get merged. Or, we can introduce some kind of overriding config here that can be used to update the two independently to break up the circular dependency?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
at some point the CI use case is bound to diverge from the local development
100%, CI already diverges by passing extra args and by opting out of e.g. devTools and benchmarkTools, see https://github.com/nodejs/node/blob/607a741941994a5f27774a807ef36149963cb821/.github/workflows/test-shared.yml#L191-L195
IMO the defaults in shell.nix should provide an "ideal dev env", because it's much easier for CI to override arguments than to expect all users to fiddle with passing custom flags.
it would be better for the devcontainer to ship the build cache with the debug symbols, otherwise the image is quite limited for the development use case
I agree, and my point is that whatever flag would make sense as default in devcontainer also make sense as default for Nix users, because the usecases of both systems overlap almost perfectly IIUC (and CI is a different beast, but can then use non-default options).
Regarding your concern about a possible circular dependency situation, I don't foresee any problem; we can override the env variable Nix is creating for us, or just re-run ./configure with different flags – but also, updating (e.g. by pulling a different commit) the *.nix files locally would also be picked up, so even for a CI use case that should be fine.
Passing flags when building the image is possible, and I'm not against it if there are use cases for it, my point is we should resist it so we don't end up maintaining two different sets of defaults if we can avoid it.
I first attempted to create the Docker image using Nix tools, but that ended up being too complicated, in the end it was easier to install Nix from the Dockerfile, and update the existing scripts to use Nix provided tools.
Worth noting the image contains a
nodebuilt with all shared libs, similar to what buildstest-sharedGHA workflow on nodejs/node; we could certainly disable that to use the vendored static deps instead, the tradeoff being the build would take longer.Fixes: #18