Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

use liblzma.dylib for xz on osx and add platform-specific testing to the rust osx shard #5936

Merged
merged 16 commits into from Jun 13, 2018
Merged

use liblzma.dylib for xz on osx and add platform-specific testing to the rust osx shard #5936

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
f607ea3
vary the xz shared lib filename depending on the current platform
cosmicexplorer Jun 8, 2018
3b4e07c
use the correct key 'mac' instead of 'darwin' into the platform
cosmicexplorer Jun 8, 2018
7a9f397
add `platform_specific_behavior` tag to python native code tests and …
cosmicexplorer Jun 8, 2018
b34dbb1
use self._liblzma_shared_lib_filename to make xz error messages more …
cosmicexplorer Jun 8, 2018
633a2b6
don't re-run platform-specific test sources twice in linux
cosmicexplorer Jun 8, 2018
0b493d5
add the bootstrap phase to the platform-specific python testing in th…
cosmicexplorer Jun 8, 2018
a1617a9
don't re-run platform-specific integration tests either on linux
cosmicexplorer Jun 8, 2018
4f78bbe
increase the timeout on platform-specific testing
cosmicexplorer Jun 8, 2018
34bd769
use the correct platform-specific library search path for the xz exe
cosmicexplorer Jun 9, 2018
62892ca
fix library path handling by using get_joined_path()
cosmicexplorer Jun 11, 2018
fe240da
remove library path manipulation thanks to pantsbuild/binaries#71
cosmicexplorer Jun 11, 2018
ff93b84
bump default_version and add comment
cosmicexplorer Jun 12, 2018
b43003b
set our member variable which was accidentally removed
cosmicexplorer Jun 12, 2018
bc5c0e0
remove pants-side xz execution wrapping
cosmicexplorer Jun 12, 2018
cead478
bump default xz version to match binary archive with wrapper script
cosmicexplorer Jun 12, 2018
1a38e92
sync up with pantsbuild/binaries#74
cosmicexplorer Jun 13, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
12 changes: 10 additions & 2 deletions src/python/pants/fs/archive.py
View file
Open in desktop
Expand Up @@ -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,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than setting $PATH here, just using self._xz_binary_path as the first arg of cmd should 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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes! The env bit 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?

Copy link
Contributor

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 $HOME will always be different! :))

# 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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should probably actually be:
env['DYLD_LIBRARY_PATH'] = '{}:{}'.format(self.._xz_library_path, env.get('DYLD_LIBRARY_PATH', ''))

Two important differences:

  1. Set the main path not the fallback
  2. Preserve existing values; they may be important

(The one below should probably also set the existing path)

But I think it would be better for us to make sure the xz we release to pantsbinaries runs out of the box (either by linking differently, or with a wrapper script), rather than patching this up in pants...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So there is a very useful method get_joined_path() I added to contextutil.py to do the path operations you describe. However, I believe pantsbuild/binaries#71 lets us remove these lines. I will make the changes here and wait for that PR to be merged.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a wrapper script to the xz released to pants-binaries in pantsbuild/binaries#72! Thanks, that was an very useful suggestion.

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(
Expand Down
3 changes: 2 additions & 1 deletion tests/python/pants_test/backend/python/tasks/BUILD
View file
Open in desktop
Expand Up @@ -39,6 +39,7 @@ python_tests(
'tests/python/pants_test/engine:scheduler_test_base',
],
tags={'platform_specific_behavior'},
timeout=2400,
)

python_tests(
Expand Down Expand Up @@ -79,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',
Expand Down