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 compiling for x86_64-unknown-linux-gnu when it's not the default target #1559

Merged
merged 3 commits into from
Nov 28, 2021
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
51 changes: 44 additions & 7 deletions crates/metadata/lib.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
#![warn(missing_docs)]
// N.B. requires nightly rustdoc to document until intra-doc links are stabilized.
//! Collect information that allows you to build a crate the same way that docs.rs would.
//!
//! This library is intended for use in docs.rs and crater, but might be helpful to others.
Expand Down Expand Up @@ -102,6 +101,10 @@ pub enum MetadataError {
#[derive(Default, Deserialize)]
#[serde(rename_all = "kebab-case")]
pub struct Metadata {
/// Whether the current crate is a proc-macro (used by docs.rs to hack around cargo bugs).
#[serde(default)]
pub proc_macro: bool,

/// List of features to pass on to `cargo`.
///
/// By default, docs.rs will only build default features.
Expand Down Expand Up @@ -194,15 +197,26 @@ impl Metadata {
/// If `include_default_targets` is `true` and `targets` is unset, this also includes
/// [`DEFAULT_TARGETS`]. Otherwise, if `include_default_targets` is `false` and `targets`
/// is unset, `other_targets` will be empty.
///
/// All of the above is ignored for proc-macros, which are always only compiled for the host.
pub fn targets(&self, include_default_targets: bool) -> BuildTargets<'_> {
// Proc macros can only be compiled for the host, so just completely ignore any configured targets.
// It would be nice to warn about this somehow ...
if self.proc_macro {
return BuildTargets {
default_target: HOST_TARGET,
other_targets: HashSet::default(),
};
}

let default_target = self
.default_target
.as_deref()
// Use the first element of `targets` if `default_target` is unset and `targets` is non-empty
.or_else(|| {
self.targets
.as_ref()
.and_then(|targets| targets.iter().next().map(String::as_str))
.and_then(|targets| targets.first().map(String::as_str))
})
.unwrap_or(HOST_TARGET);

Expand Down Expand Up @@ -303,31 +317,41 @@ impl std::str::FromStr for Metadata {
_ => None,
};

fn table(mut manifest: Table, table_name: &str) -> Option<Table> {
match manifest.remove(table_name) {
fn table<'a>(manifest: &'a Table, table_name: &str) -> Option<&'a Table> {
match manifest.get(table_name) {
Some(Value::Table(table)) => Some(table),
_ => None,
}
}

let plain_table = manifest
.clone()
.as_ref()
.and_then(|t| table(t, "package"))
.and_then(|t| table(t, "metadata"))
.and_then(|t| table(t, "docs"))
.and_then(|t| table(t, "rs"));
let quoted_table = manifest
.as_ref()
.and_then(|t| table(t, "package"))
.and_then(|t| table(t, "metadata"))
.and_then(|t| table(t, "docs.rs"));
let mut metadata = if let Some(table) = plain_table {
Value::Table(table).try_into()?
Value::Table(table.clone()).try_into()?
} else if let Some(table) = quoted_table {
Value::Table(table).try_into()?
Value::Table(table.clone()).try_into()?
} else {
Metadata::default()
};

let proc_macro = manifest
.as_ref()
.and_then(|t| table(t, "lib"))
.and_then(|table| table.get("proc-macro"))
.and_then(|val| val.as_bool());
if let Some(proc_macro) = proc_macro {
metadata.proc_macro = proc_macro;
}

metadata.rustdoc_args.push("-Z".into());
metadata.rustdoc_args.push("unstable-options".into());

Expand Down Expand Up @@ -363,6 +387,7 @@ mod test_parsing {
assert!(metadata.all_features);
assert!(metadata.no_default_features);
assert!(metadata.default_target.is_some());
assert!(!metadata.proc_macro);

let features = metadata.features.unwrap();
assert_eq!(features.len(), 2);
Expand Down Expand Up @@ -446,6 +471,18 @@ mod test_parsing {
assert!(metadata.no_default_features);
assert!(metadata.default_target.is_some());
}

#[test]
fn test_proc_macro() {
let manifest = r#"
[package]
name = "x"
[lib]
proc-macro = true
"#;
let metadata = Metadata::from_str(manifest).unwrap();
assert!(metadata.proc_macro);
}
}

#[cfg(test)]
Expand Down
116 changes: 100 additions & 16 deletions src/docbuilder/rustwide_builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -210,7 +210,8 @@ impl RustwideBuilder {
}

info!("copying essential files for {}", self.rustc_version);
let source = build.host_target_dir().join("doc");
assert!(!metadata.proc_macro);
let source = build.host_target_dir().join(HOST_TARGET).join("doc");
let dest = tempfile::Builder::new()
.prefix("essential-files")
.tempdir()?;
Expand Down Expand Up @@ -350,14 +351,23 @@ impl RustwideBuilder {
if res.result.successful {
if let Some(name) = res.cargo_metadata.root().library_name() {
let host_target = build.host_target_dir();
has_docs = host_target.join("doc").join(name).is_dir();
has_docs = host_target
.join(default_target)
.join("doc")
.join(name)
.is_dir();
}
}

let mut algs = HashSet::new();
if has_docs {
debug!("adding documentation for the default target to the database");
self.copy_docs(&build.host_target_dir(), local_storage.path(), "", true)?;
self.copy_docs(
&build.host_target_dir(),
local_storage.path(),
default_target,
true,
)?;

successful_targets.push(res.target.clone());

Expand Down Expand Up @@ -593,17 +603,20 @@ impl RustwideBuilder {
.is_ok()
});

// If we're passed a default_target which requires a cross-compile,
// cargo will put the output in `target/<target>/doc`.
// However, if this is the default build, we don't want it there,
// we want it in `target/doc`.
// NOTE: don't rename this if the build failed, because `target/<target>/doc` won't exist.
if successful && target != HOST_TARGET && is_default_target {
// mv target/$target/doc target/doc
// For proc-macros, cargo will put the output in `target/doc`.
// Move it to the target-specific directory for consistency with other builds.
// NOTE: don't rename this if the build failed, because `target/doc` won't exist.
if successful && metadata.proc_macro {
assert!(
is_default_target && target == HOST_TARGET,
"can't handle cross-compiling macros"
);
// mv target/doc target/$target/doc
let target_dir = build.host_target_dir();
let old_dir = target_dir.join(target).join("doc");
let new_dir = target_dir.join("doc");
let old_dir = target_dir.join("doc");
let new_dir = target_dir.join(target).join("doc");
debug!("rename {} to {}", old_dir.display(), new_dir.display());
std::fs::create_dir(target_dir.join(target))?;
std::fs::rename(old_dir, new_dir)?;
}

Expand Down Expand Up @@ -656,7 +669,16 @@ impl RustwideBuilder {
if let Some(cpu_limit) = self.config.build_cpu_limit {
cargo_args.push(format!("-j{}", cpu_limit));
}
if target != HOST_TARGET {
// Cargo has a series of frightening bugs around cross-compiling proc-macros:
// - Passing `--target` causes RUSTDOCFLAGS to fail to be passed 🤦
// - Passing `--target` will *create* `target/{target-name}/doc` but will put the docs in `target/doc` anyway
// As a result, it's not possible for us to support cross-compiling proc-macros.
// However, all these caveats unfortunately still apply when `{target-name}` is the host.
// So, only pass `--target` for crates that aren't proc-macros.
//
// Originally, this had a simpler check `target != HOST_TARGET`, but *that* was buggy when `HOST_TARGET` wasn't the same as the default target.
// Rather than trying to keep track of it all, only special case proc-macros, which are what we actually care about.
if !metadata.proc_macro {
cargo_args.push("--target".into());
cargo_args.push(target.into());
};
Expand Down Expand Up @@ -702,7 +724,7 @@ impl RustwideBuilder {
dest = dest.join(target);
}

info!("{} {}", source.display(), dest.display());
info!("copy {} to {}", source.display(), dest.display());
copy_dir_all(source, dest).map_err(Into::into)
}

Expand Down Expand Up @@ -835,11 +857,11 @@ mod tests {

// doc archive exists
let doc_archive = rustdoc_archive_path(crate_, version);
assert!(storage.exists(&doc_archive)?);
assert!(storage.exists(&doc_archive)?, "{}", doc_archive);

// source archive exists
let source_archive = source_archive_path(crate_, version);
assert!(storage.exists(&source_archive)?);
assert!(storage.exists(&source_archive)?, "{}", source_archive);

// default target was built and is accessible
assert!(storage.exists_in_archive(&doc_archive, &format!("{}/index.html", crate_path))?);
Expand Down Expand Up @@ -935,4 +957,66 @@ mod tests {
Ok(())
})
}

#[test]
#[ignore]
fn test_proc_macro() {
wrapper(|env| {
let crate_ = "thiserror-impl";
let version = "1.0.26";
let mut builder = RustwideBuilder::init(env).unwrap();
assert!(builder.build_package(crate_, version, PackageKind::CratesIo)?);

let storage = env.storage();

// doc archive exists
let doc_archive = rustdoc_archive_path(crate_, version);
assert!(storage.exists(&doc_archive)?);

// source archive exists
let source_archive = source_archive_path(crate_, version);
assert!(storage.exists(&source_archive)?);

Ok(())
});
}

#[test]
#[ignore]
fn test_cross_compile_non_host_default() {
wrapper(|env| {
let crate_ = "xingapi";
let version = "0.3.3";
let mut builder = RustwideBuilder::init(env).unwrap();
assert!(builder.build_package(crate_, version, PackageKind::CratesIo)?);

let storage = env.storage();

// doc archive exists
let doc_archive = rustdoc_archive_path(crate_, version);
assert!(storage.exists(&doc_archive)?, "{}", doc_archive);

// source archive exists
let source_archive = source_archive_path(crate_, version);
assert!(storage.exists(&source_archive)?, "{}", source_archive);

let target = "x86_64-unknown-linux-gnu";
let crate_path = crate_.replace("-", "_");
let target_docs_present = storage.exists_in_archive(
&doc_archive,
&format!("{}/{}/index.html", target, crate_path),
)?;

let web = env.frontend();
let target_url = format!(
"/{}/{}/{}/{}/index.html",
crate_, version, target, crate_path
);

assert!(target_docs_present);
Copy link
Member Author

Choose a reason for hiding this comment

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

I double checked and this does fail without the fix in this commit.

[2021-11-24T17:58:10Z DEBUG docs_rs::docbuilder::rustwide_builder] adding sources into database
[2021-11-24T17:58:10Z DEBUG docs_rs::db::add_package] Adding package into database
[2021-11-24T17:58:10Z DEBUG docs_rs::db::add_package] Adding doc coverage into database
[2021-11-24T17:58:10Z DEBUG docs_rs::db::add_package] Adding build into database
[2021-11-24T17:58:10Z INFO  docs_rs::db::add_package] Updating crate data for xingapi
[2021-11-24T17:58:10Z DEBUG docs_rs::docbuilder::rustwide_builder] cleaning old storage folder rustdoc/xingapi/0.3.3/
[2021-11-24T17:58:10Z DEBUG docs_rs::docbuilder::rustwide_builder] cleaning old storage folder sources/xingapi/0.3.3/
[2021-11-24T17:58:11Z ERROR docs_rs::web::page::templates] Failed to load rustc resource suffix: missing rustc version
[2021-11-24T17:58:11Z INFO  docs_rs::web] Running docs.rs web server on http://127.0.0.1:39737
thread 'docbuilder::rustwide_builder::tests::test_cross_compile_non_host_default' panicked at 'assertion failed: target_docs_present', src/docbuilder/rustwide_builder.rs:1004:13

failures:
    docbuilder::rustwide_builder::tests::test_cross_compile_non_host_default

assert_success(&target_url, web)?;

Ok(())
});
}
}