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

Support windows command resolution #1290

Merged
merged 3 commits into from
Sep 28, 2016
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
248 changes: 248 additions & 0 deletions components/core/src/fs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -185,6 +185,48 @@ pub fn svc_var_path(service_name: &str) -> PathBuf {
///
/// If the command represents an absolute path, then the `PATH` seaching will not be performed. If
/// no absolute path can be found for the command, then `None` is returned.
///
/// On Windows, the PATHEXT environment variable contains common extensions for commands,
/// for example allowing "docker.exe" to be found when searching for "docker".
///
/// # Examples
///
/// Behavior when the command exists on PATH:
///
/// ```
///
/// use std::env;
/// use std::fs;
/// use habitat_core::fs::find_command;
///
/// let first_path = fs::canonicalize("./tests/fixtures").unwrap();
/// let second_path = fs::canonicalize("./tests/fixtures/bin").unwrap();
/// let path_bufs = vec![first_path, second_path];
/// let new_path = env::join_paths(path_bufs).unwrap();
/// env::set_var("PATH", &new_path);
///
/// let result = find_command("bin_with_no_extension");
/// assert_eq!(result.is_some(), true);
/// ```
///
/// Behavior when the command does not exist on PATH:
///
/// ```
///
/// use std::env;
/// use std::fs;
/// use habitat_core::fs::find_command;
///
/// let first_path = fs::canonicalize("./tests/fixtures").unwrap();
/// let second_path = fs::canonicalize("./tests/fixtures/bin").unwrap();
/// let path_bufs = vec![first_path, second_path];
/// let new_path = env::join_paths(path_bufs).unwrap();
/// env::set_var("PATH", &new_path);
///
/// let result = find_command("missing");
/// assert_eq!(result.is_some(), false);
/// ```
///
pub fn find_command(command: &str) -> Option<PathBuf> {
// If the command path is absolute and a file exists, then use that.
let candidate = PathBuf::from(command);
Expand All @@ -200,6 +242,11 @@ pub fn find_command(command: &str) -> Option<PathBuf> {
let candidate = PathBuf::from(&path).join(command);
if candidate.is_file() {
return Some(candidate);
} else {
match find_command_with_pathext(&candidate) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

The match here would be unnecessary if the ; on line 202 was removed along with the None on line 211. find_command_with_pathext already returns an Option so if you just call that without the ; appended to the end you'll get the return value you're looking for!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the feedback!

I think that would only work for the first path in the path variable checked. The second part of the match None => {} is what lets things proceed on to the next path to check.

I'm going to write some tests to validate the behavior.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Nope, you're right. I looked quick and didn't notice we were in an iterator and trying to short-circuit out of it in this function as well

Some(result) => return Some(result),
None => {}
}
}
}
None
Expand All @@ -208,7 +255,208 @@ pub fn find_command(command: &str) -> Option<PathBuf> {
}
}

// Windows relies on path extensions to resolve commands like `docker` to `docker.exe`
// Path extensions are found in the PATHEXT environment variable.
// We should only search with PATHEXT if the file does not already have an extension.
fn find_command_with_pathext(candidate: &PathBuf) -> Option<PathBuf> {
if candidate.extension().is_none() {
match henv::var_os("PATHEXT") {
Some(pathexts) => {
for pathext in env::split_paths(&pathexts) {
let mut source_candidate = candidate.to_path_buf();
let extension = pathext.to_str().unwrap().trim_matches('.');
source_candidate.set_extension(extension);
let current_candidate = source_candidate.to_path_buf();
if current_candidate.is_file() {
return Some(current_candidate);
}
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Like in Ruby, the last line of a function is the return value in Rust. Short-circuiting the function with return Some(T) on 228 is great, but we should fall through to a None below this line. That would allow you to remove the ; on 233 & the explicit call to return None on 232.

These are small suggestions but they'll make the function control flow read a bit easier and be more "rust-like"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Definitely appreciate the feedback on making it more rust-like!

None => {},
};
}
None
}

/// Returns whether or not the current process is running with a root effective user id or not.
pub fn am_i_root() -> bool {
*EUID == 0u32
}

#[cfg(test)]
mod test_find_command {

use std::env;
use std::fs;
use std::path::PathBuf;
pub use super::find_command;

fn setup_pathext() {
let path_bufs = vec![PathBuf::from(".COM"), PathBuf::from(".EXE")];
let new_path = env::join_paths(path_bufs).unwrap();
env::set_var("PATHEXT", &new_path);
}

fn setup_empty_pathext() {
match env::var("PATHEXT") {
Ok(val) => env::remove_var("PATHEXT"),
Err(e) => {}
}
}

fn setup_path() {
let first_path = fs::canonicalize("./tests/fixtures").unwrap();
let second_path = fs::canonicalize("./tests/fixtures/bin").unwrap();
let path_bufs = vec![first_path, second_path];
let new_path = env::join_paths(path_bufs).unwrap();
env::set_var("PATH", &new_path);
}

mod without_pathext_set {
use super::{setup_path, setup_empty_pathext};
pub use super::find_command;

fn setup_environment() {
setup_path();
setup_empty_pathext();
}

mod argument_without_extension {
use super::{setup_environment, find_command};

#[test]
fn command_exists() {
setup_environment();
let result = find_command("bin_with_no_extension");
assert_eq!(result.is_some(), true);
}

#[test]
fn command_does_not_exist() {
setup_environment();
let result = find_command("missing");
assert_eq!(result.is_some(), false);
}

#[test]
fn command_exists_with_extension() {
setup_environment();
let result = find_command("win95_dominator");
assert_eq!(result.is_some(), false);
}
}

mod argument_with_extension {
use std::fs::canonicalize;
use super::{setup_environment, find_command};

#[test]
fn command_exists() {
setup_environment();
let result = find_command("bin_with_extension.exe");
assert_eq!(result.is_some(), true);
}

#[test]
fn command_does_not_exist() {
setup_environment();
let result = find_command("missing.com");
assert_eq!(result.is_some(), false);
}

#[test]
fn command_different_extension_does_exist() {
setup_environment();
let result = find_command("bin_with_extension.com");
assert_eq!(result.is_some(), false);
}

#[test]
fn first_command_on_path_found() {
setup_environment();
let target_path = canonicalize("./tests/fixtures/plan.sh").unwrap();
let result = find_command("plan.sh");
let found_path = result.unwrap();
assert_eq!(found_path, target_path);
}
}
}

#[cfg(target_os="windows")]
Copy link
Collaborator

Choose a reason for hiding this comment

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

What a neat way to group tests in a platform dependent way. I hadn't seen us use submodules in a test module yet but this is a nice pattern

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I started using the modules as a way to group the tests to keep the function names shorter, but it became handy when I needed to make one windows only :)

mod with_pathext_set {
use super::{setup_path, setup_pathext};
pub use super::find_command;

fn setup_environment() {
setup_path();
setup_pathext();
}

mod argument_without_extension {
use super::{setup_environment, find_command};

#[test]
fn command_exists() {
setup_environment();
let result = find_command("bin_with_no_extension");
assert_eq!(result.is_some(), true);
}

#[test]
fn command_does_not_exist() {
setup_environment();
let result = find_command("missing");
assert_eq!(result.is_some(), false);
}

#[test]
fn command_exists_with_extension_in_PATHEXT() {
setup_environment();
let result = find_command("bin_with_extension");
assert_eq!(result.is_some(), true);
}

#[test]
fn command_exists_with_extension_not_in_PATHEXT() {
setup_environment();
let result = find_command("win95_dominator");
assert_eq!(result.is_some(), false);
}
}

mod argument_with_extension {
use std::fs::canonicalize;
use super::{setup_environment, find_command};

#[test]
fn command_exists() {
setup_environment();
let result = find_command("bin_with_extension.exe");
assert_eq!(result.is_some(), true);
}

#[test]
fn command_does_not_exist() {
setup_environment();
let result = find_command("missing.com");
assert_eq!(result.is_some(), false);
}

#[test]
fn command_different_extension_does_exist() {
setup_environment();
let result = find_command("bin_with_extension.com");
assert_eq!(result.is_some(), false);
}

#[test]
fn first_command_on_path_found() {
setup_environment();
let target_path = canonicalize("./tests/fixtures/plan.sh").unwrap();
let result = find_command("plan.sh");
let found_path = result.unwrap();
assert_eq!(found_path, target_path);
}
}
}
}
2 changes: 1 addition & 1 deletion components/core/src/os/system/windows.rs
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ pub struct Uname {
}

pub fn uname() -> Result<Uname> {
Ok(Uname{
Ok(Uname {
sys_name: String::from("Windows"),
node_name: String::from("CHEF-WIN10"),
release: String::from("10.0.14915"),
Expand Down
Empty file.
Empty file.
Empty file.
Empty file.