Skip to content
This repository has been archived by the owner on Apr 19, 2023. It is now read-only.

Publish dockerfile #30

Draft
wants to merge 12 commits into
base: develop
Choose a base branch
from

Conversation

ddalcino
Copy link
Contributor

@ddalcino ddalcino commented Apr 3, 2022

This separates .devcontainer/Dockerfile into 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 .devcontainer folder will depend on this published container.

This will:

Fix cpp-best-practices/gui_starter_template#210

Uses two commits from #8

Iason Nikolas and others added 9 commits April 3, 2022 09:46
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-commenter
Copy link

codecov-commenter commented Apr 3, 2022

Codecov Report

Merging #30 (dc14794) into main (f37c1ab) will not change coverage.
The diff coverage is n/a.

@@           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           
Flag Coverage Δ
Linux 37.03% <ø> (ø)
Windows 80.00% <ø> (ø)
macOS 38.46% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.


Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f37c1ab...dc14794. Read the comment docs.

@ddalcino
Copy link
Contributor Author

ddalcino commented Apr 3, 2022

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.

  • We need to add a repository secret called PAT, with the value of a new personal access token with permissions to read, write, and delete packages in GHCR. Repository secrets are under 'Settings > Secrets > Actions > New repository secret'.
  • After the container builds correctly, it will be pushed to GHCR, but it will be privately accessible. From the organization profile menu, you can associate the cpp package with the cpp_boilerplate_project repository and change access permissions to 'public'. It's not very straightforward, I had to follow this guide to figure out how to do it.

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.

@ddalcino ddalcino changed the base branch from main to develop April 3, 2022 21:21
Copy link
Contributor

@Jason5480 Jason5480 left a comment

Choose a reason for hiding this comment

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

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

docker/print_versions.sh Show resolved Hide resolved
docker/Dockerfile Outdated Show resolved Hide resolved
# 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
Copy link
Contributor

@Jason5480 Jason5480 Apr 4, 2022

Choose a reason for hiding this comment

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

ADD CONAN_REVISIONS_ENABLED=1. This should also be considered as the default

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

docker/Dockerfile Outdated Show resolved Hide resolved
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
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 && \
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great! I'm sure that will be very useful

ddalcino and others added 2 commits April 4, 2022 03:44
@ddalcino
Copy link
Contributor Author

ddalcino commented Apr 4, 2022

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

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 : character common between bits of versioning info? I was thinking of something more like FROM ghcr.io/cpp_best_practices/cpp-${VARIANT}-gcc${GCC_VER}-llvm${LLVM_VER}:0.1.0, and adding aliases for some tags, like cpp:latest, for the latest versions of everything.

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
Copy link
Contributor

@Jason5480 Jason5480 Apr 4, 2022

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

@Jason5480
Copy link
Contributor

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

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 : character common between bits of versioning info? I was thinking of something more like FROM ghcr.io/cpp_best_practices/cpp-${VARIANT}-gcc${GCC_VER}-llvm${LLVM_VER}:0.1.0, and adding aliases for some tags, like cpp:latest, for the latest versions of everything.

Oh, and thanks for the feedback! It's very useful.

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.

@ddalcino
Copy link
Contributor Author

ddalcino commented Apr 5, 2022

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 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 that the ${USE_CLANG} is not needed any more since we use presets, and we pass the desired compiler through CMake.

I think it definitely makes sense to remove USE_CLANG from the parent docker/Dockerfile. It might be worthwhile to add an example docker/example.dockerfile that extends the parent file, and adds USE_CLANG back in. I will leave that change for another PR though.

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.

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 include-what-you-use. Personally, I like the fact that we are maintaining a container that installs tools in a completely different way than setup-cpp: if something goes wrong with one of them, we can easily fall back on the other.

with:
file: ./docker/Dockerfile
context: ./docker
push: ${{ github.ref == 'refs/heads/main' }}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This check is insufficient. What we need here is to check that:

  1. we are on the main branch
  2. 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.1 when we have forgotten to bump the version to 0.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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

@Jason5480
Copy link
Contributor

What is pushing this PR back?

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

Successfully merging this pull request may close these issues.

publish docker image
3 participants