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.

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

Allow tests to run with isolated stores #6743

Merged
merged 1 commit into from Nov 9, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
3 changes: 3 additions & 0 deletions src/python/pants/engine/native.py
Expand Up @@ -202,6 +202,7 @@
TypeId,
Buffer,
Buffer,
Buffer,
BufferBuffer,
TypeIdBuffer,
Buffer,
Expand Down Expand Up @@ -802,6 +803,7 @@ def new_scheduler(self,
root_subject_types,
build_root,
work_dir,
local_store_dir,
ignore_patterns,
execution_options,
construct_directory_digest,
Expand Down Expand Up @@ -865,6 +867,7 @@ def tc(constraint):
# Project tree.
self.context.utf8_buf(build_root),
self.context.utf8_buf(work_dir),
self.context.utf8_buf(local_store_dir),
self.context.utf8_buf_buf(ignore_patterns),
self.to_ids_buf(root_subject_types),
# Remote execution config.
Expand Down
3 changes: 3 additions & 0 deletions src/python/pants/engine/scheduler.py
Expand Up @@ -83,6 +83,7 @@ def __init__(
native,
project_tree,
work_dir,
local_store_dir,
rules,
execution_options,
include_trace_on_error=True,
Expand All @@ -92,6 +93,7 @@ def __init__(
:param native: An instance of engine.native.Native.
:param project_tree: An instance of ProjectTree for the current build root.
:param work_dir: The pants work dir.
:param local_store_dir: The directory to use for storing the engine's LMDB store in.
:param rules: A set of Rules which is used to compute values in the graph.
:param execution_options: Execution options for (remote) processes.
:param include_trace_on_error: Include the trace through the graph upon encountering errors.
Expand Down Expand Up @@ -119,6 +121,7 @@ def __init__(
root_subject_types=self._root_subject_types,
build_root=project_tree.build_root,
work_dir=work_dir,
local_store_dir=local_store_dir,
ignore_patterns=project_tree.ignore_patterns,
execution_options=execution_options,
construct_directory_digest=Digest,
Expand Down
4 changes: 4 additions & 0 deletions src/python/pants/init/engine_initializer.py
Expand Up @@ -256,6 +256,7 @@ def setup_legacy_graph(native, bootstrap_options, build_configuration):
return EngineInitializer.setup_legacy_graph_extended(
bootstrap_options.pants_ignore,
bootstrap_options.pants_workdir,
bootstrap_options.local_store_dir,
bootstrap_options.build_file_imports,
build_configuration,
native=native,
Expand All @@ -271,6 +272,7 @@ def setup_legacy_graph(native, bootstrap_options, build_configuration):
def setup_legacy_graph_extended(
pants_ignore_patterns,
workdir,
local_store_dir,
build_file_imports_behavior,
build_configuration,
build_root=None,
Expand All @@ -287,6 +289,7 @@ def setup_legacy_graph_extended(
:param list pants_ignore_patterns: A list of path ignore patterns for FileSystemProjectTree,
usually taken from the '--pants-ignore' global option.
:param str workdir: The pants workdir.
:param local_store_dir: The directory to use for storing the engine's LMDB store in.
:param build_file_imports_behavior: How to behave if a BUILD file being parsed tries to use
import statements. Valid values: "allow", "warn", "error".
:type build_file_imports_behavior: string
Expand Down Expand Up @@ -360,6 +363,7 @@ def setup_legacy_graph_extended(
native,
project_tree,
workdir,
local_store_dir,
rules,
execution_options,
include_trace_on_error=include_trace_on_error,
Expand Down
5 changes: 5 additions & 0 deletions src/python/pants/option/global_options.py
Expand Up @@ -277,6 +277,11 @@ def register_bootstrap_options(cls, register):
advanced=True,
help='Whether to allow import statements in BUILD files')

register('--local-store-dir', advanced=True,
help="Directory to use for engine's local file store.",
# This default is also hard-coded into the engine's rust code in
# fs::Store::default_path
default=os.path.expanduser('~/.cache/pants/lmdb_store'))
register('--remote-store-server', advanced=True,
help='host:port of grpc server to use as remote execution file store.')
register('--remote-store-thread-count', type=int, advanced=True,
Expand Down
1 change: 1 addition & 0 deletions src/rust/engine/fs/src/store.rs
Expand Up @@ -119,6 +119,7 @@ impl Store {
})
}

// This default is also hard-coded into the Python options code in global_options.py
pub fn default_path() -> PathBuf {
match dirs::home_dir() {
Some(home_dir) => home_dir.join(".cache").join("pants").join("lmdb_store"),
Expand Down
12 changes: 6 additions & 6 deletions src/rust/engine/src/context.rs
Expand Up @@ -51,6 +51,7 @@ impl Core {
build_root: &Path,
ignore_patterns: &[String],
work_dir: PathBuf,
local_store_dir: PathBuf,
remote_store_server: Option<String>,
remote_execution_server: Option<String>,
remote_instance_name: Option<String>,
Expand Down Expand Up @@ -88,13 +89,12 @@ impl Core {

let fs_pool2 = fs_pool.clone();
let store_and_command_runner = Resettable::new(move || {
let store_path = Store::default_path();

let store = safe_create_dir_all_ioerror(&store_path)
.map_err(|e| format!("Error making directory {:?}: {:?}", store_path, e))
let local_store_dir = local_store_dir.clone();
let store = safe_create_dir_all_ioerror(&local_store_dir)
.map_err(|e| format!("Error making directory {:?}: {:?}", local_store_dir, e))
.and_then(|()| match &remote_store_server {
Some(ref address) => Store::with_remote(
store_path,
local_store_dir,
fs_pool2.clone(),
address,
remote_instance_name.clone(),
Expand All @@ -104,7 +104,7 @@ impl Core {
remote_store_chunk_bytes,
remote_store_chunk_upload_timeout,
),
None => Store::local_only(store_path, fs_pool2.clone()),
None => Store::local_only(local_store_dir, fs_pool2.clone()),
}).unwrap_or_else(|e| panic!("Could not initialize Store: {:?}", e));

let underlying_command_runner: Box<CommandRunner> = match &remote_execution_server {
Expand Down
2 changes: 2 additions & 0 deletions src/rust/engine/src/lib.rs
Expand Up @@ -255,6 +255,7 @@ pub extern "C" fn scheduler_create(
type_bytes: TypeId,
build_root_buf: Buffer,
work_dir_buf: Buffer,
local_store_dir_buf: Buffer,
ignore_patterns_buf: BufferBuffer,
root_type_ids: TypeIdBuffer,
remote_store_server: Buffer,
Expand Down Expand Up @@ -336,6 +337,7 @@ pub extern "C" fn scheduler_create(
build_root_buf.to_os_string().as_ref(),
&ignore_patterns,
PathBuf::from(work_dir_buf.to_os_string()),
PathBuf::from(local_store_dir_buf.to_os_string()),
if remote_store_server_string.is_empty() {
None
} else {
Expand Down
26 changes: 14 additions & 12 deletions tests/python/pants_test/engine/legacy/test_graph.py
Expand Up @@ -58,18 +58,20 @@ def graph_helper(self,
path_ignore_patterns=None):

with temporary_dir() as work_dir:
path_ignore_patterns = path_ignore_patterns or []
build_config = build_configuration or self._default_build_config()
# TODO: This test should be swapped to using TestBase.
graph_helper = EngineInitializer.setup_legacy_graph_extended(
path_ignore_patterns,
work_dir,
build_file_imports_behavior,
build_configuration=build_config,
native=self._native,
include_trace_on_error=include_trace_on_error
)
yield graph_helper
with temporary_dir() as local_store_dir:
path_ignore_patterns = path_ignore_patterns or []
build_config = build_configuration or self._default_build_config()
# TODO: This test should be swapped to using TestBase.
graph_helper = EngineInitializer.setup_legacy_graph_extended(
path_ignore_patterns,
work_dir,
local_store_dir,
build_file_imports_behavior,
build_configuration=build_config,
native=self._native,
include_trace_on_error=include_trace_on_error
)
yield graph_helper

@contextmanager
def open_scheduler(self, specs, build_configuration=None):
Expand Down
2 changes: 2 additions & 0 deletions tests/python/pants_test/engine/scheduler_test_base.py
Expand Up @@ -53,9 +53,11 @@ def mk_scheduler(self,
rules = rules or []
work_dir = work_dir or self._create_work_dir()
project_tree = project_tree or self.mk_fs_tree(work_dir=work_dir)
local_store_dir = os.path.realpath(safe_mkdtemp())
scheduler = Scheduler(self._native,
project_tree,
work_dir,
local_store_dir,
rules,
DEFAULT_EXECUTION_OPTIONS,
include_trace_on_error=include_trace_on_error)
Expand Down
74 changes: 38 additions & 36 deletions tests/python/pants_test/engine/test_fs.py
Expand Up @@ -414,8 +414,7 @@ def test_materialize_directories(self):
text_type("63949aa823baf765eff07b946050d76ec0033144c785a94d3ebd82baa931cd16"),
80
)
scheduler = self.mk_scheduler(rules=create_fs_rules())
scheduler.materialize_directories((DirectoryToMaterialize(text_type(dir_path), digest),))
self.scheduler.materialize_directories((DirectoryToMaterialize(text_type(dir_path), digest),))

created_file = os.path.join(dir_path, "roland")
with open(created_file, 'r') as f:
Expand Down Expand Up @@ -471,9 +470,8 @@ def prime_store_with_roland_digest(self):
with temporary_dir() as temp_dir:
with open(os.path.join(temp_dir, "roland"), "w") as f:
f.write("European Burmese")
scheduler = self.mk_scheduler(rules=create_fs_rules())
globs = PathGlobs(("*",), ())
snapshot = scheduler.capture_snapshots((PathGlobsAndRoot(globs, text_type(temp_dir)),))[0]
snapshot = self.scheduler.capture_snapshots((PathGlobsAndRoot(globs, text_type(temp_dir)),))[0]
self.assert_snapshot_equals(snapshot, ["roland"], Digest(
text_type("63949aa823baf765eff07b946050d76ec0033144c785a94d3ebd82baa931cd16"),
80
Expand All @@ -482,45 +480,49 @@ def prime_store_with_roland_digest(self):
pantsbuild_digest = Digest("63652768bd65af8a4938c415bdc25e446e97c473308d26b3da65890aebacf63f", 18)

def test_download(self):
with http_server(StubHandler) as port:
url = UrlToFetch("http://localhost:{}/CNAME".format(port), self.pantsbuild_digest)
snapshot, = self.scheduler.product_request(Snapshot, subjects=[url])
self.assert_snapshot_equals(snapshot, ["CNAME"], Digest(
text_type("16ba2118adbe5b53270008790e245bbf7088033389461b08640a4092f7f647cf"),
81
))
with self.isolated_local_store():
with http_server(StubHandler) as port:
url = UrlToFetch("http://localhost:{}/CNAME".format(port), self.pantsbuild_digest)
snapshot, = self.scheduler.product_request(Snapshot, subjects=[url])
self.assert_snapshot_equals(snapshot, ["CNAME"], Digest(
text_type("16ba2118adbe5b53270008790e245bbf7088033389461b08640a4092f7f647cf"),
81
))

def test_download_missing_file(self):
with http_server(StubHandler) as port:
url = UrlToFetch("http://localhost:{}/notfound".format(port), self.pantsbuild_digest)
with self.assertRaises(ExecutionError) as cm:
self.scheduler.product_request(Snapshot, subjects=[url])
self.assertIn('404', str(cm.exception))
with self.isolated_local_store():
with http_server(StubHandler) as port:
url = UrlToFetch("http://localhost:{}/notfound".format(port), self.pantsbuild_digest)
with self.assertRaises(ExecutionError) as cm:
self.scheduler.product_request(Snapshot, subjects=[url])
self.assertIn('404', str(cm.exception))

def test_download_wrong_digest(self):
with http_server(StubHandler) as port:
url = UrlToFetch(
"http://localhost:{}/CNAME".format(port),
Digest(
self.pantsbuild_digest.fingerprint,
self.pantsbuild_digest.serialized_bytes_length + 1,
),
)
with self.assertRaises(ExecutionError) as cm:
self.scheduler.product_request(Snapshot, subjects=[url])
self.assertIn('wrong digest', str(cm.exception).lower())
with self.isolated_local_store():
with http_server(StubHandler) as port:
url = UrlToFetch(
"http://localhost:{}/CNAME".format(port),
Digest(
self.pantsbuild_digest.fingerprint,
self.pantsbuild_digest.serialized_bytes_length + 1,
),
)
with self.assertRaises(ExecutionError) as cm:
self.scheduler.product_request(Snapshot, subjects=[url])
self.assertIn('wrong digest', str(cm.exception).lower())

# It's a shame that this isn't hermetic, but setting up valid local HTTPS certificates is a pain.
def test_download_https(self):
url = UrlToFetch("https://www.pantsbuild.org/CNAME", Digest(
"63652768bd65af8a4938c415bdc25e446e97c473308d26b3da65890aebacf63f",
18,
))
snapshot, = self.scheduler.product_request(Snapshot, subjects=[url])
self.assert_snapshot_equals(snapshot, ["CNAME"], Digest(
text_type("16ba2118adbe5b53270008790e245bbf7088033389461b08640a4092f7f647cf"),
81
))
with self.isolated_local_store():
url = UrlToFetch("https://www.pantsbuild.org/CNAME", Digest(
"63652768bd65af8a4938c415bdc25e446e97c473308d26b3da65890aebacf63f",
18,
))
snapshot, = self.scheduler.product_request(Snapshot, subjects=[url])
self.assert_snapshot_equals(snapshot, ["CNAME"], Digest(
text_type("16ba2118adbe5b53270008790e245bbf7088033389461b08640a4092f7f647cf"),
81
))


class StubHandler(BaseHTTPRequestHandler):
Expand Down
2 changes: 2 additions & 0 deletions tests/python/pants_test/engine/util.py
Expand Up @@ -19,6 +19,7 @@
from pants.engine.selectors import Get
from pants.engine.struct import Struct
from pants.option.global_options import DEFAULT_EXECUTION_OPTIONS
from pants.util.dirutil import safe_mkdtemp
from pants.util.memo import memoized
from pants.util.objects import SubclassesOf
from pants_test.option.util.fakes import create_options_for_optionables
Expand Down Expand Up @@ -100,6 +101,7 @@ def create_scheduler(rules, validate=True, native=None):
native,
FileSystemProjectTree(os.getcwd()),
'./.pants.d',
safe_mkdtemp(),
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

See my comment on the other review.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think isolation by default is good, because:

  1. Isolation is generally good. I think @dotordogh or I ran into issues where
    def test_materialize_directories(self):
    # I tried passing in the digest of a file, but it didn't make it to the
    # rust code due to all of the checks we have in place (which is probably a good thing).
    self.prime_store_with_roland_digest()
    with temporary_dir() as temp_dir:
    dir_path = os.path.join(temp_dir, "containing_roland")
    digest = Digest(
    text_type("63949aa823baf765eff07b946050d76ec0033144c785a94d3ebd82baa931cd16"),
    80
    )
    scheduler = self.mk_scheduler(rules=create_fs_rules())
    scheduler.materialize_directories((DirectoryToMaterialize(text_type(dir_path), digest),))
    created_file = os.path.join(dir_path, "roland")
    with open(created_file, 'r') as f:
    content = f.read()
    self.assertEqual(content, "European Burmese")
    was either failing when it should pass, or passing when it should fail, before the prime_store_with_roland_digest was added, because it relied on the store already happening to have the digest in question in it.
  2. The speedup of re-using an existing store should be pretty small.

(The "force isolation" function is handy because the default in this PR is actually to share a store across an test class)

Happy to re-visit if you are convinced otherwise :)

rules,
execution_options=DEFAULT_EXECUTION_OPTIONS,
validate=validate,
Expand Down
25 changes: 23 additions & 2 deletions tests/python/pants_test/test_base.py
Expand Up @@ -35,8 +35,8 @@
from pants.source.source_root import SourceRootConfig
from pants.subsystem.subsystem import Subsystem
from pants.task.goal_options_mixin import GoalOptionsMixin
from pants.util.dirutil import (recursive_dirname, relative_symlink, safe_mkdir, safe_open,
safe_rmtree)
from pants.util.dirutil import (recursive_dirname, relative_symlink, safe_mkdir, safe_mkdtemp,
safe_open, safe_rmtree)
from pants.util.memo import memoized_method
from pants_test.base.context_utils import create_context_from_options
from pants_test.engine.util import init_native
Expand Down Expand Up @@ -83,6 +83,7 @@ class TestBase(unittest.TestCase):
"""

_scheduler = None
_local_store_dir = None
_build_graph = None
_address_mapper = None

Expand Down Expand Up @@ -341,6 +342,22 @@ def _reset_engine(self):
self._build_graph.reset()
self._scheduler.invalidate_all_files()

@classmethod
def aggressively_reset_scheduler(cls):
cls._scheduler = None
if cls._local_store_dir is not None:
safe_rmtree(cls._local_store_dir)

@classmethod
@contextmanager
def isolated_local_store(cls):
cls.aggressively_reset_scheduler()
cls._init_engine()
try:
yield
finally:
cls.aggressively_reset_scheduler()

@property
def build_root(self):
return self._build_root()
Expand All @@ -365,11 +382,15 @@ def _init_engine(cls):
if cls._scheduler is not None:
return

cls._local_store_dir = os.path.realpath(safe_mkdtemp())
safe_mkdir(cls._local_store_dir)

# NB: This uses the long form of initialization because it needs to directly specify
# `cls.alias_groups` rather than having them be provided by bootstrap options.
graph_session = EngineInitializer.setup_legacy_graph_extended(
pants_ignore_patterns=None,
workdir=cls._pants_workdir(),
local_store_dir=cls._local_store_dir,
build_file_imports_behavior='allow',
native=init_native(),
build_configuration=cls.build_config(),
Expand Down