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

Add a FileDigest type to differentiate file APIs from directory APIs #10900

Merged
merged 2 commits into from Oct 3, 2020

Conversation

stuhood
Copy link
Sponsor Member

@stuhood stuhood commented Oct 3, 2020

Problem

Two codepaths in our @rule API use a Digest for a file rather than a directory, and this can cause confusion, since using a Digest of the wrong type will trigger a failure to lookup a digest in storage. More generally, our Python API has so far been more typesafe than our Rust API with regard to Digests: fairly consistently using them to represent Directorys.

Solution

Introduce FileDigest, and use it in the two relevant locations:

  1. DownloadFile takes the FileDigest for the file being downloaded.
  2. single_file_digests_to_bytes takes FileDigests returned in workunits.artifacts (see Fix retrieval of bytes from artifacts in StreamingWorkunitHandler #10698).

Result

Improved type safety and clarity. Fixes #10898.

[ci skip-build-wheels]

…irectory" (`Digest`) and "digest of a file" (`FileDigest`).

[ci skip-build-wheels]
Comment on lines +225 to +227
if types.file_digest != externs::get_type_for(digest) {
return Err(format!("{} is not of type {}.", digest, types.file_digest));
}
Copy link
Sponsor Member Author

@stuhood stuhood Oct 3, 2020

Choose a reason for hiding this comment

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

A lot of the churn in this PR is due to the addition of these type checks, because it required threading the Types instance in a few more places.

I expect that in the medium term we should consider moving all of the fs.py types to Rust using cpython's class generation mechanism, as it's likely both safer (better error messages) and more efficient than the externs::project_* boilerplate:

py_class!(class PyExecutionStrategyOptions |py| {
data options: ExecutionStrategyOptions;
def __new__(
_cls,
local_parallelism: u64,
remote_parallelism: u64,
cleanup_local_dirs: bool,
speculation_delay: f64,
speculation_strategy: String,
use_local_cache: bool,
local_enable_nailgun: bool
) -> CPyResult<Self> {
Self::create_instance(py,
ExecutionStrategyOptions {
local_parallelism: local_parallelism as usize,
remote_parallelism: remote_parallelism as usize,
cleanup_local_dirs,
speculation_delay: Duration::from_millis((speculation_delay * 1000.0).round() as u64),
speculation_strategy,
use_local_cache,
local_enable_nailgun,
}
)
}
});

See http://dgrunwald.github.io/rust-cpython/doc/cpython/macro.py_class.html for more info.

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
@stuhood stuhood merged commit 787611d into pantsbuild:master Oct 3, 2020
@stuhood stuhood deleted the stuhood/add-and-use-file-digest branch October 3, 2020 05:26
@coveralls
Copy link

Coverage Status

Coverage remained the same at 0.0% when pulling 60c4acb on stuhood:stuhood/add-and-use-file-digest into 4129e06 on pantsbuild:master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Mixed use of Digest type can be confusing
3 participants