rules_rust
rules_rust copied to clipboard
bazelbuild
Runfiles should return `Result` instead of panicing
I haven't been keeping up too closely with the recent changes to runfiles but I wanna make sure the interface doesn't have panics in it and that we allow users to handle results at runtime.
https://github.com/bazelbuild/rules_rust/blob/0.44.0/tools/runfiles/runfiles.rs#L146-L155
This seems concerning. In practice does it not ever get hit?
Looks like this dates all the way back to https://github.com/bazelbuild/rules_rust/commit/b8219ac7f74d06c4983e96a1b9d0dbe00d7a52c7 from 3 years ago. This is only hit if you try to lookup a path to a missing file when using the manifest strategy. I'd imagine this is not a very common use case - if you need the file and don't have it, you're typically out of luck, and I expect virtually all usage will just unwrap the result anyway. But I suppose we could offer a secondary result-based API if anyone does want to do this conditional lookup and handle it more carefully?
A Result type generally has value when we think that a failure is expected (and not a bug). In this case, if the file doesn't exist in the manifest, it's pretty much guaranteed to be a bug, rather than an actual error (since we already validated that the runfiles directory exists on creation).
I agree with @DavidZbarsky-at that offering a secondary API seems reasonable.
FWIW, I really don't like how all implementations of runfiles (not just rules_rust's) fail if a file is missing for manifest-based runfiles, but succeed for directory based runfiles. This leads to an interesting consequence where directory based runfiles can use runfiles to look up directories, but manifest based ones can't.
I'd be ok with making a breaking change for this, but at least offering an alternative would make sense - not sure how others feel...
I'd prefer not breaking here - I think providing an alternative would make sense.
Alternatively, we may want to consider bringing up with the bazel team the possibility of changing the spec so that if the file doesn't exist it returns it anyway, since it's a source of inconsistency between manifest-based runfiles and directory-based runfiles.
I think the current state of things with the panic should be considered a critical oversight. The library itself should not make the assumption that if the rlocationpath the user asked for doesn't exist, then the entire process should exit (which is the implication of a panic). In both manifest-based and directory-based runfiles, I'm proposing Result is returned and it it's up to the user to decide what to do with that.
Alternatively, we may want to consider bringing up with the bazel team the possibility of changing the spec so that if the file doesn't exist it returns it anyway, since it's a source of inconsistency between manifest-based runfiles and directory-based runfiles.
Can you outline the behavior you expect in each case?
Directory based runfiles just returns the path. If the file doesn't exist in the manifest, then path.is_file will return false, but the command will still succeed
I have created https://github.com/bazelbuild/rules_rust/pull/2847 which converts panics to Result. The change mostly affects manifest-based runfiles which used to panic when a missing runfile was requested but in general the larger change is that rlocation! now returns an Option. If there are major concerns with this please let me know!