[email protected] 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 for tackling this Hector! > > Pinning the various dependencies would definitely be useful; we > have been bitten several times by various packages suddenly getting > updated under a seemingly unchanged setup script. > To ensure compatibility with the widest possible range of build > platforms it would seem useful to pin the OS dependencies at > suitable less recent release: probably not all Impala users run the > latest and greatest release of their respective platforms at all > times. > > It is not just the OS platform (the FROM clause of the Dockerfiles) > that may warrant pinning, the same approach seems useful for the > installed components as well, e.g. by pinning repo descriptors to > the repos corresponding to the platform version. This could ensure > that an older platform would not suddenly be updated with packages > from a much more recent release (unless explicitly > allowed/requested by a change that can be identified later from the > git history). Pinned OS versions. 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. Done 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? Done 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 Done 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 Done 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 ldconf Done 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? Done 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 Done 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? Done 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? Done 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? Done 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? The answer is most likely yes with some changes to the build script. This is centos5, so I'll drop this based on your other review. 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 Done 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 t Done http://gerrit.cloudera.org:8080/#/c/12285/1/in-docker.sh@49 PS1, Line 49: TARGET_DIR=/mnt > Is this useful to be configured? I don't see a use case for it. I think we mostly care about the SOURCE dir (which can be configured via BUILD_DIR) but I personally don't care where it ends up in the container. 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? Done 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 Done -- 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: [email protected] <[email protected]> Gerrit-Reviewer: Lars Volker <[email protected]> Gerrit-Reviewer: Laszlo Gaal <[email protected]> Gerrit-Reviewer: Philip Zeyliger <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Reviewer: [email protected] <[email protected]> Gerrit-Comment-Date: Wed, 30 Jan 2019 05:49:47 +0000 Gerrit-HasComments: Yes
