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

Adding a second Dockerfile supporting C++20 for cmake C++20 tests #211

Closed
wants to merge 10 commits into from

Conversation

dabangarang
Copy link

This pull request is a response to issue #148.
I added in support to run cmake.c++20.test within a new docker container that is based off Ubuntu 20.04 and supports C++20 and g++-10. This docker container will automatically be built and run when it is specified that cmake.c++20.test is to be run. Other tests will be run on the standard Docker container, which runs on Ubuntu 18.04, as C++ 20 is not needed for these tests.

@dabangarang dabangarang requested a review from a team July 24, 2020 23:49
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Jul 24, 2020

CLA Check
The committers are authorized under a signed CLA.

@codecov
Copy link

codecov bot commented Jul 24, 2020

Codecov Report

Merging #211 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #211   +/-   ##
=======================================
  Coverage   93.94%   93.94%           
=======================================
  Files         146      146           
  Lines        6538     6538           
=======================================
  Hits         6142     6142           
  Misses        396      396           

Copy link
Member

@lalitb lalitb left a comment

Choose a reason for hiding this comment

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

Just wondering, it could have done in single Dockerfile - i..e, installing both gcc-48 and gcc-10 version in ubuntu18 docker container. This can be done by creating ci/install-gcc10.sh with below contents

apt-get -y install software-properties-common
add-apt-repository -y ppa:ubuntu-toolchain-r/test
apt-get -y update
apt install -y gcc-10
apt install -y g++-10

And then while running cmake on tests, set CC/CXX to point to correct compiler.

@dabangarang
Copy link
Author

@lalitb apologies for the accidental early commit. I had revised my pull request to implement your suggestion, and the current commit should reflect this. Where/what parameters would I need to change in the do_ci.sh file to identify a different compiler for Cmake20? It seems as though simply changing c++ vesrion to 20 does not change GNU compiler to a compatible version.

@maxgolov
Copy link
Contributor

maxgolov commented Aug 7, 2020

You don't necessarily need to build a custom docker image. As Ubuntu-20.04 is readily available on GitHub Actions.

You can use GitHub Actions matrix build feature to build with older and newer Ubuntu as follows:

https://raw.githubusercontent.com/maxgolov/opentelemetry-cpp/master/.github/workflows/build-ubuntu.yml

@@ -2,13 +2,13 @@

set -e

BUILD_IMAGE=opentelemetry-cpp-build
BUILD_IMAGE=opentelemetry-cpp-build-test

docker image inspect "$BUILD_IMAGE" &> /dev/null || {
docker build -t "$BUILD_IMAGE" ci
Copy link
Contributor

Choose a reason for hiding this comment

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

It's probably adding considerable amount of CI time to build an image. Have you considered running the ci scripts without a docker, to deploy the tools in one of the readily available GitHub Action images?
https://raw.githubusercontent.com/maxgolov/opentelemetry-cpp/master/.github/workflows/build-ubuntu.yml
Main idea is that total compute and run time for each PR is going to be less, since you won't need to build an image.

@reyang
Copy link
Member

reyang commented Aug 17, 2020

@danielbang907, please address comments from @maxgolov and get the CI pass.

@reyang
Copy link
Member

reyang commented Oct 7, 2020

This PR is stale, closing for now. @danielbang907 please feel free to reopen it if you have time to work on it.

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.

4 participants