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

Expose materialize_directory as an intrinsic function on the scheduler #6028

Merged
merged 11 commits into from Jun 29, 2018

Conversation

Projects
None yet
3 participants
@dotordogh
Copy link
Contributor

dotordogh commented Jun 26, 2018

Solves #6007

@dotordogh dotordogh requested a review from illicitonion Jun 26, 2018

@illicitonion
Copy link
Contributor

illicitonion left a comment

Pretty much just a test to fix up a little :) Thanks!

};

with_scheduler(scheduler_ptr, |scheduler| {
let store = scheduler.core.store.clone();

This comment has been minimized.

@illicitonion

illicitonion Jun 26, 2018

Contributor

I think you can &scheduler.core.store rather than scheduler.core.store.clone() here - you don't actually need to move store anywhere, so a reference should work fine :)

This comment has been minimized.

@dotordogh

dotordogh Jun 27, 2018

Contributor

It didn't work passing a reference, but I also didn't need to clone the store. Changes to come.

@@ -357,6 +357,23 @@ def test_merge_directories(self):

self.assertEquals(both_snapshot.directory_digest, both_merged)

def test_materialize_directories(self):
with temporary_dir() as temp_dir:
dir_path = os.path.join(temp_dir, "roland")

This comment has been minimized.

@illicitonion

illicitonion Jun 26, 2018

Contributor

Either call this containing_roland rather than roland, or just use temp_dir as the containing dir

def test_materialize_directories(self):
with temporary_dir() as temp_dir:
dir_path = os.path.join(temp_dir, "roland")
digest = DirectoryDigest(

This comment has been minimized.

@illicitonion

illicitonion Jun 26, 2018

Contributor

Does anything guarantee that this digest is already in the store? If not, should probably copy the body of test_snapshot_from_outside_buildroot into this test (or a helper function) and run it, to prime the store.

(I'm actually not sure how this test is working right now... Oh... I know how... We don't create custom store paths for tests, it will just always use ${HOME}/.cache/pants/lmdb_store, right? So I guess if you delete that directory, and run this test in isolation, it will probably fail?)

This comment has been minimized.

@baroquebobcat

baroquebobcat Jun 26, 2018

Contributor

Speaking of custom store paths, Is there an issue for adding isolated lmdb paths to tests? Would that make sense to do?

This comment has been minimized.

@dotordogh

dotordogh Jun 27, 2018

Contributor

(I'm actually not sure how this test is working right now... Oh... I know how... We don't create custom store paths for tests, it will just always use ${HOME}/.cache/pants/lmdb_store, right? So I guess if you delete that directory, and run this test in isolation, it will probably fail?)

I can confirm that the test run on its own failed when I wiped ~/.cache/pants/lmdb_store, but passed when the store was primed with the digest.

This comment has been minimized.

@illicitonion

illicitonion Jun 27, 2018

Contributor

There is now! #6038

@dotordogh dotordogh requested review from baroquebobcat and illicitonion Jun 26, 2018

@baroquebobcat
Copy link
Contributor

baroquebobcat left a comment

This looks good to me! 👍

Follow on things I see

  • defining what happens if the paths to materialize to overlap within a collection of DirectoryToMaterializes. Right now, based on my reading of the extern and the underlying materialize_directories implementation, I think this would depend on the order the futures run in. It could be that this is already covered somewhere and I missed it. /cc @illicitonion
  • communicating partial failures and defining the behavior around them. I think with this patch, there'll be an error raised. I'd like to see what the errors look like for some cases, but I think that should be a follow-on work.
def test_materialize_directories(self):
with temporary_dir() as temp_dir:
dir_path = os.path.join(temp_dir, "roland")
digest = DirectoryDigest(

This comment has been minimized.

@baroquebobcat

baroquebobcat Jun 26, 2018

Contributor

Speaking of custom store paths, Is there an issue for adding isolated lmdb paths to tests? Would that make sense to do?

self.assertTrue(os.path.isfile(created_file))
with open(created_file) as f:
content = f.read()
self.assertEquals(content, "European Burmese")

This comment has been minimized.

@baroquebobcat

baroquebobcat Jun 26, 2018

Contributor

I've got one minor assertion style thought here.

I think it'd be clearer if you left out the exists and isfile checks, since open will raise an exception for either of those cases.

@@ -115,6 +115,9 @@ def file_stats(self):
return [p.stat for p in self.files]


class DirectoryToMaterialize(datatype([('path', str), ('directory_digest', DirectoryDigest)])):
pass

This comment has been minimized.

@baroquebobcat

baroquebobcat Jun 26, 2018

Contributor

It'd be nice if this had a docstring. Maybe something like

"""A request to materialize the contents of a directory digest at the provided path."""

Dorothy Ordogh
@@ -417,3 +417,16 @@ def test_glob_match_warn_logging(self):
self.assertEqual(1, len(all_warnings))
single_warning = all_warnings[0]
self.assertEqual("???", str(single_warning))

def prime_store_with_roland_digest(self):
"""This method primes the store with the digest of a file named 'roland' and contents 'European Burmese'."""

This comment has been minimized.

@illicitonion

illicitonion Jun 27, 2018

Contributor

s/the digest/a directory/?

def test_materialize_directories(self):
with temporary_dir() as temp_dir:
dir_path = os.path.join(temp_dir, "roland")
digest = DirectoryDigest(

This comment has been minimized.

@illicitonion

illicitonion Jun 27, 2018

Contributor

There is now! #6038

@illicitonion

This comment has been minimized.

Copy link
Contributor

illicitonion commented Jun 27, 2018

@baroquebobcat

defining what happens if the paths to materialize to overlap within a collection of DirectoryToMaterializes. Right now, based on my reading of the extern and the underlying materialize_directories implementation, I think this would depend on the order the futures run in. It could be that this is already covered somewhere and I missed it. /cc @illicitonion

Yeah, this is currently undefined. My instinct is to say "Don't do that"; maybe we can enforce that by checking that none of the paths to be materialized to are prefixes of any others?

communicating partial failures and defining the behavior around them. I think with this patch, there'll be an error raised. I'd like to see what the errors look like for some cases, but I think that should be a follow-on work.

I think any error => the whole operation fails with an error is reasonable, which I believe is what should happen here at the moment? But yeah, a test may be useful :)

@dotordogh dotordogh merged commit c8b42cb into pantsbuild:master Jun 29, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@dotordogh dotordogh deleted the dotordogh:dotordogh/issue-6007 branch Jun 29, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment