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 2: (15 comments) Thanks! When you say we're pinning the OS, does this actually pin the packages? Does it look like the libc (or whatever) we're getting from pinning to 6.6 actually move? Unfortunately, I bet it does, so we have to be careful about how to use it. That's the underlying reason we need to be able to build the toolchain against explicit images. http://gerrit.cloudera.org:8080/#/c/12285/2/Makefile File Makefile: http://gerrit.cloudera.org:8080/#/c/12285/2/Makefile@2 PS2, Line 2: STAMP_DIR=$(BUILD_DIR)/stamp copyright, etc. http://gerrit.cloudera.org:8080/#/c/12285/2/Makefile@15 PS2, Line 15: ./in-docker.py $(IN_DOCKER_ARGS) $(@F) -- ./buildall.sh |sed -s 's/^/$(@F): /' Does that sed lose error codes because of pipefail-equivalent issues? (My quick testing suggests that it does.) http://gerrit.cloudera.org:8080/#/c/12285/2/docker/all/assert-dependencies-present.py File docker/all/assert-dependencies-present.py: http://gerrit.cloudera.org:8080/#/c/12285/2/docker/all/assert-dependencies-present.py@17 PS2, Line 17: # native toolchain. Must be python2.6 compatible. Worth a note about why it must be 2.6 compatible. http://gerrit.cloudera.org:8080/#/c/12285/2/docker/all/assert-dependencies-present.py@25 PS2, Line 25: def run(cmd): I think you can call this check_output() since it's essentailly a re-implementation of subprocess.check_output() if we're on 2.6, which doesn't have it, if I remember correctly... http://gerrit.cloudera.org:8080/#/c/12285/2/docker/all/assert-dependencies-present.py@34 PS2, Line 34: for s in l: : if re.match(regex, s): : return True : return False I don't know if it's better, but this is basically any and map. >>> import re >>> r = re.compile('a') >>> any(map(r.match, ["a", "b"])) True >>> any(map(r.match, ["c", "b"])) False http://gerrit.cloudera.org:8080/#/c/12285/2/docker/all/assert-dependencies-present.py@41 PS2, Line 41: patterns = ['libdb.*.so', : 'libkrb.*.so', : 'libncurses.so', : 'libsasl.*.so', : 'libcrypto.so', : 'libz.so'] There are dots here that are wildcards and dots here that are real dots. Should we be correct about it? http://gerrit.cloudera.org:8080/#/c/12285/2/docker/all/assert-dependencies-present.py@47 PS2, Line 47: out = run(['ldconfig', '-p'])[0] : libraries = [] : for line in out.splitlines(): : libraries.append(line.split()[0]) I'm ambivalent, but: libraries = [ line.split()[0] for line in check_output(["ldconfig", "-p"]).splitlines() ] http://gerrit.cloudera.org:8080/#/c/12285/2/docker/all/assert-dependencies-present.py@77 PS2, Line 77: print 'Checking program: %s' % p use logging? http://gerrit.cloudera.org:8080/#/c/12285/2/docker/all/assert-dependencies-present.py@79 PS2, Line 79: sys.exit('Unable to find %s in PATH' % p) Use exceptions (and line 54)? http://gerrit.cloudera.org:8080/#/c/12285/2/docker/all/assert-dependencies-present.py@85 PS2, Line 85: import distutils.core # noqa: F401 Maybe just move this to top of the file and python will fail and it'll be clear enough? http://gerrit.cloudera.org:8080/#/c/12285/2/in-docker.py File in-docker.py: http://gerrit.cloudera.org:8080/#/c/12285/2/in-docker.py@42 PS2, Line 42: in-docker.py --docker-args "-t" impala-toolchain-centos6 -- bash Don't you need the "-i"? http://gerrit.cloudera.org:8080/#/c/12285/2/in-docker.py@70 PS2, Line 70: '-v', '/etc/passwd:/etc/passwd:ro', : '-v', '/etc/group:/etc/group:ro', We've maybe seen cases where this doesn't work, but I'm fine with it if it works. http://gerrit.cloudera.org:8080/#/c/12285/2/in-docker.py@77 PS2, Line 77: parser.add_argument('--docker-args', default='') Add help? What's the quoting convention here? http://gerrit.cloudera.org:8080/#/c/12285/2/in-docker.py@125 PS2, Line 125: xargs = subprocess.Popen(['xargs', '-i', 'cp', '--parents', '{}', distro_build_dir], stdin=git.stdout) I had to look up -i and it looks like maybe it's deprecated? This option is a synonym for -Ireplace-str if replace-str is specified. If the replace-str argument is missing, the effect is the same as -I{}. This option is deprecated; use -I instead. http://gerrit.cloudera.org:8080/#/c/12285/2/in-docker.py@147 PS2, Line 147: sys.stderr.write('Running: %s\n' % ' '.join(cmd)) use logging? -- 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: 2 Gerrit-Owner: hector.aco...@cloudera.com <hector.aco...@cloudera.com> Gerrit-Reviewer: Lars Volker <l...@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-Reviewer: hector.aco...@cloudera.com <hector.aco...@cloudera.com> Gerrit-Comment-Date: Wed, 30 Jan 2019 19:46:55 +0000 Gerrit-HasComments: Yes
