-
Notifications
You must be signed in to change notification settings - Fork 77
updates to tools/docker-format/Dockerfile : container builds again #449
base: master
Are you sure you want to change the base?
updates to tools/docker-format/Dockerfile : container builds again #449
Conversation
@@ -1,8 +1,28 @@ | |||
FROM ubuntu:cosmic | |||
|
|||
# repositories are obsolete. the image need to be changed. |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-
Is the "focal" version simpler?
-
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 |
There was a problem hiding this comment.
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.
There was a problem hiding this 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.
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.