From ac0913634a9826adf334aac5d54f7747ef38f8f7 Mon Sep 17 00:00:00 2001 From: Joshua Nelson Date: Wed, 24 Nov 2021 12:10:58 -0500 Subject: [PATCH 1/3] don't take an owned value in `table` --- crates/metadata/lib.rs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/crates/metadata/lib.rs b/crates/metadata/lib.rs index 339faf913..7b280ae98 100644 --- a/crates/metadata/lib.rs +++ b/crates/metadata/lib.rs @@ -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. @@ -303,27 +302,28 @@ impl std::str::FromStr for Metadata { _ => None, }; - fn table(mut manifest: Table, table_name: &str) -> Option { - 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() }; From 8772e1a26a9da0ce22ca90b60b4af7875e235366 Mon Sep 17 00:00:00 2001 From: Joshua Nelson Date: Wed, 24 Nov 2021 12:11:10 -0500 Subject: [PATCH 2/3] record whether the crate is a proc-macro or not --- crates/metadata/lib.rs | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/crates/metadata/lib.rs b/crates/metadata/lib.rs index 7b280ae98..fb33e48e8 100644 --- a/crates/metadata/lib.rs +++ b/crates/metadata/lib.rs @@ -101,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. @@ -328,6 +332,15 @@ impl std::str::FromStr for Metadata { 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()); @@ -363,6 +376,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); @@ -446,6 +460,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)] From 7007dc92f16c7684acabc7c9d27968c1bdf7f43c Mon Sep 17 00:00:00 2001 From: Joshua Nelson Date: Wed, 24 Nov 2021 12:12:07 -0500 Subject: [PATCH 3/3] fix the actual bug - only skip `--target` for proc-macros - don't try build non-host targets for proc-macros, since they're not supported anyway. - change rustwide_builder to look in the platform-specific target directory for the default target - stores all source docs in the target/{target_name} directory, rather than trying to special case anything - this also creates the `{target_name}` directory before copying, because `rename` delegates directly to the syscall and doesn't create parent directories --- crates/metadata/lib.rs | 13 +++- src/docbuilder/rustwide_builder.rs | 116 +++++++++++++++++++++++++---- 2 files changed, 112 insertions(+), 17 deletions(-) diff --git a/crates/metadata/lib.rs b/crates/metadata/lib.rs index fb33e48e8..b837dfc3b 100644 --- a/crates/metadata/lib.rs +++ b/crates/metadata/lib.rs @@ -197,7 +197,18 @@ 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() @@ -205,7 +216,7 @@ impl Metadata { .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); diff --git a/src/docbuilder/rustwide_builder.rs b/src/docbuilder/rustwide_builder.rs index caadd4380..4a6af675a 100644 --- a/src/docbuilder/rustwide_builder.rs +++ b/src/docbuilder/rustwide_builder.rs @@ -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()?; @@ -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()); @@ -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//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//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)?; } @@ -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()); }; @@ -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) } @@ -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))?); @@ -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); + assert_success(&target_url, web)?; + + Ok(()) + }); + } }