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

Add support for merging Snapshots #5746

Merged
merged 4 commits into from Apr 26, 2018

Conversation

Projects
None yet
2 participants
@stuhood
Copy link
Member

stuhood commented Apr 25, 2018

Problem

As described in #5707: we need a way to merge Snapshot objects (although we have not yet decided how to expose them to @rules.)

Solution

Add Snapshot::merge.

Result

Fixes #5707.

@stuhood stuhood requested review from illicitonion and dotordogh Apr 25, 2018

@stuhood stuhood force-pushed the twitter:stuhood/merge-snapshots branch from 5577a5c to 2293f7f Apr 25, 2018

@stuhood

This comment has been minimized.

Copy link
Member

stuhood commented Apr 25, 2018

(for the record: the individual commits here aren't useful)

@stuhood stuhood force-pushed the twitter:stuhood/merge-snapshots branch from 2293f7f to dd07479 Apr 25, 2018

@illicitonion
Copy link
Contributor

illicitonion left a comment

Looks great :) Thanks!

// TODO: This will not have been stored... we'll need to explicitly store it.
return future::ok(EMPTY_DIGEST).to_boxed();
} else if dir_digests.len() == 1 {
let mut dir_digests = dir_digests;

This comment has been minimized.

@illicitonion

illicitonion Apr 26, 2018

Contributor

Would possibly be a little clearer to make the argument mut, rather than to mut-ify it in the method body

This comment has been minimized.

@stuhood

stuhood Apr 26, 2018

Member

I did this to avoid that... this localizes the awareness of the mutability to where it is actually relevant.

};
let merged_root_directory = store.load_directory(merged.digest).wait().unwrap().unwrap();

assert_eq!(merged.path_stats.len(), 3);

This comment has been minimized.

@illicitonion

illicitonion Apr 26, 2018

Contributor

Can you compare the actual contents, rather than just the length? Ordering is well defined here

This comment has been minimized.

@stuhood

stuhood Apr 26, 2018

Member

Yea, good point.

.files
.iter()
.map(|filenode| filenode.name.clone())
.collect::<HashSet<_>>(),

This comment has been minimized.

@illicitonion

illicitonion Apr 26, 2018

Contributor

Nodes in Directories have a canonical ordering (they should be sorted), so this should probably be a Vec rather than a HashSet

};

match merged_res {
Err(ref msg) if msg.contains("Snapshots contained duplicate path: ") => (),

This comment has been minimized.

@illicitonion

illicitonion Apr 26, 2018

Contributor

Maybe throw in a check that the error message contains the strong roland?

@illicitonion
Copy link
Contributor

illicitonion left a comment

Thanks!

@stuhood stuhood merged commit 2c9cece into pantsbuild:master Apr 26, 2018

1 check passed

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

@stuhood stuhood deleted the twitter:stuhood/merge-snapshots branch Apr 26, 2018

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