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

[remoting] Move local process execution tempdirs into the workdir, add option to not delete them #6023

Merged
merged 14 commits into from Jun 28, 2018

Conversation

Projects
None yet
3 participants
@kwlzn
Copy link
Member

kwlzn commented Jun 26, 2018

Closes #5996

@illicitonion
Copy link
Contributor

illicitonion left a comment

Looks good :) Just a bunch of small simplifications that can be made :)

@@ -90,6 +103,7 @@ impl super::CommandRunner for CommandRunner {
let env = req.env;
let output_file_paths = req.output_files;
let output_dir_paths = req.output_directories;
let cleanup_local_dirs = self.cleanup_local_dirs.clone();

This comment has been minimized.

@illicitonion

illicitonion Jun 26, 2018

Contributor

.clone() on a type which implements Copy, like bool, is unnecessary - the compiler will automatically insert copies where it needs to. Just let cleanup_local_dirs = self.cleanup_local_dirs; should be fine here.

This comment has been minimized.

@kwlzn

kwlzn Jun 27, 2018

Member

done

@@ -135,8 +149,16 @@ impl super::CommandRunner for CommandRunner {
)
})
// Force workdir not to get dropped until after we've ingested the outputs
.map(|result| (result, workdir))
.map(|(result, _workdir)| result)
.map(move |result| (result, workdir, cleanup_local_dirs) )

This comment has been minimized.

@illicitonion

illicitonion Jun 26, 2018

Contributor

You shouldn't need this change to the map, or to the closure below's arguments; cleanup_local_dirs should implicitly be moved into the closure below from the enclosing scope if you refer to it in the body of that closure.

This comment has been minimized.

@kwlzn

kwlzn Jun 27, 2018

Member

ah, indeed - some small journey of compilation errors lead me down that path.

seems like what I ultimately needed there was just the move bit up the chain tho, as without that I get:

error[E0373]: closure may outlive the current function, but it borrows `cleanup_local_dirs`, which is owned by the current function
   --> process_execution/src/local.rs:124:17
    |
124 |       .and_then(|(output, workdir)| {
    |                 ^^^^^^^^^^^^^^^^^^^ may outlive borrowed value `cleanup_local_dirs`
...
154 |             if !cleanup_local_dirs {
    |                 ------------------ `cleanup_local_dirs` is borrowed here
help: to force the closure to take ownership of `cleanup_local_dirs` (and any other referenced variables), use the `move` keyword
    |
124 |       .and_then(move |(output, workdir)| {
    |                 ^^^^^^^^^^^^^^^^^^^^^^^^
.map(|(result, _workdir)| result)
.map(move |result| (result, workdir, cleanup_local_dirs) )
.map(move |(result, workdir, cleanup_local_dirs)| {
if cleanup_local_dirs == false {

This comment has been minimized.

@illicitonion

illicitonion Jun 26, 2018

Contributor

if !cleanup_local_dirs {

This comment has been minimized.

@kwlzn

kwlzn Jun 27, 2018

Member

done

// This consumes the `TempDir` without deleting directory on the filesystem, meaning
// that the temporary directory will no longer be automatically deleted when dropped.
let preserved_path = workdir.into_path();
info!("preserved local process execution dir: {:?}", preserved_path.into_os_string());

This comment has been minimized.

@illicitonion

illicitonion Jun 26, 2018

Contributor

May also want to include the execution request (or at least the description field from it) in this log, to help people know which directory they're interested in

This comment has been minimized.

@kwlzn

kwlzn Jun 27, 2018

Member

done

// This consumes the `TempDir` without deleting directory on the filesystem, meaning
// that the temporary directory will no longer be automatically deleted when dropped.
let preserved_path = workdir.into_path();
info!("preserved local process execution dir: {:?}", preserved_path.into_os_string());

This comment has been minimized.

@illicitonion

illicitonion Jun 26, 2018

Contributor

.into_os_string() should be unnecessary here; Path implements Debug. (Or you can call presersved_path.display() and format with {} if you prefer)

This comment has been minimized.

@stuhood

stuhood Jun 26, 2018

Member

Path does not implement Display (for unicodey reasons), but os_string does on Unix. So a third option is:

info!("preserved local process execution dir: {}", preserved_path.as_os_str());

This comment has been minimized.

@kwlzn

kwlzn Jun 27, 2018

Member

done

fn run_command_locally(
req: ExecuteProcessRequest,
) -> Result<FallibleExecuteProcessResult, String> {
run_command_locally_in_dir(req)
let work_dir = TempDir::new().unwrap();

This comment has been minimized.

@illicitonion

illicitonion Jun 26, 2018

Contributor

This function is interesting now... work_dir, being a TempDir, will itself be rm -rf'd when this function returns, which vaguely subverts the cleanup flag.

For how this is used today, this is fine, but may be worth a comment for the future. Or maybe not :)

This comment has been minimized.

@kwlzn

kwlzn Jun 27, 2018

Member

yeah, completely intentional for this variant of the helper based on how it's being used.

) -> Result<FallibleExecuteProcessResult, String> {
let store_dir = TempDir::new().unwrap();
let pool = Arc::new(fs::ResettablePool::new("test-pool-".to_owned()));
let store = fs::Store::local_only(store_dir.path(), pool.clone()).unwrap();
let runner = super::CommandRunner {
store: store,
fs_pool: pool,
work_dir: dir.clone(),

This comment has been minimized.

@illicitonion

illicitonion Jun 26, 2018

Contributor

Shouldn't need this .clone()

This comment has been minimized.

@kwlzn

kwlzn Jun 27, 2018

Member

done

Arg::with_name("work-dir")
.long("work-dir")
.takes_value(true)
.required(true)

This comment has been minimized.

@illicitonion

illicitonion Jun 26, 2018

Contributor

Let's remove the .required(true), and default this to std::env::temp_dir() by on line 106 doing a .or_else(std::env::temp_dir) call instead of .unwrap()

This comment has been minimized.

@kwlzn

kwlzn Jun 27, 2018

Member

done

None => Box::new(process_execution::local::CommandRunner::new(
store,
pool,
work_dir.clone(),

This comment has been minimized.

@illicitonion

illicitonion Jun 26, 2018

Contributor

Shouldn't need this .clone()

This comment has been minimized.

@kwlzn

kwlzn Jun 27, 2018

Member

done

@@ -94,6 +95,8 @@ impl Core {
None => Box::new(process_execution::local::CommandRunner::new(
store.clone(),
fs_pool.clone(),
work_dir.clone().to_path_buf(),

This comment has been minimized.

@illicitonion

illicitonion Jun 26, 2018

Contributor

I have a personal style preference that if a function needs to own some data, it should take an owned version of it, rather than taking a reference and cloning it, because that's more upfront about the cost. So personally I would change this function signature to take a PathBuf instead of a &Path, rather than doing the cloning here. In this case, it avoids the clone entirely, because the caller is actually doing: work_dir_buf.to_os_string().as_ref(), so rather than doing work_dir_buf.to_os_string().as_ref().clone().to_path_buf() we should end up just doing PathBuf::from(work_dir_buf.to_os_string()) and being happy.

(Also, I think work_dir.clone().to_path_buf() could be re-written as just work_dir.to_path_buf(), but the above is probably better :))

This comment has been minimized.

@kwlzn

kwlzn Jun 27, 2018

Member

not seeing the &Path you're referring to here?

but went with work_dir.to_path_buf() instead.

This comment has been minimized.

@illicitonion

illicitonion Jun 27, 2018

Contributor

Aah, I meant the actual work_dir arg; if you change its type to PathBuf, and have scheduler_create call PathBuf::from(work_dir.to_os_string()) instead of work_dir_buf.to_os_string().as_ref(), you can save a copy, and express intent a little more cleanly

.map(|dirent| dirent.unwrap().path().to_owned())
.collect();

let subdirs = testutil::file::list_dir(&preserved_work_root);
assert_eq!(subdirs.len(), 1);

// Then look for a file like e.g. `/tmp/abc1234/process-execution7zt4pH/roland`
let mut rolands_path = preserved_work_root.clone();

This comment has been minimized.

@illicitonion

illicitonion Jun 27, 2018

Contributor

let rolands_path = preserved_work_dir.join(subdirs[0]).join("roland");

This comment has been minimized.

@kwlzn

kwlzn Jun 28, 2018

Member

👍

let work_dir = PathBuf::from(args.value_of("work-dir").unwrap());
let work_dir = args
.value_of("work-dir")
.map(|s| PathBuf::from(s))

This comment has been minimized.

@illicitonion

illicitonion Jun 27, 2018

Contributor

.map(PathBuf::from)

This comment has been minimized.

@kwlzn

kwlzn Jun 28, 2018

Member

ah, right.

@@ -94,6 +95,8 @@ impl Core {
None => Box::new(process_execution::local::CommandRunner::new(
store.clone(),
fs_pool.clone(),
work_dir.clone().to_path_buf(),

This comment has been minimized.

@illicitonion

illicitonion Jun 27, 2018

Contributor

Aah, I meant the actual work_dir arg; if you change its type to PathBuf, and have scheduler_create call PathBuf::from(work_dir.to_os_string()) instead of work_dir_buf.to_os_string().as_ref(), you can save a copy, and express intent a little more cleanly

@kwlzn kwlzn force-pushed the twitter:kwlzn/5996 branch from 7c9c728 to 9a52d30 Jun 28, 2018

@stuhood
Copy link
Member

stuhood left a comment

Thanks.

@@ -209,7 +209,8 @@
uint64_t,
uint64_t,
uint64_t,
uint64_t);
uint64_t,
uint8_t);

This comment has been minimized.

@stuhood

stuhood Jun 28, 2018

Member

Can use _Bool for this.

This comment has been minimized.

@kwlzn

kwlzn Jun 28, 2018

Member

fixed

@kwlzn kwlzn merged commit 46e1be5 into pantsbuild:master Jun 28, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment