-
-
Notifications
You must be signed in to change notification settings - Fork 37
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
Devcontainer improvements #10
base: master
Are you sure you want to change the base?
Devcontainer improvements #10
Conversation
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.
Awesome. Thanks for taking what I started and running with it!
@@ -94,23 +128,19 @@ let | |||
# make sure /tmp exists | |||
mkdir -m 0777 tmp | |||
|
|||
# allow ubuntu ELF binaries to run. VSCode copies it's own. | |||
mkdir -p lib64 | |||
ln -s ${glibc}/lib64/ld-linux-x86-64.so.2 lib64/ld-linux-x86-64.so.2 |
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.
are you sure you don't want to keep this? Other extensions might also download random binaries.
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 would add later if there would be some issues, would not make this default as it solves only part of problem. If some third party binary is not statically linked, it's a good chance it will require also some other libraries. Regarding installing of vscode plugins can probably be automatized using home-manager
module and binaries can be patched there.
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.
please don't remove this otherwise it will break actions/checkout@v3 when we use devcontainer as github action iamge
# VSCode assumes that /sbin/ip exists | ||
mkdir sbin | ||
ln -s /nix/var/nix/profiles/default/bin/ip sbin/ip | ||
''; | ||
|
||
config = { | ||
Cmd = [ "/nix/var/nix/profiles/default/bin/bash" ]; | ||
Entrypoint = [ "/nix/var/nix/profiles/default/bin/entrypoint.sh" ]; |
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.
When I was hacking on it, vscode was running the container with its own entry point. Is that not the case anymore?
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.
Microsoft does seem to do the same in some of their devcontainers: https://github.com/microsoft/vscode-dev-containers/blob/560deca1511296d64d31acea1ba08feea3f7850b/containers/docker-from-docker/.devcontainer/Dockerfile#L26
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 mean it works for me if using docker-compose, but have to still test with plain devcontainer.
3f2de75
to
6264454
Compare
@offlinehacker sorry this went out of my radar. Would you like me to merge this PR? |
I can't say, I don't use it currently, but maybe others find it useful. I don't like some parts of how nix is building docker images, as it's too magic. I switched to debian based image and wrote: https://github.com/xtruder/debian-nix-devcontainer, which I am successfully using for of all of my projects. |
Yeah, that makes sense. I also think that it will be less pain as tooling in vscode tends to assume a debian-like environment. |
This pull request implements several improvements for devcontainers:
LD_LIBRARY_PATH
, but by usingentrypoint
that does patchelf on vscode node binary. See also comment in code.I also implemented example repo that is using this image for devcontainer: https://github.com/xtruder/nix-devcontainer