-
Notifications
You must be signed in to change notification settings - Fork 9k
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 #29259
Devcontainer #29259
Conversation
@fredyshox Mind writing a small README for this? Think it's already in a pretty usable state |
I imagine some contributors are going to be relying on this, so think we should add a small test before merging this. |
Dockerfile.openpilot_dev
Outdated
@@ -0,0 +1,5 @@ | |||
FROM ghcr.io/commaai/openpilot-base:latest |
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.
would put this file in .devcontainer/
Btw I've checked how slow would it be to build aarch64 images using QEMU and That's very unfortunate, because this feature makes it super easy to have stable development experience on a mac. |
I've just found out that Azure pipelines seem to support arm64. We're already using that in some places, maybe we could utilize that for building such images. |
I don't want to introduce another CI system for this. QEMU should be fine, or BuildJet (#29277) if we end up merging that. |
tools/README.md
Outdated
``` | ||
xhost +localhost | ||
``` | ||
Note that this is temporary and only affects current XQuartz session. To make it pernament, add the above line to shell rc file. |
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.
typo: permanent
Added simple CI test for this which builds an image and runs the container using |
# remove gitconfig if exists, since its gonna be replaced by host one | ||
RUN rm -f /root/.gitconfig |
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.
won't it be shadowed anyway when it's mounted in?
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.
We are not mounting it ourselves, vscode/devcontainer does that for us, and it fails silently if it exists already.
# remove gitconfig if exists, since its gonna be replaced by host one | ||
RUN rm -f /root/.gitconfig | ||
RUN apt update && apt install -y vim net-tools usbutils htop | ||
RUN pip install ipython jupyter jupyterlab |
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.
we should consider adding this as a poetry group
tools/README.md
Outdated
|
||
openpilot supports [Dev Containers](https://containers.dev/). Dev containers provide customizable and consistent development environment wrapped inside a container. This means you can develop in a designated environment matching our primary development target, regardless of you local setup. | ||
|
||
Dev containers are supported in [multiple editors and IDEs](https://containers.dev/supporting), including Visual Studio Code. To start using it in VS Code, make sure you have `docker` installed. Then install `Dev Containers` extension. More detailed installation instructions can be found [here](https://code.visualstudio.com/docs/devcontainers/containers#_installation). |
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.
this new section is quite long. we should just link to the real instructions for each thing, i.e. https://code.visualstudio.com/docs/devcontainers/create-dev-container for this one
5abd18b
to
1d1c822
Compare
tools/README.md
Outdated
@@ -36,6 +36,32 @@ Build openpilot with this command: | |||
scons -u -j$(nproc) | |||
``` | |||
|
|||
### Dev Container | |||
|
|||
openpilot supports [Dev Containers](https://containers.dev/). Dev containers provide customizable and consistent development environment wrapped inside a container. This means you can develop in a designated environment matching our primary development target, regardless of you local setup. |
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.
you -> your
|
||
Dev containers are supported in [multiple editors and IDEs](https://containers.dev/supporting), including [Visual Studio Code](https://code.visualstudio.com/docs/devcontainers/containers). | ||
|
||
#### X11 forwarding on macOS |
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.
seems like we can link away this whole section
@@ -0,0 +1,24 @@ | |||
name: development tools |
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.
we've already got tools_tests.yaml
- name: Stop containers | ||
run: docker kill $(docker ps -q) |
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.
doesn't seem necessary
@adeebshihadeh is this mergeable now? |
.github/workflows/tools_tests.yaml
Outdated
name: devcontainer | ||
runs-on: ubuntu-latest | ||
if: github.repository == 'commaai/openpilot' |
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.
what's this for?
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.
not needed in this case
run: devcontainer build --workspace-folder . | ||
- name: Run dev container | ||
run: devcontainer up --workspace-folder . |
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.
can we also run something like "scons --dry-run" to get some coverage on the environment?
Should be fine now, can you try again? |
Same thing (this is not an efficient loop) |
New issue
|
Seems it's missing the scons cache |
Removed references to openpilot_env since it got removed |
I don't think that causes big inconvenience, as it's relatively quick to build. Including it would significantly inflate image size, which is already quite big. |
It should be volume mounted, not copied in |
"--volume=${localEnv:XAUTHORITY}:/root/.Xauthority", | ||
"--volume=${localEnv:HOME}/.comma:/root/.comma", | ||
"--volume=/tmp/comma_download_cache:/tmp/comma_download_cache", | ||
"--volume=/tmp/devcontainer_scons_cache:/tmp/scons_cache", |
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.
why is there a special devcontainer cache?
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 didn't want to mess with hosts cache, which may be incompatible (like on macos)
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.
hmm, but this is also true for the build targets
This makes it super easy to set up a dev environment with vscode.
It prepares the docker, and automatically mounts in the work dir.