Skip to content
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

devcontainers #358

Merged
merged 11 commits into from
Jan 21, 2025
Merged

devcontainers #358

merged 11 commits into from
Jan 21, 2025

Conversation

AshFungor
Copy link
Contributor

Hey,

Here is my proposal for devcontainers.
Folder structure:

  • devcontainers
    • build.py - script to handle cmake configure & build, plus optionally conan setup and some minor jobs as well
    • setup.py - this script creates necessary configuration for VSCode
    • conanfile.py - recipe for conan
    • templates - folder for distribution-based configs (dockerfiles and conan profiles)

build.py is optional here, one might just use regular CMake commands to achieve the same result. Conan is optional, too - template images should provide required packages. Although I suppose it is a good idea to allow picking specific versions with it.

@riebl, I would appreciate your feedback on this structure.

@AshFungor AshFungor changed the title added build script, template devcontainer for arch linux devcontainers Jan 8, 2025
@AshFungor
Copy link
Contributor Author

All scripts are ready now, same as images.

Each template folder has Dockerfile for respective container and sample conan profile. Unfortunately, the variety of platforms and system setups makes it almost impossible to write universal profile, so these ones are kind of references.

Conan is not a must to use anyway, images are built with all dependencies satisfied.

I've added docs to each piece, tested it with common uses. Sadly, as you mentioned, maintaining this is tough and I don't see an easy way to automate testing for now.

Let me know your thoughts, @riebl.

@AshFungor AshFungor marked this pull request as ready for review January 9, 2025 02:25
@riebl
Copy link
Owner

riebl commented Jan 9, 2025

Thanks for your efforts! It looks quite good after a first quick checkup. Please give me some more time, because I would like to play with it as soon as I am back from my business trip.

Does it have any particular benefit to support multiple base images for the DevContainer setup?
Could the maintenance effort be reduced by just using a minimal base image and relying entirely on Conan for a full build environment?
Though not critical for DevContainers, the Dockerfiles could benefit from a multi-stage approach. Furthermore, removing stuff from the container images in separate RUN steps still "bloats" the overall image size. But as I said, that is not critical for the moment.

@AshFungor
Copy link
Contributor Author

AshFungor commented Jan 9, 2025

Of course, no problem :)

  1. Base images. I had initial idea of supporting several popular distributions for convenience, but now I realize that we probably just need stable and rolling release to meet most people's needs. I tested both Debian and Arch Linux image on my Arch Linux machine and they all worked just fine. Considering Debian has unstable releases too, we can just leave Arch Linux image out, if you prefer it this way.

  2. I'm glad you mentioned conan. To be honest, I don't like forcing people into using specific tool, unless there is no other choice. Conan is a wonderful tool if you know what are you doing, but someone might not be used to it, so providing dependencies system-wide might be a good approach.
    By the way, should I make a setting for separate volume for conan folder? Stopping container clears default path for conan folder (defined as CONAN_HOME) and upon VSCode reloading you have to compile everything again. One might set CONAN_HOME to point somewhere in build directory, so changes are persisted on host machine, but I don't know, seems counter-intuitive a bit.

  3. I think we might just remove that rm -r at this point, it takes quite some time in build process and yields no benefits. We can also move package cache clear command in base stage, too.

  4. Sorry, I didn't get your point about multi-stage builds? :) I thought mine are multistaged

@AshFungor
Copy link
Contributor Author

Looks like mounting XAUTHORITY temporary file was a bad idea since it is generated on boot with random name, and docker does not allow recreating mounts. The only solution I found was this:
microsoft/vscode-remote-release#9955 (comment)

Also did 3) #358 (comment)

@AshFungor
Copy link
Contributor Author

By the way: VSCode resets extensions in devcontainer on every reload unless you specify them like this:

"customizations": {
        "vscode": {
            "extensions": [
                "ms-python.vscode-pylance",
                "waderyan.gitblame",
                "twxs.cmake",
                "llvm-vs-code-extensions.vscode-clangd"
            ]
        }
    }

You can add them with Extensions UI, too :)

Copy link
Owner

@riebl riebl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am even considering switching over to the new dev containers :-)
Some step-by-step usage instructions for beginners are still needed, though.
Great work and looking forward to your replies to my comments!

devcontainers/setup.py Outdated Show resolved Hide resolved
devcontainers/setup.py Outdated Show resolved Hide resolved
devcontainers/build.py Outdated Show resolved Hide resolved
devcontainers/build.py Outdated Show resolved Hide resolved
devcontainers/build.py Outdated Show resolved Hide resolved
devcontainers/templates/debian/Dockerfile Outdated Show resolved Hide resolved
devcontainers/templates/arch-linux/Dockerfile Outdated Show resolved Hide resolved
@AshFungor AshFungor requested a review from riebl January 20, 2025 21:15
Copy link
Owner

@riebl riebl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! I am currently testing some different setups, e.g. what happens when running Wayland on the host system instead of X11.

Furthermore, I am thinking about moving some files:

  • devcontainers/templates/*/Dockerfile to .devcontainer/*/Dockerfile
  • devcontainers/templates/*/x86-64.ini to conan/*-x86_64.ini
  • devcontainers/build.py to the root directory
  • devcontainers/conanfile.py either to root directory or to conan subdirectory
    Motivation: Except for the Dockerfiles, the files in devcontainers are not limited to devcontainers.

.devcontainer/arch-linux/devcontainer.json Outdated Show resolved Hide resolved
.gitignore Outdated Show resolved Hide resolved
@AshFungor
Copy link
Contributor Author

Wayland

I think wayland has different unix socket we'll need to connect to. Might be better to structure our folders like this, then:

.devcontainers
├── Wayland
│   ├── arch-linux
│   └── debian
└── X11
    ├── arch-linux
    └── debian

@riebl
Copy link
Owner

riebl commented Jan 21, 2025

Something is still a little bit odd: I can remove all the /tmp/.X11-unix, DISPLAY and xhost snippets from the Arch Linux devcontainer.json and still run /sumo/bin/sumo-gui without problems even with Wayland. In fact, the VSCode DevContainer extension is expected to handle the X11 magic nowadays.
But it fails for Debian container for some reason.

@AshFungor
Copy link
Contributor Author

maybe sumo version is too new?
I was running everyting from ArchLinux, probably you can try downgrade sumo version?)

@riebl
Copy link
Owner

riebl commented Jan 21, 2025

Okay, I think this PR is ready for merging. I still don't know why the X11 display requires the explicit configuration with Debian but not Arch Linux, but we can improve this later on.
Maybe you @AshFungor can check if removing the X11 stuff from arch-linux/devcontainer.json also works on your system?

@riebl riebl merged commit d82fba4 into riebl:master Jan 21, 2025
@AshFungor
Copy link
Contributor Author

if removing the X11 stuff from arch-linux/devcontainer.json also works on your system

It does ;)
I'm quite happy to see stuff working without interference.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants