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.

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

rustc: Try to contain prepends to PATH #34083

Merged
merged 1 commit into from
Jun 8, 2016
+16 −4
Merged

rustc: Try to contain prepends to PATH #34083

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
20 changes: 16 additions & 4 deletions src/librustc_driver/driver.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -669,12 +669,24 @@ pub fn phase_2_configure_and_expand<'a>(sess: &Session,
// dependent dlls. Note that this uses cfg!(windows) as opposed to
// targ_cfg because syntax extensions are always loaded for the host
// compiler, not for the target.
let mut _old_path = OsString::new();
//
// This is somewhat of an inherently racy operation, however, as
// multiple threads calling this function could possibly continue
// extending PATH far beyond what it should. To solve this for now we
// just don't add any new elements to PATH which are already there
// within PATH. This is basically a targeted fix at #17360 for rustdoc
// which runs rustc in parallel but has been seen (#33844) to cause
// problems with PATH becoming too long.
let mut old_path = OsString::new();
Copy link
Contributor

Choose a reason for hiding this comment

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

Aren't you getting the "variable not used" warning on non-Windows platforms without the underscore?

Copy link
Member Author

Choose a reason for hiding this comment

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

Nah by using if instead of #[cfg] it's the same as doing this on Unix:

if false {
    let foo = bar;
    something(foo);
}

which doesn't emit an unused variable warning.

if cfg!(windows) {
_old_path = env::var_os("PATH").unwrap_or(_old_path);
old_path = env::var_os("PATH").unwrap_or(old_path);
let mut new_path = sess.host_filesearch(PathKind::All)
.get_dylib_search_paths();
new_path.extend(env::split_paths(&_old_path));
for path in env::split_paths(&old_path) {
if !new_path.contains(&path) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Although this makes the for loop O(n^2), it is fine IMO because new_path shouldn't have too many entries.

Copy link
Contributor

Choose a reason for hiding this comment

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

No, it's repeatedly adding up!

Copy link
Contributor

@xen0n xen0n Jun 5, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

I would do something like:

let old_path = env::split_paths(&old_path).filter(|p| !new_path.contains(p)).collect();
new_path.extend(old_path);

This collect is necessary to make sure all new_path queries are looking at its pristine state. Or make another HashSet from new_path just to be explicit and O(1), but I'd not go that far. (Consulting a constant list is also O(1) actually.)

Copy link
Member Author

Choose a reason for hiding this comment

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

No, when run concurrently everything is adding the same paths, so it doesn't add up. There's no way this is more expensive than, say, reading the source file.

Copy link
Contributor

Choose a reason for hiding this comment

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

Fair point! Premature optimization is evil, yeah? 😈

new_path.push(path);
}
}
env::set_var("PATH", &env::join_paths(new_path).unwrap());
}
let features = sess.features.borrow();
Expand All @@ -694,7 +706,7 @@ pub fn phase_2_configure_and_expand<'a>(sess: &Session,
syntax_exts,
krate);
if cfg!(windows) {
env::set_var("PATH", &_old_path);
env::set_var("PATH", &old_path);
}
*sess.available_macros.borrow_mut() = macro_names;
ret
Expand Down