use liblzma.dylib for xz on osx and add platform-specific testing to the rust osx shard #5936
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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: 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,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 {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 | ||
|
|
||
|
|
@@ -140,16 +156,24 @@ def _invoke_xz(self, xz_input_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, | ||
| } | ||
|
|
||
| # 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 | ||
|
There was a problem hiding this comment. I think this should probably actually be: Two important differences:
(The one below should probably also set the existing path) But I think it would be better for us to make sure the There was a problem hiding this comment. So there is a very useful method There was a problem hiding this comment. Added a wrapper script to the |
||
| 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( | ||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than setting
$PATHhere, just usingself._xz_binary_pathas the first arg ofcmdshould do the job, right?Then we can either use the system env, as pants tends to, or an empty env, as I would be very happy about
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes! The
envbit was only necessary to emulate the kind of wrapping we are now doing in pantsbuild/binaries#72. Were you thinking I might want to explicitly clear the env here?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, explicitly clearing the env in general is nice because it means we're more consistent/reproducible.
It probably doesn't matter either way here, but for things on the build path which we cache (possibly even outside of the local machine) things (like invoking gcc), stripping the entire env, and adding whatever we need explicitly, is a nice safe approach to try to ensure reproducibility. It also means that we can think to explicitly add those env vars to any cache keys we may need to (or, with v2 process execution, this happens by default, so it ensures that we actually get cache hits across users, because otherwise things like
$HOMEwill always be different! :))