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.

Sign up for GitHub

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

Jump to bottom

Fix regression in unmatched globs error message #10595

Merged
merged 1 commit into from Aug 12, 2020
Merged

Fix regression in unmatched globs error message #10595

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
72 changes: 28 additions & 44 deletions src/rust/engine/src/intrinsics.rs
View file
Open in desktop
Expand Up @@ -213,16 +213,15 @@ fn remove_prefix_request_to_digest(
let store = core.store();

async move {
let input_digest = lift_digest(&externs::project_ignoring_type(&args[0], "digest"))?;
let input_digest =
lift_digest(&externs::project_ignoring_type(&args[0], "digest")).map_err(|e| throw(&e))?;
Comment on lines -216 to +217
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that this change and most others are not necessary. The only thing we need is the fix for download_file_to_digest and path_globs_to_digest.

But, personally, I think I find this refactor easier to follow and that it has less coercion of types, like no let Result<_, String>. Still, let me know if this is actually an improvement or I should revert to the old.

Copy link
Contributor

Choose a reason for hiding this comment

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

The one concern I have about this hunk is that it seems to add two throw() conversions where there were none before. Is it possible to keep those as string errors and then re-use the .map_err() at the bottom? Or is the point of this part that you have to add those two throw()s in order to remove the let res statement?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think removing the let res is clearer regardless, so I'm fine if we make this tradeoff.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is only part of the refactor. The end effect should be the same, so it's only a question of what's clearer here.

I added comments on the two code blocks that are the actual fixes. Everything else can be reverted if preferred.

Pardon that I didn't make that clearer from the start, and thank you for the prompt feedback :)

let prefix = externs::project_str(&args[0], "prefix");
let digest = store
.strip_prefix(input_digest, PathBuf::from(prefix))
.await
.map_err(|e| format!("{:?}", e))?;
let res: Result<_, String> = Ok(Snapshot::store_directory(&core, &digest));
res
.map_err(|e| throw(&format!("{:?}", e)))?;
Ok(Snapshot::store_directory(&core, &digest))
}
.map_err(|e: String| throw(&e))
.boxed()
}

Expand All @@ -233,16 +232,15 @@ fn add_prefix_request_to_digest(
let core = context.core;
let store = core.store();
async move {
let input_digest = lift_digest(&externs::project_ignoring_type(&args[0], "digest"))?;
let input_digest =
lift_digest(&externs::project_ignoring_type(&args[0], "digest")).map_err(|e| throw(&e))?;
let prefix = externs::project_str(&args[0], "prefix");
let digest = store
.add_prefix(input_digest, PathBuf::from(prefix))
.await
.map_err(|e| format!("{:?}", e))?;
let res: Result<_, String> = Ok(Snapshot::store_directory(&core, &digest));
res
.map_err(|e| throw(&format!("{:?}", e)))?;
Ok(Snapshot::store_directory(&core, &digest))
}
.map_err(|e: String| throw(&e))
.boxed()
}

Expand Down Expand Up @@ -270,13 +268,11 @@ fn merge_digests_request_to_digest(
.collect();
async move {
let digest = store
.merge(digests?)
.merge(digests.map_err(|e| throw(&e))?)
.await
.map_err(|e| format!("{:?}", e))?;
let res: Result<_, String> = Ok(Snapshot::store_directory(&core, &digest));
res
.map_err(|e| throw(&format!("{:?}", e)))?;
Ok(Snapshot::store_directory(&core, &digest))
}
.map_err(|err: String| throw(&err))
.boxed()
}

Expand All @@ -286,15 +282,10 @@ fn download_file_to_digest(
) -> BoxFuture<'static, NodeResult<Value>> {
let core = context.core.clone();
async move {
let key = externs::acquire_key_for(args.pop().unwrap()).map_err(|e| format!("{:?}", e))?;
let snapshot = context
.get(DownloadedFile(key))
.await
.map_err(|e| format!("{:?}", e))?;
let res: Result<_, String> = Ok(Snapshot::store_directory(&core, &snapshot.digest));
res
let key = externs::acquire_key_for(args.pop().unwrap())?;
let snapshot = context.get(DownloadedFile(key)).await?;
Ok(Snapshot::store_directory(&core, &snapshot.digest))
}
.map_err(|err: String| throw(&err))
.boxed()
}
Comment on lines 284 to 290
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actual fix for DownloadFile.


Expand All @@ -304,15 +295,10 @@ fn path_globs_to_digest(
) -> BoxFuture<'static, NodeResult<Value>> {
let core = context.core.clone();
async move {
let key = externs::acquire_key_for(args.pop().unwrap()).map_err(|e| format!("{:?}", e))?;
let digest = context
.get(Snapshot(key))
.await
.map_err(|e| format!("{:?}", e))?;
let res: Result<_, String> = Ok(Snapshot::store_directory(&core, &digest));
res
let key = externs::acquire_key_for(args.pop().unwrap())?;
let digest = context.get(Snapshot(key)).await?;
Ok(Snapshot::store_directory(&core, &digest))
}
.map_err(|err: String| throw(&err))
Comment on lines -307 to -315
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actual fix for PathGlobs.

.boxed()
}

Expand Down Expand Up @@ -343,12 +329,13 @@ fn create_digest_to_digest(
let store = context.core.store();

async move {
let digests = future::try_join_all(digests).await?;
let digest = store.merge(digests).await.map_err(|e| format!("{:?}", e))?;
let res: Result<_, String> = Ok(Snapshot::store_directory(&context.core, &digest));
res
let digests = future::try_join_all(digests).await.map_err(|e| throw(&e))?;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this the part that fixes it (because we don't call format!() on the inside and then throw() on the outside)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, only a refactor.

let digest = store
.merge(digests)
.await
.map_err(|e| throw(&format!("{:?}", e)))?;
Ok(Snapshot::store_directory(&context.core, &digest))
}
.map_err(|err: String| throw(&err))
.boxed()
}

Expand All @@ -360,18 +347,15 @@ fn digest_subset_to_digest(
let store = context.core.store();

async move {
let path_globs = Snapshot::lift_path_globs(&globs)?;
let original_digest = lift_digest(&externs::project_ignoring_type(&args[0], "digest"))?;
let path_globs = Snapshot::lift_path_globs(&globs).map_err(|e| throw(&e))?;
let original_digest =
lift_digest(&externs::project_ignoring_type(&args[0], "digest")).map_err(|e| throw(&e))?;
let subset_params = SubsetParams { globs: path_globs };

let digest = store
.subset(original_digest, subset_params)
.await
.map_err(|e| format!("{:?}", e))?;

let res: Result<_, String> = Ok(Snapshot::store_directory(&context.core, &digest));
res
.map_err(|e| throw(&format!("{:?}", e)))?;
Ok(Snapshot::store_directory(&context.core, &digest))
}
.map_err(|err: String| throw(&err))
.boxed()
}