Publish dockerfile #30
Conversation
This causes the container build process to execute each of the programs, querying the version of each program. If any program cannot report its version, the build process fails.
Users that need a CLI code editor can add it back in themselves. Users that are using the Dockerfile to build and deploy code can add their project themselves.
Codecov Report
@@ Coverage Diff @@
## main #30 +/- ##
=======================================
Coverage 77.77% 77.77%
=======================================
Files 3 3
Lines 36 36
Branches 19 19
=======================================
Hits 28 28
Misses 7 7
Partials 1 1
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report at Codecov.
|
|
To make this work, some settings will need to be changed. This process is partially described in https://docs.github.com/en/packages/working-with-a-github-packages-registry/working-with-the-container-registry.
After these settings are changed, cpp_boilerplate_project will be linked to the packages that this workflow generates. You can see what that would look like here, on my personal fork of cpp_boilerplate_project: https://github.com/ddalcino/cpp_boilerplate_project/pkgs/container/cpp. |
There was a problem hiding this comment.
I think we should have multiple base containers for every combination of the provided variables that devcontainer can inherit from and we support. (e.g. ghcr.io/cpp_best_practices/cpp:focal:11:13, ghcr.io/cpp_best_practices/cpp:bionic:11:13 etc..)
So that the FROM call should look like FROM ghcr.io/cpp_best_practices/cpp:${VARIANT}:${GCC_VER}:${LLVM_VER}:0.1.0
| # Some packages request that Conan use the system package manager to install | ||
| # a few dependencies. This flag allows Conan to proceed with these installations; | ||
| # leaving this flag undefined can cause some installation failures. | ||
| ENV CONAN_SYSREQUIRES_MODE enabled |
There was a problem hiding this comment.
ADD CONAN_REVISIONS_ENABLED=1. This should also be considered as the default
There was a problem hiding this comment.
I am actually considering removing all of the CONAN_* variables; I think they might fall under the category of "things that aren't needed by all users, and don't belong here". They can easily be added to .devcontainer/Dockerfile if necessary.
There was a problem hiding this comment.
Let's keep here only the bare minimum things that are needed to compile this project. However. I think that conan is needed here. Also, I would like to have consistent behavior between devcontainer and the CI. SO if you remove the conan how the CI will compile this?
There was a problem hiding this comment.
You raise a good point: We should not release a Dockerfile that cannot build the project. That means we need an extra CI step that tests the Dockerfile, to prove that it can build the project: if it fails this step, the workflow should not be allowed to push the container to GHCI.
The idea of 'removing Conan' should really be, 'move Conan out of docker/Dockerfile and into .devcontainer/Dockerfile'. The point of this change would be to allow users the choice of a different package manager, for instance vcpkg (see our sister project https://github.com/aminya/cpp_vcpkg_project). These users would need to extend docker/Dockerfile to make it work with vcpkg, but they would be able to do so without polluting their environment variables with Conan-specific variables they don't need, and wasting extra disk space on Conan programs they aren't going to use.
Now that I think about it, I think this is outside the scope of this PR, and that moving Conan-specific code should wait for another PR.
There was a problem hiding this comment.
The package manager selection could be just another variable in the container. To use the selected package manager, we should just pass the correct CMAKE_TOOLCHAIN_FILE generated by the (CMakeToolchain, CMakeDeps) conan and vcpkg. This is the least intrusive way to set up dependencies since I believe that CMakeLists.txt files should be package manager agnostic. Then the -DCMAKE_TOOLCHAIN_FILE=conan_toolchain.cmake can be passed using a 'conan' spesific preset. Well this is outside of the scope of this PR though.
| ARG VARIANT="focal" | ||
| FROM ubuntu:${VARIANT} | ||
| # FROM ghcr.io/cpp_best_practices/cpp:0.1.0 # TODO: activate | ||
| FROM ghcr.io/ddalcino/cpp:0.1.0 |
There was a problem hiding this comment.
How the variables like ${VARIANT}, ${LLVM_VER} etc. are passed from the devcontainer.json file to the actual dev container that VSCode use with setup?
There was a problem hiding this comment.
We can add a matrix to .github/workflows/publish-dockerfile.yml that builds separate containers with all the different variables, and names them appropriately. It should not be too difficult to do.
| ARG VARIANT | ||
|
|
||
| # Install necessary packages available from standard repos | ||
| RUN apt-get update -qq && export DEBIAN_FRONTEND=noninteractive && \ |
There was a problem hiding this comment.
I also have a non-root user implementation https://github.com/Jason5480/cpp_boilerplate_project/blob/aab9da20a2a29578dd119b543785a5cdf36a6349/.devcontainer/Dockerfile#L11 that works but I would prefer to add it in a separate PR after this merge.
There was a problem hiding this comment.
Great! I'm sure that will be very useful
Co-authored-by: Iason Nikolas <jasonnikolas5480@gmail.com>
Co-authored-by: Iason Nikolas <jasonnikolas5480@gmail.com>
ddalcino
force-pushed
the
publish-dockerfile
branch
from
0ae68b9
to
5db36c9
Compare
I am planning on doing something like this, but I'm not sure about the names. I want to follow established naming/versioning conventions, but I'm not certain what they are. Is a Oh, and thanks for the feedback! It's very useful. |
| #WORKDIR /workspaces/cpp_starter_project | ||
|
|
||
| CMD ["/bin/bash"] | ||
| # Add your own modifications to the Docker environment here |
There was a problem hiding this comment.
editors were removed. I agree with this change since the most common use cases don't need them. However, you can add the commands commented out as an example.
There was a problem hiding this comment.
Yes, I removed the editors and the commented-out code that you wanted to remove in #8, and I asked you not to. I was trying to make the old Dockerfile do too many things at once, and that was never going to work out. I was wrong. Thank you for disagreeing with me, and being a good sport about the disagreement.
Giving descriptive prefixes is even better! But what happens with the optional variables like ${USE_CLANG} and with the ${USE_ROOT} I added in my branch? I think that the ${USE_CLANG} is not needed any more since we use presets, and we pass the desired compiler through CMake. Now the second optional variable will be discussed later. Another idea I had is to support within devcontainers all containers that the CI supports. So that I can easily reproduce in my machine the ci builds no matter what my native OS is. For example, if I make a change and my local windows build succeed but the CI did not, I would like to be able to have the windows container in my machine to reproduce that failure. Even better, being able to select the devcontainer OS between the "big three" can be very handy in many situations. I understand this may be a huge development so this can be done in a separate PR. For now, I would just add an "os-unixlike-${VARIANT}" prefix to the container name in case we add more in the future! Thank you too for all those contributions you make. |
I haven't looked at it yet, but the state of USE_ROOT sounds like a very useful thing to add to the container tag.
I think it definitely makes sense to remove USE_CLANG from the parent
Have you looked into Github self-hosted runners? It sounds like that's what you are describing. I'm not too certain though; I have never used them. The only real way for the Docker container to truly mirror the GH workflow CI environment would be to run this Docker container in CI. We can get close to the CI environments by removing most of the Docker container and replacing it with setup-cpp, the same tool we use to install most of our packages in CI, but there would always be differences. There are a few things that our container can do that setup-cpp cannot, for instance, install |
| with: | ||
| file: ./docker/Dockerfile | ||
| context: ./docker | ||
| push: ${{ github.ref == 'refs/heads/main' }} |
There was a problem hiding this comment.
This check is insufficient. What we need here is to check that:
- we are on the main branch
- there isn't already an existing container in our package repository that has the same tag, that a push would mistakenly overwrite. We don't want to overwrite an existing
cpp-gcc11-llvm-13:0.1.1when we have forgotten to bump the version to0.1.2; there may not be a way to get the original back.
If both these conditions are true, it should be safe to push a new version of the container.
I'm not so experienced with GH workflow syntax, and I'm not clear on how to make this check work. Honestly, I'm not even sure if ${{ github.ref == 'refs/heads/main' }} does what it looks like it does.
There was a problem hiding this comment.
Hmmm not familiar with GH syntax either. However, this "problem" reminds me why conan introduced revisions for recipes. Docker images (like recipes) have different development life cycles thus you need a way to reference back every change both in code and container to have a reproducible environment. I found this post https://stackoverflow.com/questions/56212495/properly-versioning-docker-images that give some options we have about versioning that might be helpful.
|
What is pushing this PR back? |
This separates
.devcontainer/Dockerfileinto dependent and dependee Dockerfiles. One Dockerfile will be built and published in the Github Container Repository during a CI workflow. The Dockerfile that remains in the.devcontainerfolder will depend on this published container.This will:
Fix cpp-best-practices/gui_starter_template#210
Uses two commits from #8