Philip Zeyliger has posted comments on this change. ( http://gerrit.cloudera.org:8080/12285 )
Change subject: Initial support for building the toolchain in docker ...................................................................... Patch Set 1: (16 comments) Thanks! This is a huge improvement on the Jenkins-only version of this. in-docker.sh is borderline worthy of re-writing in Python, though it's a bit hard to tell whether or not it'll get shorter. I'll leave that up to you. I think we should try to separate the docker image building versus the building a little bit more than you have. Historically, we've pinned our AMI's, so the build is fairly reproducible. As you've got it here, a change to RedHat's libc will tease its way into our stuff, and maybe we want that to be explicit. I don't think that's a big deal: it's a matter of defining a default place to pull the images from... http://gerrit.cloudera.org:8080/#/c/12285/1/buildall-docker.sh File buildall-docker.sh: http://gerrit.cloudera.org:8080/#/c/12285/1/buildall-docker.sh@2 PS1, Line 2: # Copyright 2017 Cloudera Inc. 2019. http://gerrit.cloudera.org:8080/#/c/12285/1/buildall-docker.sh@23 PS1, Line 23: for image in $(cd docker; ./buildall.sh); do Perhaps add a TODO for parallelism? http://gerrit.cloudera.org:8080/#/c/12285/1/docker/all/assert.sh File docker/all/assert.sh: http://gerrit.cloudera.org:8080/#/c/12285/1/docker/all/assert.sh@2 PS1, Line 2: # Copyright 2015 Cloudera Inc. 2019 http://gerrit.cloudera.org:8080/#/c/12285/1/docker/all/assert.sh@16 PS1, Line 16: # Assert that a system has the correct libraries/binaries to build the Perhaps rename assert to something more friendly? Like "assert-dependencies-present.sh"? http://gerrit.cloudera.org:8080/#/c/12285/1/docker/all/assert.sh@19 PS1, Line 19: set -u : set -e : set -o pipefail It's temping to write this in python from the get-go. The parsing of ldconfig -p in particular is a bit tedious in shell. http://gerrit.cloudera.org:8080/#/c/12285/1/docker/all/assert.sh@35 PS1, Line 35: rm -f /tmp/lib You seem like you use a temp dir on line 23; what's this about? http://gerrit.cloudera.org:8080/#/c/12285/1/docker/all/postinstall.sh File docker/all/postinstall.sh: http://gerrit.cloudera.org:8080/#/c/12285/1/docker/all/postinstall.sh@2 PS1, Line 2: # Copyright 2015 Cloudera Inc. 2019 http://gerrit.cloudera.org:8080/#/c/12285/1/docker/all/postinstall.sh@18 PS1, Line 18: getent hosts github.com| xargs -n1 ssh-keyscan -t rsa,dsa >> /etc/ssh/ssh_known_hosts Comment what this does? http://gerrit.cloudera.org:8080/#/c/12285/1/docker/centos/yum-install File docker/centos/yum-install: http://gerrit.cloudera.org:8080/#/c/12285/1/docker/centos/yum-install@23 PS1, Line 23: yum install $@ 2>&1 | tee $t Does this preserve yum failures, given that you don't have pipefail set? Should this be "$@" rather than $@? http://gerrit.cloudera.org:8080/#/c/12285/1/docker/centos/yum-install@25 PS1, Line 25: >&2 echo "yum-install: One or more packages not available but specified as arguments" avoid tabs? http://gerrit.cloudera.org:8080/#/c/12285/1/docker/centos5.df File docker/centos5.df: http://gerrit.cloudera.org:8080/#/c/12285/1/docker/centos5.df@47 PS1, Line 47: # XXX: Thrift 0.9.3 requires python2.6, and uses system python. ??? Can we manipulate this via PATH? http://gerrit.cloudera.org:8080/#/c/12285/1/docker/centos5/epel/epel-release-5-4.noarch.rpm File docker/centos5/epel/epel-release-5-4.noarch.rpm: http://gerrit.cloudera.org:8080/#/c/12285/1/docker/centos5/epel/epel-release-5-4.noarch.rpm@a1 PS1, Line 1: I always worry about binary files in git repos... Can we do this in a more text-y way? (perhaps by just editing yum.repos.d files? http://gerrit.cloudera.org:8080/#/c/12285/1/in-docker.sh File in-docker.sh: http://gerrit.cloudera.org:8080/#/c/12285/1/in-docker.sh@16 PS1, Line 16: # Script that sets up environment used in order to perform full (or partial) Can you document how the file system mounting is set up here? i.e., where the output is. http://gerrit.cloudera.org:8080/#/c/12285/1/in-docker.sh@49 PS1, Line 49: TARGET_DIR=/mnt Is this useful to be configured? http://gerrit.cloudera.org:8080/#/c/12285/1/in-docker.sh@66 PS1, Line 66: while [[ $1 ]]; do Can you do this with "-z" and avoid the "set +u" stuff? http://gerrit.cloudera.org:8080/#/c/12285/1/in-docker.sh@87 PS1, Line 87: # GCC_VERSION works here Comment that this looks for subdirectories of source (like source/foo) and produces variables FOO_VERSION for them. You don't really need the basename here. You can do something like: find . -mindepth 1 -maxdepth 1 -type d -printf "%f\n" The %f gives you the last component of the name. -- To view, visit http://gerrit.cloudera.org:8080/12285 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: native-toolchain Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If42c9bc06a3d303642eb37dea784b61e2a1f5cc6 Gerrit-Change-Number: 12285 Gerrit-PatchSet: 1 Gerrit-Owner: hector.aco...@cloudera.com <hector.aco...@cloudera.com> Gerrit-Reviewer: Laszlo Gaal <laszlo.g...@cloudera.com> Gerrit-Reviewer: Philip Zeyliger <phi...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Mon, 28 Jan 2019 22:27:31 +0000 Gerrit-HasComments: Yes
