use liblzma.dylib for xz on osx and add platform-specific testing to …

…the rust osx shard (#5936) ### Problem See #5928. The `xz` archiver wasn't tested on osx at all, and failed to find `liblzma.so` on osx (it should have been `liblzma.dylib`). There were additional errors with library search paths reported in that PR which I was not immediately able to repro. This PR hopefully fixes all of those errors, and ensures they won't happen again with the addition of platform-specific testing (see previous issue at #5920). ### Solution - Switch to a statically linked `xz`. - Fix the incorrect key `'darwin'` in the platform dictionary in the `LLVM` subsystem (to `'mac'`). - Add the tag `platform_specific_behavior` to the new python target `tests/python/pants_test/backend/python/tasks:python_native_code_testing`, which covers the production of `python_dist()`s with native code. - Add the `-z` argument to `build-support/bin/ci.sh` to run all tests with the `platform_specific_behavior` tag. Also clean up old unused options in the getopts call, and convert echo statements to a simpler heredoc. - Change the name of the "Rust Tests OSX" shard to "Rust + Platform-specific Tests OSX", and add the `-z` switch to the `ci.sh` invocation. **Note:** the tests in `tests/python/pants_test/backend/native/subsystems` are going to be removed in #5815, otherwise they would be tagged similarly. ### Result `./pants test tests/python/pants_test/backend/python/tasks:python_native_code_testing` now passes on osx, and this fact is now being tested in an osx shard in travis.
pantsbuild · Jun 13, 2018 · 27714fe · 27714fe
1 parent 2429091
commit 27714fe
Show file tree

Hide file tree

Showing 6 changed files with 102 additions and 87 deletions.
diff --git a/.travis.yml b/.travis.yml
@@ -430,14 +430,15 @@ 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
+        # 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
@@ -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 <<EOF
+Runs commons tests for local or hosted CI.
+
+Usage: $0 (-h|-fxbkmsrjlpuyncia)
+ -h           print out this help message
+ -f           skip python code formatting checks
+ -x           skip bootstrap clean-all (assume bootstrapping from a
+              fresh clone)
+ -b           skip bootstraping pants from local sources
+ -k           skip bootstrapped pants self compile check
+ -m           skip sanity checks of bootstrapped pants and repo BUILD
+              files
+ -r           skip doc generation tests
+ -j           skip core jvm tests
+ -l           skip internal backends python tests
+ -p           skip core python tests
+ -u SHARD_NUMBER/TOTAL_SHARDS
+              if running core python tests, divide them into
+              TOTAL_SHARDS shards and just run those in SHARD_NUMBER
+              to run only even tests: '-u 0/2', odd: '-u 1/2'
+ -n           skip contrib python tests
+ -e           skip rust tests
+ -y SHARD_NUMBER/TOTAL_SHARDS
+              if running contrib python tests, divide them into
+              TOTAL_SHARDS shards and just run those in SHARD_NUMBER
+              to run only even tests: '-u 0/2', odd: '-u 1/2'
+ -c           skip pants integration tests (includes examples and testprojects)
+ -i SHARD_NUMBER/TOTAL_SHARDS
+              if running integration tests, divide them into
+              TOTAL_SHARDS shards and just run those in SHARD_NUMBER
+              to run only even tests: '-i 0/2', odd: '-i 1/2'
+ -t           skip lint
+ -z           test platform-specific behavior
+EOF
   if (( $# > 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,18 @@ 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)"
+  (
+    ./pants.pex ${PANTS_ARGS[@]} --tag='+platform_specific_behavior' test \
+                tests/python:: -- ${PYTEST_PASSTHRU_ARGS}
+  ) || 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/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',
   }
 

diff --git 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),)
 
@@ -71,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
 
@@ -217,15 +219,12 @@ def path_entries(self):
 
 class XZ(NativeTool):
   options_scope = 'xz'
-  default_version = '5.2.4'
+  default_version = '5.2.4-3'
   archive_type = 'tgz'
 
   @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
@@ -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
@@ -96,16 +95,13 @@ 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
 
-  def __init__(self, xz_binary_path, xz_library_path):
+  def __init__(self, xz_binary_path):
 
-    # TODO(cosmicexplorer): 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 "
@@ -114,21 +110,6 @@ def __init__(self, xz_binary_path, xz_library_path):
 
     self._xz_binary_path = xz_binary_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 "
-        "be provided to decompress xz archives."
-        .format(xz_library_path))
-
-    lib_lzma_dylib = os.path.join(xz_library_path, 'liblzma.so')
-    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."
-        .format(xz_library_path))
-
-    self._xz_library_path = xz_library_path
-
     super(XZCompressedTarArchiver, self).__init__('r|', 'tar.xz')
 
   @contextmanager
@@ -138,29 +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)
-
-    # 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.
-    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.
-      'LD_LIBRARY_PATH': self._xz_library_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 = [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.
@@ -169,15 +142,15 @@ 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:
       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`

diff --git a/tests/python/pants_test/backend/python/tasks/BUILD b/tests/python/pants_test/backend/python/tasks/BUILD
@@ -17,8 +17,33 @@ 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=python_native_code_test_files,
+  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'},
+  timeout=2400,
+)
+
 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',
@@ -55,7 +80,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',