[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 2: (15 comments) 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. Done 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 q I set the shell to /bin/bash -o pipefail and I did get this to fail after I messed up a rebase. % cat Makefile;make SHELL=/bin/bash -o pipefail all: exit 99 | true exit 99 | true make: *** [Makefile:4: all] Error 99 I found it hard to figure out what failed without that sed, but I'm fine removing it too. 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. Done 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-implem Done 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. Done 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. Sh Done 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: Done 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? Done 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)? Done 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 c Done 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"? We include that by default. 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 This seems to work reliably for now, I'll include additional fixes if needed. http://gerrit.cloudera.org:8080/#/c/12285/2/in-docker.py@77 PS2, Line 77: parser.add_argument('--docker-args', default='') > Add help? Done 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? Done 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? 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: 2 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 22:38:05 +0000 Gerrit-HasComments: Yes
