Skip to content

Find the program using PATHEXT #37381

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.

Sign up for GitHub

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

Closed
wants to merge 10 commits into from
Closed

Find the program using PATHEXT #37381

Changes from 1 commit
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
40ed572
Find the program using PATHEXT
Oct 24, 2016
cc9d064
Added env_lookup fn
Oct 25, 2016
a3c72fd
Added tests for Command::env_lookup()
Oct 26, 2016
63b4c79
Hoist the PATHEXT env lookup
Oct 26, 2016
f364190
Fixed some nitpicks for better code quality
Oct 26, 2016
41a0df0
Added process_command tests for find_program
Oct 26, 2016
6b9b6f1
On batch files set ApplicationName to `cmd.exe`
Oct 28, 2016
abe5e1a
Remove find_program tests
Oct 31, 2016
0444a6d
Add as_vec_u16 fn
Oct 31, 2016
e9e960e
Add tests for make_command_line modifications
Nov 1, 2016
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
29 changes: 23 additions & 6 deletions src/libstd/sys/windows/process.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,12 +125,8 @@ impl Command {
self.stderr = Some(stderr);
}

pub fn spawn(&mut self, default: Stdio, needs_stdin: bool)
-> io::Result<(Process, StdioPipes)> {
// To have the spawning semantics of unix/windows stay the same, we need
// to read the *child's* PATH if one is provided. See #15149 for more
// details.
let program = self.env.as_ref().and_then(|env| {
pub fn find_program(&mut self) -> Option<Path> {
self.env.as_ref().and_then(|env| {
for (key, v) in env {
if OsStr::new("PATH") != &**key { continue }

Expand All @@ -141,12 +137,33 @@ impl Command {
.with_extension(env::consts::EXE_EXTENSION);
if fs::metadata(&path).is_ok() {
return Some(path.into_os_string())
} else {
Copy link
Member

Choose a reason for hiding this comment

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

We typically try to avoid rightward drift where possible, so because of the return above you can omit this else, de-indenting what's below. You can also change what's below to:

let exts = match env_pathext {
    Some(e) => e,
    None => continue,
};
// ...

to get rid of another layer of indentation.

// Windows relies on path extensions to resolve commands.
// Path extensions are found in the PATHEXT environment variable.
if let Some(exts) = self.env.get("PATHEXT") {
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps this lookup could be hoisted out?

for ext in split_paths(&exts) {
let ext_str = pathext.to_str().unwrap().trim_matches('.');
Copy link
Member

Choose a reason for hiding this comment

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

We should avoid an unwrap() here and just skip anything that can't be turned into a string

Copy link
Author

Choose a reason for hiding this comment

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

Sounds good to me! What about using to_string_lossy() instead? Something like this:

                            for ext in split_paths(&exts) {
                                let ext_str = ext.to_string_lossy();
                                let path = path.with_extension(
                                                ext_str.trim_matches('.')
                                );
                                if fs::metadata(&path).is_ok() {
                                    return Some(path.into_os_string())
                                }
                            }

let path = path.join(self.program.to_str().unwrap())
Copy link
Member

Choose a reason for hiding this comment

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

Here I think you just need to call with_extension as otherwise this is joining self.program onto the root path twice I think.

.with_extension(ext_str);
if fs::metadata(&path).is_ok() {
return Some(path.into_os_string())
}
}
}
}
}
break
}
None
});
}

pub fn spawn(&mut self, default: Stdio, needs_stdin: bool)
-> io::Result<(Process, StdioPipes)> {
// To have the spawning semantics of unix/windows stay the same, we need
// to read the *child's* PATH if one is provided. See #15149 for more
// details.
let program = self.find_program();

let mut si = zeroed_startupinfo();
si.cb = mem::size_of::<c::STARTUPINFO>() as c::DWORD;
Expand Down