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

Implement directory based runfiles lookup [#130] #132

Merged
merged 9 commits into from
Oct 20, 2018

Conversation

mfarrugi
Copy link
Collaborator

@mfarrugi mfarrugi commented Oct 19, 2018

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.

Copy link
Collaborator

@acmcarther acmcarther left a 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> {
Copy link
Collaborator

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:

Suggested change
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).

Copy link
Collaborator Author

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.

Copy link
Collaborator

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
Copy link
Collaborator

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.

Copy link
Collaborator Author

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.

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;
Copy link
Collaborator

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.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why would this happen?

Copy link
Collaborator Author

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;
Copy link
Collaborator

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?

Copy link
Collaborator Author

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.

@mfarrugi
Copy link
Collaborator Author

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?

@acmcarther
Copy link
Collaborator

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:
examples/runfiles/hello_world.txt
hello world!
examples/runfiles/example.rs

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)

@mfarrugi mfarrugi merged commit 1678ed0 into bazelbuild:master Oct 20, 2018
@mfarrugi mfarrugi deleted the marco-fix-runfiles branch October 19, 2020 03:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fixup and Export runfiles library Runfiles Tests are failing on remote execution
3 participants