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

updates to tools/docker-format/Dockerfile : container builds again #449

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

alichnewsky
Copy link

@alichnewsky alichnewsky commented Jun 20, 2020

current docker file fails to build. Should be linked to issue #448
As a result, can't run pre-commit formatting before PR fail in CI builds.

to validate rebuild the container as per instructions in the README.md file.

@@ -1,8 +1,28 @@
FROM ubuntu:cosmic

# repositories are obsolete. the image need to be changed.
Copy link
Contributor

Choose a reason for hiding this comment

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

If we update to something newer than cosmic, do we also get golang>=1.12?

Copy link
Author

Choose a reason for hiding this comment

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

Upgrading from cosmic to focal ( 20.04 LTS ) gives you exactly golang 1.13.
It requires python3 but the same cmake-format package version works.
The default clang-format is 10.0 and needs to be downgraded not to have side effects on the current c++ code.

On other hand, installing golang manually makes the upgrade path clear should buildifier requirements change again.

currently in this PR

docker run -v $PWD:/opencensus-cpp -it opencensus-cpp/format:cosmic /opencensus-cpp/tools/docker-format/run.sh
clang-format version 7.0.0-3 (tags/RELEASE_700/final)
Running buildifier.
Running cmake-format 0.6.10.

potential "focal" version

docker run -v $PWD:/opencensus-cpp -it opencensus-cpp/format:focal /opencensus-cpp/tools/docker-format/run.sh

clang-format version 7.0.1-12 (tags/RELEASE_701/final)
Running buildifier.
Running cmake-format 0.6.10.

I can submit the latter if you'd rather.

Copy link
Contributor

Choose a reason for hiding this comment

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

  1. Is the "focal" version simpler?

  2. Instead of installing golang in "cosmic" is it easier to just install a release of buildifier? e.g. https://github.com/bazelbuild/buildtools/releases/download/3.3.0/buildifier

# brings golang to 1.10 by default.bazelbuilds wants 1.12 minimum....
#RUN apt install -y golang

RUN apt install -y curl
Copy link
Contributor

Choose a reason for hiding this comment

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

Please merge all of the RUNs together into a single one to avoid a long chain of docker overlays.

Copy link

@lgruen lgruen left a comment

Choose a reason for hiding this comment

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

Sorry, I'm not working on OpenCensus anymore, but can't remove myself as a reviewer from this PR. Dismissing this review using "request changes" might remove it from my PR queue though.

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.

3 participants