From f607ea3a9e5b4c1424e20f86299c01326d4f0b0e Mon Sep 17 00:00:00 2001 From: Daniel McClanahan <1305167+cosmicexplorer@users.noreply.github.com> Date: Fri, 8 Jun 2018 00:11:42 -0700 Subject: [PATCH 01/16] vary the xz shared lib filename depending on the current platform --- src/python/pants/binaries/binary_tool.py | 5 ++-- src/python/pants/fs/archive.py | 32 +++++++++++++++++------- 2 files changed, 25 insertions(+), 12 deletions(-) diff --git a/src/python/pants/binaries/binary_tool.py b/src/python/pants/binaries/binary_tool.py index 2f50edafd53..96b63590081 100644 --- a/src/python/pants/binaries/binary_tool.py +++ b/src/python/pants/binaries/binary_tool.py @@ -52,9 +52,8 @@ class BinaryToolBase(Subsystem): def subsystem_dependencies(cls): sub_deps = super(BinaryToolBase, cls).subsystem_dependencies() + (BinaryUtil.Factory,) - # TODO(cosmicexplorer): if we need to do more conditional subsystem dependencies, do it - # declaratively with a dict class field so that we only try to create or access it if we - # declared a dependency on it. + # TODO: if we need to do more conditional subsystem dependencies, do it declaratively with a + # dict class field so that we only try to create or access it if we declared a dependency on it. if cls.archive_type == 'txz': sub_deps = sub_deps + (XZ.scoped(cls),) diff --git a/src/python/pants/fs/archive.py b/src/python/pants/fs/archive.py index 09ab21c9908..14e3cf9741b 100644 --- a/src/python/pants/fs/archive.py +++ b/src/python/pants/fs/archive.py @@ -15,7 +15,9 @@ from pants.util.contextutil import open_tar, open_zip, temporary_dir from pants.util.dirutil import (is_executable, safe_concurrent_rename, safe_walk, split_basename_and_dirname) +from pants.util.memo import memoized_classproperty from pants.util.meta import AbstractClass +from pants.util.osutil import get_normalized_os_name from pants.util.process_handler import subprocess from pants.util.strutil import ensure_text @@ -103,9 +105,21 @@ class XZCompressedTarArchiver(TarArchiver): class XZArchiverError(Exception): pass + # FIXME: move Platform#resolve_platform_specific() into somewhere common and consume it here after + # #5815 is merged. + @memoized_classproperty + def _liblzma_shared_lib_filename(cls): + normalized_os_name = get_normalized_os_name() + if normalized_os_name == 'darwin': + return 'liblzma.dylib' + elif normalized_os_name == 'linux': + return 'liblzma.so' + else: + raise cls.XZArchiverError("Unrecognized platform: {}.".format(normalized_os_name)) + def __init__(self, xz_binary_path, xz_library_path): - # TODO(cosmicexplorer): test these exceptions somewhere! + # TODO: test these exceptions somewhere! if not is_executable(xz_binary_path): raise self.XZArchiverError( "The path {} does not name an existing executable file. An xz executable must be provided " @@ -116,15 +130,15 @@ def __init__(self, xz_binary_path, xz_library_path): if not os.path.isdir(xz_library_path): raise self.XZArchiverError( - "The path {} does not name an existing directory. A directory containing liblzma.so must " + "The path {} does not name an existing directory. A directory containing liblzma.{so,dylib} must " "be provided to decompress xz archives." .format(xz_library_path)) - lib_lzma_dylib = os.path.join(xz_library_path, 'liblzma.so') + lib_lzma_dylib = os.path.join(xz_library_path, self._liblzma_shared_lib_filename) if not os.path.isfile(lib_lzma_dylib): raise self.XZArchiverError( - "The path {} names an existing directory, but it does not contain liblzma.so. A directory " - "containing liblzma.so must be provided to decompress xz archives." + "The path {} names an existing directory, but it does not contain liblzma.{so,dylib}. A directory " + "containing liblzma.{so,dylib} must be provided to decompress xz archives." .format(xz_library_path)) self._xz_library_path = xz_library_path @@ -140,14 +154,14 @@ def _invoke_xz(self, xz_input_file): """ (xz_bin_dir, xz_filename) = split_basename_and_dirname(self._xz_binary_path) - # TODO(cosmicexplorer): --threads=0 is supposed to use "the number of processor cores on the - # machine", but I see no more than 100% cpu used at any point. This seems like it could be a - # bug? If performance is an issue, investigate further. + # FIXME: --threads=0 is supposed to use "the number of processor cores on the machine", but I + # see no more than 100% cpu used at any point. This seems like it could be a bug? If performance + # is an issue, investigate further. cmd = [xz_filename, '--decompress', '--stdout', '--keep', '--threads=0', xz_input_file] env = { # Isolate the path so we know we're using our provided version of xz. 'PATH': xz_bin_dir, - # Only allow our xz's lib directory to resolve the liblzma.so dependency at runtime. + # Only allow our xz's lib directory to resolve the liblzma.{so,dylib} dependency at runtime. 'LD_LIBRARY_PATH': self._xz_library_path, } try: From 3b4e07c99cd82a1b173ccaf05eecc6032713af12 Mon Sep 17 00:00:00 2001 From: Daniel McClanahan <1305167+cosmicexplorer@users.noreply.github.com> Date: Fri, 8 Jun 2018 00:16:58 -0700 Subject: [PATCH 02/16] use the correct key 'mac' instead of 'darwin' into the platform --- src/python/pants/backend/native/subsystems/llvm.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/python/pants/backend/native/subsystems/llvm.py b/src/python/pants/backend/native/subsystems/llvm.py index 43c3c757a80..ec4933fc9f1 100644 --- a/src/python/pants/backend/native/subsystems/llvm.py +++ b/src/python/pants/backend/native/subsystems/llvm.py @@ -18,10 +18,10 @@ class LLVMReleaseUrlGenerator(BinaryToolUrlGenerator): _ARCHIVE_BASE_FMT = 'clang+llvm-{version}-x86_64-{system_id}' - # TODO(cosmicexplorer): Give a more useful error message than KeyError if the host platform was - # not recognized (and make it easy for other BinaryTool subclasses to do this as well). + # TODO: Give a more useful error message than KeyError if the host platform was not recognized + # (and make it easy for other BinaryTool subclasses to do this as well). _SYSTEM_ID = { - 'darwin': 'apple-darwin', + 'mac': 'apple-darwin', 'linux': 'linux-gnu-ubuntu-16.04', } From 7a9f39722e79a23b6660c18f50802efdbb652d1b Mon Sep 17 00:00:00 2001 From: Daniel McClanahan <1305167+cosmicexplorer@users.noreply.github.com> Date: Fri, 8 Jun 2018 01:25:34 -0700 Subject: [PATCH 03/16] add `platform_specific_behavior` tag to python native code tests and run those tests in ci --- .travis.yml | 4 +- build-support/bin/ci.sh | 75 +++++++++++-------- .../pants_test/backend/python/tasks/BUILD | 22 ++++++ 3 files changed, 68 insertions(+), 33 deletions(-) diff --git a/.travis.yml b/.travis.yml index f88c28d137f..3f1c430d93f 100644 --- a/.travis.yml +++ b/.travis.yml @@ -430,14 +430,14 @@ matrix: osx_image: xcode8.3 language: generic env: - - SHARD="Rust Tests OSX" + - SHARD="Rust + Platform-specific Tests OSX" before_install: - brew tap caskroom/cask && brew update && brew cask install osxfuse before_script: - ulimit -c unlimited - ulimit -n 8192 script: - - ./build-support/bin/ci.sh -bcfjklmnprtx + - ./build-support/bin/ci.sh -bcfjklmnprtxz deploy: # See: https://docs.travis-ci.com/user/deployment/s3/ diff --git a/build-support/bin/ci.sh b/build-support/bin/ci.sh index 77c313b99cf..9909f5da3a1 100755 --- a/build-support/bin/ci.sh +++ b/build-support/bin/ci.sh @@ -10,36 +10,40 @@ cd ${REPO_ROOT} source build-support/common.sh function usage() { - echo "Runs commons tests for local or hosted CI." - echo - echo "Usage: $0 (-h|-fxbkmsrjlpuyncia)" - echo " -h print out this help message" - echo " -f skip python code formatting checks" - echo " -x skip bootstrap clean-all (assume bootstrapping from a" - echo " fresh clone)" - echo " -b skip bootstraping pants from local sources" - echo " -k skip bootstrapped pants self compile check" - echo " -m skip sanity checks of bootstrapped pants and repo BUILD" - echo " files" - echo " -r skip doc generation tests" - echo " -j skip core jvm tests" - echo " -l skip internal backends python tests" - echo " -p skip core python tests" - echo " -u SHARD_NUMBER/TOTAL_SHARDS" - echo " if running core python tests, divide them into" - echo " TOTAL_SHARDS shards and just run those in SHARD_NUMBER" - echo " to run only even tests: '-u 0/2', odd: '-u 1/2'" - echo " -n skip contrib python tests" - echo " -e skip rust tests" - echo " -y SHARD_NUMBER/TOTAL_SHARDS" - echo " if running contrib python tests, divide them into" - echo " TOTAL_SHARDS shards and just run those in SHARD_NUMBER" - echo " to run only even tests: '-u 0/2', odd: '-u 1/2'" - echo " -c skip pants integration tests (includes examples and testprojects)" - echo " -i SHARD_NUMBER/TOTAL_SHARDS" - echo " if running integration tests, divide them into" - echo " TOTAL_SHARDS shards and just run those in SHARD_NUMBER" - echo " to run only even tests: '-i 0/2', odd: '-i 1/2'" + cat < 0 )); then die "$@" else @@ -57,7 +61,7 @@ python_unit_shard="0/1" python_contrib_shard="0/1" python_intg_shard="0/1" -while getopts "hfxbkmsrjlpeu:ny:ci:at" opt; do +while getopts "hfxbkmrjlpeu:ny:ci:tz" opt; do case ${opt} in h) usage ;; f) skip_pre_commit_checks="true" ;; @@ -76,6 +80,7 @@ while getopts "hfxbkmsrjlpeu:ny:ci:at" opt; do c) skip_integration="true" ;; i) python_intg_shard=${OPTARG} ;; t) skip_lint="true" ;; + z) test_platform_specific_behavior="true" ;; *) usage "Invalid option: -${OPTARG}" ;; esac done @@ -224,6 +229,14 @@ if [[ "${skip_rust_tests:-false}" == "false" ]]; then end_travis_section fi +if [[ "${test_platform_specific_behavior:-false}" == 'true' ]]; then + start_travis_section "Platform-specific tests" "Running platform-specific testing on platform: $(uname)" + ( + ./pants.pex ${PANTS_ARGS[@]} --tag='+platform_specific_behavior' test tests:: + ) || die "Pants platform-specific test failure" + end_travis_section +fi + if [[ "${skip_integration:-false}" == "false" ]]; then if [[ "0/1" != "${python_intg_shard}" ]]; then diff --git a/tests/python/pants_test/backend/python/tasks/BUILD b/tests/python/pants_test/backend/python/tasks/BUILD index 2c5aaabc171..27ef0999cca 100644 --- a/tests/python/pants_test/backend/python/tasks/BUILD +++ b/tests/python/pants_test/backend/python/tasks/BUILD @@ -17,6 +17,28 @@ python_library( ] ) +python_tests( + name='python_native_code_testing', + sources=[ + # TODO: This test does not have any platform-specific testing right now, but that will be + # changed when #5815 is merged. + 'test_build_local_python_distributions.py', + 'test_python_distribution_integration.py', + ], + dependencies=[ + ':python_task_test_base', + 'src/python/pants/backend/native', + 'src/python/pants/backend/python:plugin', + 'src/python/pants/backend/python/targets', + 'src/python/pants/backend/python/tasks', + 'src/python/pants/util:contextutil', + 'src/python/pants/util:process_handler', + 'tests/python/pants_test:int-test', + 'tests/python/pants_test/engine:scheduler_test_base', + ], + tags={'platform_specific_behavior'}, +) + python_tests( sources=globs('test_*.py', exclude=[globs('*_integration.py')]), dependencies=[ From b34dbb103130c7def1e6869e7116745ee513dc9c Mon Sep 17 00:00:00 2001 From: Daniel McClanahan <1305167+cosmicexplorer@users.noreply.github.com> Date: Fri, 8 Jun 2018 13:35:52 -0700 Subject: [PATCH 04/16] use self._liblzma_shared_lib_filename to make xz error messages more specific --- src/python/pants/fs/archive.py | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/src/python/pants/fs/archive.py b/src/python/pants/fs/archive.py index 14e3cf9741b..02d40b26479 100644 --- a/src/python/pants/fs/archive.py +++ b/src/python/pants/fs/archive.py @@ -130,16 +130,18 @@ def __init__(self, xz_binary_path, xz_library_path): if not os.path.isdir(xz_library_path): raise self.XZArchiverError( - "The path {} does not name an existing directory. A directory containing liblzma.{so,dylib} must " - "be provided to decompress xz archives." - .format(xz_library_path)) + "The path {lib_dir} does not name an existing directory. A directory containing " + "{dylib_filename} must be provided to decompress xz archives." + .format(lib_dir=xz_library_path, + dylib_filename=self._liblzma_shared_lib_filename)) lib_lzma_dylib = os.path.join(xz_library_path, self._liblzma_shared_lib_filename) if not os.path.isfile(lib_lzma_dylib): raise self.XZArchiverError( - "The path {} names an existing directory, but it does not contain liblzma.{so,dylib}. A directory " - "containing liblzma.{so,dylib} must be provided to decompress xz archives." - .format(xz_library_path)) + "The path {lib_dir} names an existing directory, but it does not contain {dylib_filename}. " + "A directory containing {dylib_filename} must be provided to decompress xz archives." + .format(lib_dir=xz_library_path, + dylib_filename=self._liblzma_shared_lib_filename)) self._xz_library_path = xz_library_path From 633a2b6bca6fd3f12c27185577e723cdf627477e Mon Sep 17 00:00:00 2001 From: Daniel McClanahan <1305167+cosmicexplorer@users.noreply.github.com> Date: Fri, 8 Jun 2018 13:37:02 -0700 Subject: [PATCH 05/16] don't re-run platform-specific test sources twice in linux --- .../python/pants_test/backend/python/tasks/BUILD | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/tests/python/pants_test/backend/python/tasks/BUILD b/tests/python/pants_test/backend/python/tasks/BUILD index 27ef0999cca..92069b42804 100644 --- a/tests/python/pants_test/backend/python/tasks/BUILD +++ b/tests/python/pants_test/backend/python/tasks/BUILD @@ -17,14 +17,16 @@ python_library( ] ) +python_native_code_test_files = [ + # TODO: This test does not have any platform-specific testing right now, but that will be + # changed when #5815 is merged. + 'test_build_local_python_distributions.py', + 'test_python_distribution_integration.py', +] + python_tests( name='python_native_code_testing', - sources=[ - # TODO: This test does not have any platform-specific testing right now, but that will be - # changed when #5815 is merged. - 'test_build_local_python_distributions.py', - 'test_python_distribution_integration.py', - ], + sources=python_native_code_test_files, dependencies=[ ':python_task_test_base', 'src/python/pants/backend/native', @@ -40,7 +42,7 @@ python_tests( ) python_tests( - sources=globs('test_*.py', exclude=[globs('*_integration.py')]), + sources=globs('test_*.py', exclude=([globs('*_integration.py')] + python_native_code_test_files)), dependencies=[ ':python_task_test_base', '3rdparty/python/twitter/commons:twitter.common.collections', From 0b493d551650749d9efaee4fc2ea17e11e7e4326 Mon Sep 17 00:00:00 2001 From: Daniel McClanahan <1305167+cosmicexplorer@users.noreply.github.com> Date: Fri, 8 Jun 2018 13:38:00 -0700 Subject: [PATCH 06/16] add the bootstrap phase to the platform-specific python testing in the new shard --- .travis.yml | 3 ++- build-support/bin/ci.sh | 8 ++++++-- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/.travis.yml b/.travis.yml index 3f1c430d93f..07619197c57 100644 --- a/.travis.yml +++ b/.travis.yml @@ -437,7 +437,8 @@ matrix: - ulimit -c unlimited - ulimit -n 8192 script: - - ./build-support/bin/ci.sh -bcfjklmnprtxz + # Platform-specific tests currently need a pants pex, so we bootstrap here (no -b) and set -z to run the platform-specific tests. + - ./build-support/bin/ci.sh -cfjklmnprtxz deploy: # See: https://docs.travis-ci.com/user/deployment/s3/ diff --git a/build-support/bin/ci.sh b/build-support/bin/ci.sh index 9909f5da3a1..17d23bb009f 100755 --- a/build-support/bin/ci.sh +++ b/build-support/bin/ci.sh @@ -229,10 +229,14 @@ if [[ "${skip_rust_tests:-false}" == "false" ]]; then end_travis_section fi +# NB: this only tests python tests right now -- the command needs to be edited if test targets in +# other languages are tagged with 'platform_specific_behavior' in the future. if [[ "${test_platform_specific_behavior:-false}" == 'true' ]]; then - start_travis_section "Platform-specific tests" "Running platform-specific testing on platform: $(uname)" + start_travis_section "Platform-specific tests" \ + "Running platform-specific testing on platform: $(uname)" ( - ./pants.pex ${PANTS_ARGS[@]} --tag='+platform_specific_behavior' test tests:: + ./pants.pex ${PANTS_ARGS[@]} --tag='+platform_specific_behavior' test \ + tests/python:: -- ${PYTEST_PASSTHRU_ARGS} ) || die "Pants platform-specific test failure" end_travis_section fi From a1617a97ca647e174f68c1787bd9e0bd13bd2e48 Mon Sep 17 00:00:00 2001 From: Daniel McClanahan <1305167+cosmicexplorer@users.noreply.github.com> Date: Fri, 8 Jun 2018 13:41:50 -0700 Subject: [PATCH 07/16] don't re-run platform-specific integration tests either on linux --- tests/python/pants_test/backend/python/tasks/BUILD | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/python/pants_test/backend/python/tasks/BUILD b/tests/python/pants_test/backend/python/tasks/BUILD index 92069b42804..23c42591fad 100644 --- a/tests/python/pants_test/backend/python/tasks/BUILD +++ b/tests/python/pants_test/backend/python/tasks/BUILD @@ -79,7 +79,7 @@ python_tests( python_tests( name='integration', - sources=globs('*_integration.py'), + sources=globs('*_integration.py', exclude=python_native_code_test_files), dependencies=[ '3rdparty/python:pex', 'src/python/pants/util:contextutil', From 4f78bbe1f90820950f347160707498e4ebf3eb27 Mon Sep 17 00:00:00 2001 From: Daniel McClanahan <1305167+cosmicexplorer@users.noreply.github.com> Date: Fri, 8 Jun 2018 13:59:41 -0700 Subject: [PATCH 08/16] increase the timeout on platform-specific testing --- tests/python/pants_test/backend/python/tasks/BUILD | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/python/pants_test/backend/python/tasks/BUILD b/tests/python/pants_test/backend/python/tasks/BUILD index 23c42591fad..ee117750797 100644 --- a/tests/python/pants_test/backend/python/tasks/BUILD +++ b/tests/python/pants_test/backend/python/tasks/BUILD @@ -39,6 +39,7 @@ python_tests( 'tests/python/pants_test/engine:scheduler_test_base', ], tags={'platform_specific_behavior'}, + timeout=2400, ) python_tests( From 34bd7693bf52b480f103223e145a0ac9144f8887 Mon Sep 17 00:00:00 2001 From: Daniel McClanahan <1305167+cosmicexplorer@users.noreply.github.com> Date: Fri, 8 Jun 2018 17:18:08 -0700 Subject: [PATCH 09/16] use the correct platform-specific library search path for the xz exe --- src/python/pants/fs/archive.py | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/src/python/pants/fs/archive.py b/src/python/pants/fs/archive.py index 02d40b26479..f13de7696f9 100644 --- a/src/python/pants/fs/archive.py +++ b/src/python/pants/fs/archive.py @@ -163,9 +163,17 @@ def _invoke_xz(self, xz_input_file): env = { # Isolate the path so we know we're using our provided version of xz. 'PATH': xz_bin_dir, - # Only allow our xz's lib directory to resolve the liblzma.{so,dylib} dependency at runtime. - 'LD_LIBRARY_PATH': self._xz_library_path, } + + # Only allow our xz's lib directory to resolve the liblzma.{so,dylib} dependency at runtime. + normalized_os_name = get_normalized_os_name() + if normalized_os_name == 'darwin': + env['DYLD_FALLBACK_LIBRARY_PATH'] = self._xz_library_path + elif normalized_os_name == 'linux': + env['LD_LIBRARY_PATH'] = self._xz_library_path + else: + raise self.XZArchiverError("Unrecognized platform: {}.".format(normalized_os_name)) + try: # Pipe stderr to our own stderr, but leave stdout open so we can yield it. process = subprocess.Popen( From 62892cae11653ffaf8350d0efccc5f0f85e1240b Mon Sep 17 00:00:00 2001 From: Danny McClanahan <1305167+cosmicexplorer@users.noreply.github.com> Date: Mon, 11 Jun 2018 16:06:23 -0700 Subject: [PATCH 10/16] fix library path handling by using get_joined_path() --- src/python/pants/fs/archive.py | 20 ++++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-) diff --git a/src/python/pants/fs/archive.py b/src/python/pants/fs/archive.py index f13de7696f9..31f989c2f43 100644 --- a/src/python/pants/fs/archive.py +++ b/src/python/pants/fs/archive.py @@ -12,7 +12,7 @@ from contextlib import contextmanager from zipfile import ZIP_DEFLATED -from pants.util.contextutil import open_tar, open_zip, temporary_dir +from pants.util.contextutil import get_joined_path, open_tar, open_zip, temporary_dir from pants.util.dirutil import (is_executable, safe_concurrent_rename, safe_walk, split_basename_and_dirname) from pants.util.memo import memoized_classproperty @@ -105,10 +105,10 @@ class XZCompressedTarArchiver(TarArchiver): class XZArchiverError(Exception): pass - # FIXME: move Platform#resolve_platform_specific() into somewhere common and consume it here after - # #5815 is merged. @memoized_classproperty def _liblzma_shared_lib_filename(cls): + # FIXME: move Platform#resolve_platform_specific() into somewhere common and consume it here after + # #5815 is merged. normalized_os_name = get_normalized_os_name() if normalized_os_name == 'darwin': return 'liblzma.dylib' @@ -165,15 +165,23 @@ def _invoke_xz(self, xz_input_file): 'PATH': xz_bin_dir, } - # Only allow our xz's lib directory to resolve the liblzma.{so,dylib} dependency at runtime. + # Allow our xz's lib directory to resolve the liblzma.{so,dylib} dependency at runtime by + # prepending it to the platform-specific library path environment variable. + # FIXME: move Platform#resolve_platform_specific() into somewhere common and consume it here + # after #5815 is merged. normalized_os_name = get_normalized_os_name() if normalized_os_name == 'darwin': - env['DYLD_FALLBACK_LIBRARY_PATH'] = self._xz_library_path + env_var='DYLD_LIBRARY_PATH' elif normalized_os_name == 'linux': - env['LD_LIBRARY_PATH'] = self._xz_library_path + env_var='LD_LIBRARY_PATH' else: raise self.XZArchiverError("Unrecognized platform: {}.".format(normalized_os_name)) + env[env_var] = get_joined_path([self._xz_library_path], + env=os.environ.copy(), + env_var=env_var, + prepend=True) + try: # Pipe stderr to our own stderr, but leave stdout open so we can yield it. process = subprocess.Popen( From fe240da26e6a319cba04e7fed46c969dc4dcf767 Mon Sep 17 00:00:00 2001 From: Danny McClanahan <1305167+cosmicexplorer@users.noreply.github.com> Date: Mon, 11 Jun 2018 16:34:18 -0700 Subject: [PATCH 11/16] remove library path manipulation thanks to pantsbuild/binaries#71 --- src/python/pants/binaries/binary_tool.py | 5 +- src/python/pants/fs/archive.py | 60 ++---------------------- 2 files changed, 5 insertions(+), 60 deletions(-) diff --git a/src/python/pants/binaries/binary_tool.py b/src/python/pants/binaries/binary_tool.py index 96b63590081..816c22bc4ef 100644 --- a/src/python/pants/binaries/binary_tool.py +++ b/src/python/pants/binaries/binary_tool.py @@ -221,10 +221,7 @@ class XZ(NativeTool): @memoized_property def tar_xz_extractor(self): - return XZCompressedTarArchiver(self._executable_location(), self._lib_dir()) + return XZCompressedTarArchiver(self._executable_location()) def _executable_location(self): return os.path.join(self.select(), 'bin', 'xz') - - def _lib_dir(self): - return os.path.join(self.select(), 'lib') diff --git a/src/python/pants/fs/archive.py b/src/python/pants/fs/archive.py index 31f989c2f43..8ff4a4cf460 100644 --- a/src/python/pants/fs/archive.py +++ b/src/python/pants/fs/archive.py @@ -12,12 +12,10 @@ from contextlib import contextmanager from zipfile import ZIP_DEFLATED -from pants.util.contextutil import get_joined_path, open_tar, open_zip, temporary_dir +from pants.util.contextutil import open_tar, open_zip, temporary_dir from pants.util.dirutil import (is_executable, safe_concurrent_rename, safe_walk, split_basename_and_dirname) -from pants.util.memo import memoized_classproperty from pants.util.meta import AbstractClass -from pants.util.osutil import get_normalized_os_name from pants.util.process_handler import subprocess from pants.util.strutil import ensure_text @@ -98,53 +96,19 @@ class XZCompressedTarArchiver(TarArchiver): Invokes an xz executable to decompress a .tar.xz into a tar stream, which is piped into the extract() method. - - NB: This class will raise an error if used to create an archive! This class can only currently be - used to extract from xz archives. """ class XZArchiverError(Exception): pass - @memoized_classproperty - def _liblzma_shared_lib_filename(cls): - # FIXME: move Platform#resolve_platform_specific() into somewhere common and consume it here after - # #5815 is merged. - normalized_os_name = get_normalized_os_name() - if normalized_os_name == 'darwin': - return 'liblzma.dylib' - elif normalized_os_name == 'linux': - return 'liblzma.so' - else: - raise cls.XZArchiverError("Unrecognized platform: {}.".format(normalized_os_name)) - - def __init__(self, xz_binary_path, xz_library_path): + def __init__(self, xz_binary_path): - # TODO: test these exceptions somewhere! + # TODO: test this exception somewhere! if not is_executable(xz_binary_path): raise self.XZArchiverError( "The path {} does not name an existing executable file. An xz executable must be provided " "to decompress xz archives." .format(xz_binary_path)) - self._xz_binary_path = xz_binary_path - - if not os.path.isdir(xz_library_path): - raise self.XZArchiverError( - "The path {lib_dir} does not name an existing directory. A directory containing " - "{dylib_filename} must be provided to decompress xz archives." - .format(lib_dir=xz_library_path, - dylib_filename=self._liblzma_shared_lib_filename)) - - lib_lzma_dylib = os.path.join(xz_library_path, self._liblzma_shared_lib_filename) - if not os.path.isfile(lib_lzma_dylib): - raise self.XZArchiverError( - "The path {lib_dir} names an existing directory, but it does not contain {dylib_filename}. " - "A directory containing {dylib_filename} must be provided to decompress xz archives." - .format(lib_dir=xz_library_path, - dylib_filename=self._liblzma_shared_lib_filename)) - - self._xz_library_path = xz_library_path - super(XZCompressedTarArchiver, self).__init__('r|', 'tar.xz') @contextmanager @@ -165,23 +129,6 @@ def _invoke_xz(self, xz_input_file): 'PATH': xz_bin_dir, } - # Allow our xz's lib directory to resolve the liblzma.{so,dylib} dependency at runtime by - # prepending it to the platform-specific library path environment variable. - # FIXME: move Platform#resolve_platform_specific() into somewhere common and consume it here - # after #5815 is merged. - normalized_os_name = get_normalized_os_name() - if normalized_os_name == 'darwin': - env_var='DYLD_LIBRARY_PATH' - elif normalized_os_name == 'linux': - env_var='LD_LIBRARY_PATH' - else: - raise self.XZArchiverError("Unrecognized platform: {}.".format(normalized_os_name)) - - env[env_var] = get_joined_path([self._xz_library_path], - env=os.environ.copy(), - env_var=env_var, - prepend=True) - try: # Pipe stderr to our own stderr, but leave stdout open so we can yield it. process = subprocess.Popen( @@ -210,6 +157,7 @@ def _extract(self, path, outdir): return super(XZCompressedTarArchiver, self)._extract( xz_decompressed_tar_stream, outdir, mode=self.mode) + # TODO: implement this method, if we ever need it. def create(self, *args, **kwargs): """ :raises: :class:`NotImplementedError` From ff93b84e954bfcc40890086657af885d19f99691 Mon Sep 17 00:00:00 2001 From: Danny McClanahan <1305167+cosmicexplorer@users.noreply.github.com> Date: Mon, 11 Jun 2018 17:28:07 -0700 Subject: [PATCH 12/16] bump default_version and add comment --- src/python/pants/binaries/binary_tool.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/python/pants/binaries/binary_tool.py b/src/python/pants/binaries/binary_tool.py index 816c22bc4ef..90799dd3341 100644 --- a/src/python/pants/binaries/binary_tool.py +++ b/src/python/pants/binaries/binary_tool.py @@ -70,6 +70,9 @@ def _get_archiver(self): if not self.archive_type: return None + # This forces downloading and extracting the `XZ` archive if any BinaryTool with a 'txz' + # archive_type is used, but that's fine, because unless the cache is manually changed we won't + # do more work than necessary. if self.archive_type == 'txz': return self._xz.tar_xz_extractor @@ -216,7 +219,7 @@ def path_entries(self): class XZ(NativeTool): options_scope = 'xz' - default_version = '5.2.4' + default_version = '5.2.4-1' archive_type = 'tgz' @memoized_property From b43003bdfd971deac6072f102abbdd3a2fb06560 Mon Sep 17 00:00:00 2001 From: Danny McClanahan <1305167+cosmicexplorer@users.noreply.github.com> Date: Mon, 11 Jun 2018 19:06:52 -0700 Subject: [PATCH 13/16] set our member variable which was accidentally removed --- src/python/pants/fs/archive.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/python/pants/fs/archive.py b/src/python/pants/fs/archive.py index 8ff4a4cf460..ea680aff57e 100644 --- a/src/python/pants/fs/archive.py +++ b/src/python/pants/fs/archive.py @@ -109,6 +109,8 @@ def __init__(self, xz_binary_path): "to decompress xz archives." .format(xz_binary_path)) + self._xz_binary_path = xz_binary_path + super(XZCompressedTarArchiver, self).__init__('r|', 'tar.xz') @contextmanager From bc5c0e02341db09e3cdc9aa29eb2e5d9a8a1dfa0 Mon Sep 17 00:00:00 2001 From: Danny McClanahan <1305167+cosmicexplorer@users.noreply.github.com> Date: Tue, 12 Jun 2018 12:38:28 -0700 Subject: [PATCH 14/16] remove pants-side xz execution wrapping --- src/python/pants/backend/python/pex_util.py | 2 +- src/python/pants/fs/archive.py | 23 +++++++-------------- 2 files changed, 8 insertions(+), 17 deletions(-) diff --git a/src/python/pants/backend/python/pex_util.py b/src/python/pants/backend/python/pex_util.py index f5ba1eee250..6ca782b5433 100644 --- a/src/python/pants/backend/python/pex_util.py +++ b/src/python/pants/backend/python/pex_util.py @@ -32,4 +32,4 @@ def get_local_platform(): # TODO(John Sirois): Kill some or all usages when https://github.com/pantsbuild/pex/issues/511 # is fixed. current_platform = Platform.current() - return current_platform.platform \ No newline at end of file + return current_platform.platform diff --git a/src/python/pants/fs/archive.py b/src/python/pants/fs/archive.py index ea680aff57e..7c2ae2e564a 100644 --- a/src/python/pants/fs/archive.py +++ b/src/python/pants/fs/archive.py @@ -13,8 +13,7 @@ from zipfile import ZIP_DEFLATED from pants.util.contextutil import open_tar, open_zip, temporary_dir -from pants.util.dirutil import (is_executable, safe_concurrent_rename, safe_walk, - split_basename_and_dirname) +from pants.util.dirutil import is_executable, safe_concurrent_rename, safe_walk from pants.util.meta import AbstractClass from pants.util.process_handler import subprocess from pants.util.strutil import ensure_text @@ -120,28 +119,21 @@ def _invoke_xz(self, xz_input_file): This allows streaming the decompressed tar archive directly into a tar decompression stream, which is significantly faster in practice than making a temporary file. """ - (xz_bin_dir, xz_filename) = split_basename_and_dirname(self._xz_binary_path) - # FIXME: --threads=0 is supposed to use "the number of processor cores on the machine", but I # see no more than 100% cpu used at any point. This seems like it could be a bug? If performance # is an issue, investigate further. - cmd = [xz_filename, '--decompress', '--stdout', '--keep', '--threads=0', xz_input_file] - env = { - # Isolate the path so we know we're using our provided version of xz. - 'PATH': xz_bin_dir, - } + cmd = [self._xz_binary_path, '--decompress', '--stdout', '--keep', '--threads=0', xz_input_file] try: # Pipe stderr to our own stderr, but leave stdout open so we can yield it. process = subprocess.Popen( cmd, stdout=subprocess.PIPE, - stderr=sys.stderr, - env=env) + stderr=sys.stderr) except OSError as e: raise self.XZArchiverError( - "Error invoking xz with command {} and environment {} for input file {}: {}" - .format(cmd, env, xz_input_file, e), + "Error invoking xz with command {} for input file {}: {}" + .format(cmd, xz_input_file, e), e) # This is a file object. @@ -150,9 +142,8 @@ def _invoke_xz(self, xz_input_file): rc = process.wait() if rc != 0: raise self.XZArchiverError( - "Error decompressing xz input with command {} and environment {} for input file {}. " - "Exit code was: {}. " - .format(cmd, env, xz_input_file, rc)) + "Error decompressing xz input with command {} for input file {}. Exit code was: {}. " + .format(cmd, xz_input_file, rc)) def _extract(self, path, outdir): with self._invoke_xz(path) as xz_decompressed_tar_stream: From cead478a2463866e00dcaa3cb46b20c46079c16f Mon Sep 17 00:00:00 2001 From: Daniel McClanahan <1305167+cosmicexplorer@users.noreply.github.com> Date: Tue, 12 Jun 2018 14:08:57 -0700 Subject: [PATCH 15/16] bump default xz version to match binary archive with wrapper script --- src/python/pants/binaries/binary_tool.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/python/pants/binaries/binary_tool.py b/src/python/pants/binaries/binary_tool.py index 90799dd3341..cd276cf6e18 100644 --- a/src/python/pants/binaries/binary_tool.py +++ b/src/python/pants/binaries/binary_tool.py @@ -219,7 +219,7 @@ def path_entries(self): class XZ(NativeTool): options_scope = 'xz' - default_version = '5.2.4-1' + default_version = '5.2.4-2' archive_type = 'tgz' @memoized_property From 1a38e9282ac4b3a50c6e2dfc5711d7791ca9a2aa Mon Sep 17 00:00:00 2001 From: Danny McClanahan <1305167+cosmicexplorer@users.noreply.github.com> Date: Tue, 12 Jun 2018 21:03:40 -0700 Subject: [PATCH 16/16] sync up with pantsbuild/binaries#74 --- src/python/pants/binaries/binary_tool.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/python/pants/binaries/binary_tool.py b/src/python/pants/binaries/binary_tool.py index cd276cf6e18..5b8753073bf 100644 --- a/src/python/pants/binaries/binary_tool.py +++ b/src/python/pants/binaries/binary_tool.py @@ -219,7 +219,7 @@ def path_entries(self): class XZ(NativeTool): options_scope = 'xz' - default_version = '5.2.4-2' + default_version = '5.2.4-3' archive_type = 'tgz' @memoized_property