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

Fix renaming a project using build scripts #4818

Merged
merged 1 commit into from
Dec 18, 2017
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
68 changes: 51 additions & 17 deletions src/cargo/ops/cargo_rustc/custom_build.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ use std::sync::{Mutex, Arc};
use core::PackageId;
use util::{Freshness, Cfg};
use util::errors::{CargoResult, CargoResultExt, CargoError};
use util::{internal, profile, paths};
use util::{self, internal, profile, paths};
use util::machine_message;

use super::job::Work;
Expand All @@ -27,7 +27,7 @@ pub struct BuildOutput {
/// Metadata to pass to the immediate dependencies
pub metadata: Vec<(String, String)>,
/// Paths to trigger a rerun of this build script.
pub rerun_if_changed: Vec<String>,
pub rerun_if_changed: Vec<PathBuf>,
/// Environment variables which, when changed, will cause a rebuild.
pub rerun_if_env_changed: Vec<String>,
/// Warnings generated by this build,
Expand Down Expand Up @@ -65,7 +65,7 @@ pub struct BuildScripts {

pub struct BuildDeps {
pub build_script_output: PathBuf,
pub rerun_if_changed: Vec<String>,
pub rerun_if_changed: Vec<PathBuf>,
pub rerun_if_env_changed: Vec<String>,
}

Expand Down Expand Up @@ -178,29 +178,37 @@ fn build_work<'a, 'cfg>(cx: &mut Context<'a, 'cfg>, unit: &Unit<'a>)
let pkg_name = unit.pkg.to_string();
let build_state = Arc::clone(&cx.build_state);
let id = unit.pkg.package_id().clone();
let (output_file, err_file) = {
let (output_file, err_file, root_output_file) = {
let build_output_parent = build_output.parent().unwrap();
let output_file = build_output_parent.join("output");
let err_file = build_output_parent.join("stderr");
(output_file, err_file)
let root_output_file = build_output_parent.join("root-output");
(output_file, err_file, root_output_file)
};
let root_output = cx.target_root().to_path_buf();
let all = (id.clone(), pkg_name.clone(), Arc::clone(&build_state),
output_file.clone());
output_file.clone(), root_output.clone());
let build_scripts = super::load_build_deps(cx, unit);
let kind = unit.kind;
let json_messages = cx.build_config.json_messages;

// Check to see if the build script has already run, and if it has keep
// track of whether it has told us about some explicit dependencies
let prev_output = BuildOutput::parse_file(&output_file, &pkg_name).ok();
let prev_root_output = paths::read_bytes(&root_output_file)
.and_then(|bytes| util::bytes2path(&bytes))
.unwrap_or(cmd.get_cwd().unwrap().to_path_buf());
let prev_output = BuildOutput::parse_file(
&output_file,
&pkg_name,
&prev_root_output,
&root_output,
).ok();
let deps = BuildDeps::new(&output_file, prev_output.as_ref());
cx.build_explicit_deps.insert(*unit, deps);

fs::create_dir_all(&script_output)?;
fs::create_dir_all(&build_output)?;

let root_output = cx.target_root().to_path_buf();

// Prepare the unit of "dirty work" which will actually run the custom build
// command.
//
Expand Down Expand Up @@ -266,7 +274,13 @@ fn build_work<'a, 'cfg>(cx: &mut Context<'a, 'cfg>, unit: &Unit<'a>)
// well.
paths::write(&output_file, &output.stdout)?;
paths::write(&err_file, &output.stderr)?;
let parsed_output = BuildOutput::parse(&output.stdout, &pkg_name)?;
paths::write(&root_output_file, &util::path2bytes(&root_output)?)?;
let parsed_output = BuildOutput::parse(
&output.stdout,
&pkg_name,
&root_output,
&root_output,
)?;

if json_messages {
let library_paths = parsed_output.library_paths.iter().map(|l| {
Expand All @@ -289,10 +303,17 @@ fn build_work<'a, 'cfg>(cx: &mut Context<'a, 'cfg>, unit: &Unit<'a>)
// itself to run when we actually end up just discarding what we calculated
// above.
let fresh = Work::new(move |_tx| {
let (id, pkg_name, build_state, output_file) = all;
let (id, pkg_name, build_state, output_file, root_output) = all;
let output = match prev_output {
Some(output) => output,
None => BuildOutput::parse_file(&output_file, &pkg_name)?,
None => {
BuildOutput::parse_file(
&output_file,
&pkg_name,
&prev_root_output,
&root_output,
)?
}
};
build_state.insert(id, kind, output);
Ok(())
Expand Down Expand Up @@ -321,14 +342,20 @@ impl BuildState {
}

impl BuildOutput {
pub fn parse_file(path: &Path, pkg_name: &str) -> CargoResult<BuildOutput> {
pub fn parse_file(path: &Path,
pkg_name: &str,
root_output_when_generated: &Path,
root_output: &Path) -> CargoResult<BuildOutput> {
let contents = paths::read_bytes(path)?;
BuildOutput::parse(&contents, pkg_name)
BuildOutput::parse(&contents, pkg_name, root_output_when_generated, root_output)
}

// Parses the output of a script.
// The `pkg_name` is used for error messages.
pub fn parse(input: &[u8], pkg_name: &str) -> CargoResult<BuildOutput> {
pub fn parse(input: &[u8],
pkg_name: &str,
root_output_when_generated: &Path,
root_output: &Path) -> CargoResult<BuildOutput> {
let mut library_paths = Vec::new();
let mut library_links = Vec::new();
let mut cfgs = Vec::new();
Expand Down Expand Up @@ -364,6 +391,13 @@ impl BuildOutput {
_ => bail!("Wrong output in {}: `{}`", whence, line),
};

let path = |val: &str| {
Copy link
Member

Choose a reason for hiding this comment

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

Could we, instead of post-processing after reading, do some pre-processing before writing, so as not to store absolute paths at the moment of generation at all?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hm yeah I wasn't sure what was the best option here, we don't have a serialized format for Cargo's parsed data structures for the output file so it'd have to be added, it just wasn't clear to me how much of a benefit we'd get from it.

Do you think that the preprocessed version would be less buggy though?

Copy link
Member

Choose a reason for hiding this comment

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

I don't think there are any tangible benefits one way or the other, but not having absolute paths feels slightly cleaner. But I am fine either way!

match Path::new(val).strip_prefix(root_output_when_generated) {
Ok(path) => root_output.join(path),
Err(_) => PathBuf::from(val),
}
};

match key {
"rustc-flags" => {
let (paths, links) =
Expand All @@ -372,11 +406,11 @@ impl BuildOutput {
library_paths.extend(paths.into_iter());
}
"rustc-link-lib" => library_links.push(value.to_string()),
"rustc-link-search" => library_paths.push(PathBuf::from(value)),
"rustc-link-search" => library_paths.push(path(value)),
"rustc-cfg" => cfgs.push(value.to_string()),
"rustc-env" => env.push(BuildOutput::parse_rustc_env(value, &whence)?),
"warning" => warnings.push(value.to_string()),
"rerun-if-changed" => rerun_if_changed.push(value.to_string()),
"rerun-if-changed" => rerun_if_changed.push(path(value)),
"rerun-if-env-changed" => rerun_if_env_changed.push(value.to_string()),
_ => metadata.push((key.to_string(), value.to_string())),
}
Expand Down
4 changes: 3 additions & 1 deletion src/cargo/ops/cargo_rustc/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -551,7 +551,9 @@ fn link_targets<'a, 'cfg>(cx: &mut Context<'a, 'cfg>,
#[cfg(windows)]
use std::os::windows::fs::symlink_dir as symlink;

symlink(src, dst)
let dst_dir = dst.parent().unwrap();
assert!(src.starts_with(dst_dir));
symlink(src.strip_prefix(dst_dir).unwrap(), dst)
} else {
fs::hard_link(src, dst)
};
Expand Down
105 changes: 105 additions & 0 deletions tests/build-script.rs
Original file line number Diff line number Diff line change
@@ -1,8 +1,10 @@
extern crate cargotest;
extern crate hamcrest;

use std::env;
use std::fs::{self, File};
use std::io::prelude::*;
use std::path::PathBuf;

use cargotest::{rustc_host, sleep_ms};
use cargotest::support::{project, execs};
Expand Down Expand Up @@ -2794,3 +2796,106 @@ package `a v0.5.0 (file://[..])`
also links to native library `a`
"));
}

#[test]
fn rename_with_link_search_path() {
let p = project("foo")
.file("Cargo.toml", r#"
[project]
name = "foo"
version = "0.5.0"
authors = []

[lib]
crate-type = ["cdylib"]
"#)
.file("src/lib.rs", "
#[no_mangle]
pub extern fn cargo_test_foo() {}
");
let p = p.build();

assert_that(p.cargo("build"), execs().with_status(0));

let p2 = project("bar")
.file("Cargo.toml", r#"
[project]
name = "bar"
version = "0.5.0"
authors = []
"#)
.file("build.rs", r#"
use std::env;
use std::fs;
use std::path::PathBuf;

fn main() {
// Move the `libfoo.so` from the root of our project into the
// build directory. This way Cargo should automatically manage
// `LD_LIBRARY_PATH` and such.
let root = PathBuf::from(env::var_os("CARGO_MANIFEST_DIR").unwrap());
let file = format!("{}foo{}", env::consts::DLL_PREFIX, env::consts::DLL_SUFFIX);
let src = root.join(&file);

let dst_dir = PathBuf::from(env::var_os("OUT_DIR").unwrap());
let dst = dst_dir.join(&file);

fs::copy(&src, &dst).unwrap();
// handle windows, like below
drop(fs::copy(root.join("foo.dll.lib"), dst_dir.join("foo.dll.lib")));

println!("cargo:rerun-if-changed=build.rs");
if cfg!(target_env = "msvc") {
println!("cargo:rustc-link-lib=foo.dll");
} else {
println!("cargo:rustc-link-lib=foo");
}
println!("cargo:rustc-link-search={}",
dst.parent().unwrap().display());
}
"#)
.file("src/main.rs", r#"
extern {
#[link_name = "cargo_test_foo"]
fn foo();
}

fn main() {
unsafe { foo(); }
}
"#);
let p2 = p2.build();

// Move the output `libfoo.so` into the directory of `p2`, and then delete
// the `p` project. On OSX the `libfoo.dylib` artifact references the
// original path in `p` so we want to make sure that it can't find it (hence
// the deletion).
let root = PathBuf::from(p.root());
let root = root.join("target").join("debug").join("deps");
let file = format!("{}foo{}", env::consts::DLL_PREFIX, env::consts::DLL_SUFFIX);
let src = root.join(&file);

let dst = p2.root().join(&file);

fs::copy(&src, &dst).unwrap();
// copy the import library for windows, if it exists
drop(fs::copy(&root.join("foo.dll.lib"), p2.root().join("foo.dll.lib")));
fs::remove_dir_all(p.root()).unwrap();

// Everything should work the first time
assert_that(p2.cargo("run"),
execs().with_status(0));

// Now rename the root directory and rerun `cargo run`. Not only should we
// not build anything but we also shouldn't crash.
let mut new = p2.root();
new.pop();
new.push("bar2");
fs::rename(p2.root(), &new).unwrap();
assert_that(p2.cargo("run").cwd(&new),
execs().with_status(0)
.with_stderr("\
[FINISHED] [..]
[RUNNING] [..]
"));
}