-
-
Notifications
You must be signed in to change notification settings - Fork 638
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.
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
cache python tools in ~/.cache/pants #7236
Merged
cosmicexplorer
merged 10 commits into
pantsbuild:master
from
cosmicexplorer:cache-python-tools
Feb 22, 2019
Merged
Changes from 7 commits
Commits
Show all changes
10 commits
Select commit
Hold shift + click to select a range
9033602
cache python tools in ~/.cache/pants
cosmicexplorer 7061521
namespace pex tools by interpreter identity
cosmicexplorer 5a67f6e
add testing, which fails on this machine
cosmicexplorer 15dce5d
make the python test work!
cosmicexplorer f5511b2
add note about caching granularity from PR comments
cosmicexplorer 8673c25
add explicit interpreter constraints for the conan tool
cosmicexplorer 64a8f12
expand conan default interpreter constraints and add TODO
cosmicexplorer 0156bb0
remove fragile/unnecessary empty stderr assert
cosmicexplorer 8846543
remove the extremely fragile globbing and just compare the pex paths
cosmicexplorer ee9ab74
canonically override the cache dir location in options and ensure we …
cosmicexplorer File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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
91 changes: 91 additions & 0 deletions
91
tests/python/pants_test/backend/python/tasks/test_python_tool.py
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 |
---|---|---|
@@ -0,0 +1,91 @@ | ||
# coding=utf-8 | ||
# Copyright 2019 Pants project contributors (see CONTRIBUTORS.md). | ||
# Licensed under the Apache License, Version 2.0 (see LICENSE). | ||
|
||
from __future__ import absolute_import, division, print_function, unicode_literals | ||
|
||
import glob | ||
import os | ||
import re | ||
|
||
from pants.backend.python.subsystems.python_tool_base import PythonToolBase | ||
from pants.backend.python.tasks.python_tool_prep_base import PythonToolInstance, PythonToolPrepBase | ||
from pants.task.task import Task | ||
from pants.util.collections import assert_single_element | ||
from pants.util.contextutil import environment_as, temporary_dir | ||
from pants_test.backend.python.tasks.python_task_test_base import PythonTaskTestBase | ||
|
||
|
||
class Tool(PythonToolBase): | ||
options_scope = 'test-tool' | ||
default_requirements = [ | ||
'pex==1.5.3', | ||
] | ||
default_entry_point = 'pex.bin.pex:main' | ||
|
||
|
||
class ToolInstance(PythonToolInstance): | ||
pass | ||
|
||
|
||
class ToolPrep(PythonToolPrepBase): | ||
options_scope = 'tool-prep-task' | ||
tool_subsystem_cls = Tool | ||
tool_instance_cls = ToolInstance | ||
|
||
|
||
class ToolTask(Task): | ||
options_scope = 'tool-task' | ||
|
||
@classmethod | ||
def prepare(cls, options, round_manager): | ||
super(ToolTask, cls).prepare(options, round_manager) | ||
round_manager.require_data(ToolPrep.tool_instance_cls) | ||
|
||
def execute(self): | ||
tool_for_pex = self.context.products.get_data(ToolPrep.tool_instance_cls) | ||
stdout, stderr, exit_code, _ = tool_for_pex.output(['--version']) | ||
assert '' == stderr | ||
cosmicexplorer marked this conversation as resolved.
Show resolved
Hide resolved
|
||
assert re.match(r'.*\.pex 1.5.3', stdout) | ||
assert 0 == exit_code | ||
|
||
|
||
class PythonToolPrepTest(PythonTaskTestBase): | ||
|
||
@classmethod | ||
def task_type(cls): | ||
return ToolTask | ||
|
||
def _assert_tool_execution_for_python_version(self, use_py3=True): | ||
scope_string = '3' if use_py3 else '2' | ||
constraint_string = 'CPython>=3' if use_py3 else 'CPython<3' | ||
tool_prep_type = self.synthesize_task_subtype(ToolPrep, 'tp_scope_py{}'.format(scope_string)) | ||
context = self.context(for_task_types=[tool_prep_type], for_subsystems=[Tool], options={ | ||
'test-tool': { | ||
'interpreter_constraints': [constraint_string], | ||
}, | ||
}) | ||
# XDG_CACHE_HOME overrides the location of the cache dir. | ||
cosmicexplorer marked this conversation as resolved.
Show resolved
Hide resolved
|
||
with temporary_dir() as tmp_dir, environment_as(XDG_CACHE_HOME=tmp_dir): | ||
tool_prep_task = tool_prep_type(context, os.path.join( | ||
self.pants_workdir, 'tp_py{}'.format(scope_string))) | ||
tool_prep_task.execute() | ||
# Check that the tool is in an interpreter-specific subdir of the cache dir. | ||
cosmicexplorer marked this conversation as resolved.
Show resolved
Hide resolved
|
||
constructed_tool_location_glob = os.path.join( | ||
tmp_dir, 'pants', 'python', | ||
'CPython-{}*'.format(scope_string), | ||
tool_prep_task.fingerprint, | ||
'test-tool-*.pex', | ||
) | ||
tool_location = assert_single_element(glob.glob(constructed_tool_location_glob)) | ||
self.assertTrue(os.path.isdir(tool_location)) | ||
|
||
# Check that the tool can be executed successfully. | ||
self.create_task(context).execute() | ||
pex_tool = context.products.get_data(ToolPrep.tool_instance_cls) | ||
self.assertTrue(pex_tool.interpreter.identity.matches(constraint_string)) | ||
|
||
def test_tool_execution(self): | ||
"""Test that python tools are fingerprinted by python interpreter.""" | ||
self._assert_tool_execution_for_python_version(use_py3=True) | ||
self._assert_tool_execution_for_python_version(use_py3=False) |
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.
The
tool_subsystem.get_requirement_specs()
is a list, but order shouldn't actually matter. This should probably be sorted, but I guess someone could intentionally tweak order to work around a particular requirement resolution resolve-order issue :/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.
I wasn't sure how to effectively fingerprint this and should have made a note of that. Under the assumption that the requirements are going to be static enough to not blow up the user's disk space with the pants cache, it might be reasonable to assume the ordering will remain static as well?
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, I think its reasonable to risk too-fine-grained-caching. In practice the lists are static as you suggest, and when mutated almost certainly to change deps and not just permute their order.