-
Notifications
You must be signed in to change notification settings - Fork 426
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
Implement directory based runfiles lookup [#130] #132
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for implementing this!
Comments below are mostly nits, but I did want to ask about having some example usage of this in @examples//
. Or is that waiting on some other change?
/// Creates a directory based Runfiles object. | ||
/// | ||
/// Manifest based creation is not currently supported. | ||
pub fn create() -> io::Result<Self> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I dont feel strongly about this, but we probably should conform to Rust idioms:
pub fn create() -> io::Result<Self> { | |
pub fn new() -> io::Result<Self> { |
Unsure what Rust zeitgeist has to say about new
methods not returning Self directly (though it's obviously justified here).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
runfiles.py struck me as unidiomatic as well, so I leaned towards looking the same as other runfiles libraries. I also don't feel strongly about this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, matching python (in defying local norms) SGTM
/// Returns the runtime path of a runfile. | ||
/// | ||
/// Runfiles are data-dependencies of Bazel-built binaries and tests. | ||
/// The returned path may not be valid. The caller should check the path's |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Under what conditions would the path not be valid? I'm unclear on whether this just refers to non-existent paths, or if some bazel-related circumstances can arise to cause a file not to appear.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this means that you can call rlocation on some path, and it will return xxx.runfiles/ + path
regardless of path exists, depending on the type of Runfiles object.
tools/runfiles/runfiles.rs
Outdated
let exec_path = std::env::args().nth(0).expect("arg 0 was not set"); | ||
let exec_path = PathBuf::from(&exec_path); | ||
|
||
let mut binary_path = exec_path; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: I suspect you can remove the shadowing (which i detest, but others disagree) by just naming the PathBuf::from output to "binary_path" and making it mutable.
return Ok(runfiles_path); | ||
} | ||
|
||
// Check if we're already under a *.runfiles directory. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why would this happen?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a. If the executing binary is part of another's runfiles tree, eg. a py_binary calling out to our rust_binary.
b. Someone called the symlink under .runfiles to start the process.
I have also seen many binaries deployed with a merged .runfiles tree.
|
||
// Follow symlinks and keep looking. | ||
if !fs::symlink_metadata(&binary_path)?.file_type().is_symlink() { | ||
break; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: The control flow here strikes me as unusual. This seems to be breaking out of the loop { }
, but it isn't obvious from code comments that we've reached a terminal state -- in reality, the above comment seems to indicate the opposite.
Perhaps the comment here should be revisited?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wrote the comment before flipping the if statement, will move it down.
I removed the hello_runfiles example as part of this because it was redundant with the documentation and test in runfiles.rs -- what else would you like to see in an example? |
Regd an example: A personal usecase I have is similar to #79 -- include_bytes! for a genfile dependency. I don't think the default include! macros can take anything other than string literals though, so we'd have to go without the macro unless replacement macros were included in the crate. A really good example might be one that genrule to serialize a value into a file and then deserializes it in the example, but that would require some deps. Instead, here's a simpler one: extern crate runfiles;
use runfiles::Runfiles;
fn main() {
let fpath: PathBuf =
Runfiles::create().unwrap().rlocation("examples/runfiles/hello_world.txt")
let contents = {
let file = File::open(&fpath).unwrap();
let mut buf_reader = BufReader::new(file);
let mut contents = String::new();
buf_reader.read_to_string(&mut contents).unwrap();
contents
};
assert_eq!(contents, "hello world!");
} (% compiler errors) |
Lookup based on java and python, API is a subset of Runfiles.java and looks similar to runfiles.py as well.
I punted on manifest-based lookup because I don't quite understand the usage; it's to work around symlink issues on windows?
This should fix osx usage, but I would like to see this pass CI before merging, so blocked on #131.
This fixes #112 and resolves #130.