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
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 1 addition & 3 deletions .bazelci/presubmit.yml
Original file line number Diff line number Diff line change
Expand Up @@ -30,9 +30,7 @@ platforms:
- "//test/..."
- "@examples//..."
- "-//test/conflicting_deps:conflicting_deps_test"
# Runfiles currently not working properly with remote execution
# https://github.com/bazelbuild/rules_rust/issues/112
- "-@examples//hello_runfiles:hello_runfiles_test"
# rust_doc_test is likely not fully sandboxed
- "-@examples//fibonacci:fibonacci_doc_test"
- "-@examples//hello_lib:hello_lib_doc_test"
- "-//tools/runfiles:runfiles_doc_test"
6 changes: 6 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
@@ -1,2 +1,8 @@
# vim
*.swp

# bazel
/bazel-*

# rustfmt
*.rs.bk
2 changes: 0 additions & 2 deletions examples/ffi/c_calling_rust/BUILD
Original file line number Diff line number Diff line change
@@ -1,5 +1,3 @@
package(default_visibility = ["//visibility:private"])

load("@//rust:rust.bzl", "rust_library", "rust_test", "rust_binary")

rust_library(
Expand Down
2 changes: 0 additions & 2 deletions examples/fibonacci/BUILD
Original file line number Diff line number Diff line change
@@ -1,5 +1,3 @@
package(default_visibility = ["//visibility:public"])

load(
"@io_bazel_rules_rust//rust:rust.bzl",
"rust_library",
Expand Down
2 changes: 0 additions & 2 deletions examples/hello_out_dir/BUILD
Original file line number Diff line number Diff line change
@@ -1,5 +1,3 @@
package(default_visibility = ["//visibility:public"])

load(
"@io_bazel_rules_rust//rust:rust.bzl",
"rust_binary",
Expand Down
19 changes: 4 additions & 15 deletions examples/hello_runfiles/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -2,25 +2,14 @@ package(default_visibility = ["//visibility:public"])

load(
"@io_bazel_rules_rust//rust:rust.bzl",
"rust_library",
"rust_binary",
"rust_library",
"rust_test",
)

rust_library(
name = "runfiles",
srcs = ["src/lib.rs"],
)

rust_binary(
name = "hello_runfiles",
srcs = ["src/main.rs"],
data = ["data/sample.txt"],
deps = [":runfiles"],
)

rust_test(
name = "hello_runfiles_test",
data = ["data/sample.txt"],
deps = [":runfiles"],
srcs = ["hello_runfiles.rs"],
data = ["hello_runfiles.rs"], # Yes, we're being cute.
deps = ["@io_bazel_rules_rust//tools/runfiles"],
)
18 changes: 18 additions & 0 deletions examples/hello_runfiles/hello_runfiles.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
extern crate runfiles;

use std::io::prelude::*;
use std::fs::File;

use runfiles::Runfiles;

fn main() {
let r = Runfiles::create().unwrap();

let mut f = File::open(r.rlocation("examples/hello_runfiles/hello_runfiles.rs")).unwrap();

let mut buffer = String::new();
f.read_to_string(&mut buffer).unwrap();

assert_eq!(buffer.len(), 427);
println!("This program's source is:\n```\n{}\n```", buffer);
}
43 changes: 0 additions & 43 deletions examples/hello_runfiles/src/lib.rs

This file was deleted.

16 changes: 0 additions & 16 deletions examples/hello_runfiles/src/main.rs

This file was deleted.

23 changes: 23 additions & 0 deletions tools/runfiles/BUILD
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
load(
"@io_bazel_rules_rust//rust:rust.bzl",
"rust_doc_test",
"rust_library",
"rust_test",
)

rust_library(
name = "runfiles",
srcs = ["runfiles.rs"],
visibility = ["//visibility:public"],
)

rust_test(
name = "runfiles_test",
data = ["data/sample.txt"],
deps = [":runfiles"],
)

rust_doc_test(
name = "runfiles_doc_test",
dep = ":runfiles",
)
File renamed without changes.
129 changes: 129 additions & 0 deletions tools/runfiles/runfiles.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,129 @@
//! Runfiles lookup library for Bazel-built Rust binaries and tests.
//!
//! USAGE:
//!
//! 1. Depend on this runfiles library from your build rule:
//! ```python
//! rust_binary(
//! name = "my_binary",
//! ...
//! data = ["//path/to/my/data.txt"],
//! deps = ["@io_bazel_rules_rust//tools/runfiles"],
//! )
//! ```
//!
//! 2. Import the runfiles library.
//! ```
//! extern crate runfiles;
//!
//! use runfiles::Runfiles;
//! ```
//!
//! 3. Create a Runfiles object and use rlocation to look up runfile paths:
//! ```ignore -- This doesn't work under rust_doc_test because argv[0] is not what we expect.
//!
//! use runfiles::Runfiles;
//!
//! let r = Runfiles::create().unwrap();
//! let path = r.rlocation("my_workspace/path/to/my/data.txt");
//!
//! let f = File::open(path).unwrap();
//! // ...
//! ```

use std::io;
use std::fs;
use std::path::PathBuf;
use std::path::Path;
use std::env;

pub struct Runfiles {
runfiles_dir: PathBuf,
}

impl Runfiles {
/// 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

Ok(Runfiles { runfiles_dir: find_runfiles_dir()? })
}

/// 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.

/// validity and that the path exists.
pub fn rlocation(&self, path: impl AsRef<Path>) -> PathBuf {
let path = path.as_ref();
if path.is_absolute() {
return path.to_path_buf();
}
self.runfiles_dir.join(path)
}
}

/// Returns the .runfiles directory for the currently executing binary.
fn find_runfiles_dir() -> io::Result<PathBuf> {
let exec_path = std::env::args().nth(0).expect("arg 0 was not set");

let mut binary_path = PathBuf::from(&exec_path);
loop {
// Check for our neighboring $binary.runfiles directory.
let mut runfiles_name = binary_path.file_name().unwrap().to_owned();
runfiles_name.push(".runfiles");

let runfiles_path = binary_path.with_file_name(&runfiles_name);
if runfiles_path.is_dir() {
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.

{
// TODO: 1.28 adds Path::ancestors() which is a little simpler.
let mut next = binary_path.parent();
while let Some(ancestor) = next {
if ancestor.file_name().map_or(false, |f| {
f.to_string_lossy().ends_with(".runfiles")
})
{
return Ok(ancestor.to_path_buf());
}
next = ancestor.parent();
}
}

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.

}
// Follow symlinks and keep looking.
binary_path = binary_path.read_link()?;
if binary_path.is_relative() {
binary_path = env::current_dir()?.join(binary_path)
}
}

panic!("Failed to find .runfiles directory.");
}

#[cfg(test)]
mod test {
use super::*;

use std::io::prelude::*;
use std::fs::File;

#[test]
fn test_can_read_data_from_runfiles() {
let r = Runfiles::create().unwrap();

let mut f = File::open(r.rlocation(
"io_bazel_rules_rust/tools/runfiles/data/sample.txt",
)).unwrap();

let mut buffer = String::new();
f.read_to_string(&mut buffer).unwrap();

assert_eq!("Example Text!", buffer);
}
}