Skip to content
Open
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
99 changes: 91 additions & 8 deletions crates/bin/docs_rs_builder/src/docbuilder/rustwide_builder.rs

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

CI failed cause new artifact-based test hit rustwide local manifest validation before our build path.
i changed the test to check cargo metadata flag forwarding directly - fails without -Zbindeps, passes with it.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I thing I would like to keep a full build test to be sure the whole feature keeps working.

CI failed cause new artifact-based test hit rustwide local manifest validation before our build path.

Which part fails?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

It fails in rustwide’s local crate manifest validation step (before the docs.rs build path): rustwide::prepare::validate_manifest runs cargo metadata --manifest-path Cargo.toml --no-deps without -Zbindeps, so Cargo rejects artifact = "...".

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't fully follow.

I can only see that build_local_package calls load_metadata_from_rustwide, which you both adapt in this PR?

So from what I see, any local crate manifest validation path would also be called in the "normal" build path?

It's totally possible I'm just missing context or details, but since I'll need to maintain it, I want to fully understand :)

Can you push a commit where I can see the failing test how you tried it? That would help .

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

generally I'm super happy this can make progress

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I just tried to build the crate from the issue using your branch, using the "normal" docs.rs build, not test.

2026-03-14T12:35:01.734600Z  INFO build_package{name=KrateName("protoc-plugin-by-closure") version=Version(Version { major: 0, minor: 1, patch: 6 }) kind=CratesIo collect_metrics=true}:build_package_inner{name=KrateName("prot
oc-plugin-by-closure") version=Version(Version { major: 0, minor: 1, patch: 6 }) kind=CratesIo crate_id=CrateId(1) release_id=ReleaseId(1) build_id=BuildId(1) collect_metrics=true}: rustwide::cmd: running `Command { std: CARG
O_HOME="/opt/docsrs/rustwide/cargo-home" RUSTUP_HOME="/opt/docsrs/rustwide/rustup-home" "/opt/docsrs/rustwide/cargo-home/bin/cargo" "+nightly" "metadata" "--manifest-path" "Cargo.toml" "--no-deps", kill_on_drop: false }`
2026-03-14T12:35:02.031737Z DEBUG build_package{name=KrateName("protoc-plugin-by-closure") version=Version(Version { major: 0, minor: 1, patch: 6 }) kind=CratesIo collect_metrics=true}:update_build_with_error{build_id=BuildId
(1) build_error=Some(Other(invalid Cargo.toml syntax

Stack backtrace:
   0: anyhow::error::<impl core::convert::From<E> for anyhow::Error>::from
   1: <T as core::convert::Into<U>>::into
   2: rustwide::prepare::Prepare::validate_manifest
   3: rustwide::prepare::Prepare::prepare
   4: rustwide::build::BuildDirectory::run
             at ./usr/local/cargo/registry/src/index.crates.io-1949cf8c6b5b557f/rustwide-0.22.1/src/build.rs:197:17

This looks like the failing test that you saw was a sign that this feature wouldn't have worked at all for builds.

Did you manually run a build to test it?

In any case: I believe you should re-add the test, and then we can figure out what is necessary to fix it.

Perhaps even a change to rustwide?

Original file line number Diff line number Diff line change
Expand Up @@ -106,9 +106,11 @@ fn load_metadata_from_rustwide(
workspace: &Workspace,
toolchain: &Toolchain,
source_dir: &Path,
host_unstable_flags: &[String],
) -> Result<CargoMetadata> {
let res = Command::new(workspace, toolchain.cargo())
.args(["metadata", "--format-version", "1"])
.args(host_unstable_flags)
.current_directory(source_dir)
.log_output(false)
.run_capture()?;
Expand Down Expand Up @@ -495,10 +497,17 @@ impl RustwideBuilder {
}

pub fn build_local_package(&mut self, path: &Path) -> Result<BuildPackageSummary> {
let metadata = load_metadata_from_rustwide(&self.workspace, &self.toolchain, path)
.map_err(|err| {
err.context(format!("failed to load local package {}", path.display()))
})?;
let host_unstable_flags = Metadata::from_crate_root(path)
.map(|metadata| metadata.unstable_cargo_flags())
.unwrap_or_default();

let metadata = load_metadata_from_rustwide(
&self.workspace,
&self.toolchain,
path,
&host_unstable_flags,
)
.map_err(|err| err.context(format!("failed to load local package {}", path.display())))?;
let package = metadata.root();
self.build_package(
&package
Expand Down Expand Up @@ -639,13 +648,19 @@ impl RustwideBuilder {
let local_storage = tempfile::tempdir_in(&self.config.temp_dir)?;

let mut algs = HashSet::new();
let source_stats = {
let (source_stats, host_unstable_flags) = {
let _span = info_span!("adding sources into database").entered();
debug!("adding sources into database");
let temp_dir = tempfile::tempdir_in(&self.config.temp_dir)?;

krate.copy_source_to(&self.workspace, temp_dir.path())?;

// Read from this copy: rustwide's prepare phase needs the flags before the build

@syphar syphar Aug 18, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Shouldn't we fix rustwide to have the flags available when the build needs them?

Or am I missing something?

View changes since the review

// closure below gets access to the sources.
let host_unstable_flags = Metadata::from_crate_root(temp_dir.path())
.map(|metadata| metadata.unstable_cargo_flags())
.unwrap_or_default();

let stats = self.runtime.block_on(
self.storage
.store_all_in_archive(&source_archive_path(name, version), &temp_dir),
Expand All @@ -654,11 +669,12 @@ impl RustwideBuilder {
fs::remove_dir_all(temp_dir.path())?;

algs.insert(stats.alg);
stats
(stats, host_unstable_flags)
};

let successful = build_dir
.build(&self.toolchain, &krate, self.prepare_sandbox(&limits))
.extra_cargo_args(&host_unstable_flags)
.run(|build| {
// NOTE: rustwide will run `copy_source_to` again when preparing the call to this
// closure.
Expand All @@ -674,7 +690,25 @@ impl RustwideBuilder {
{
let _span = info_span!("fetch_build_std_dependencies").entered();
// Fetch this before we enter the sandbox, so networking isn't blocked.
build.fetch_build_std_dependencies(&targets)?;
if host_unstable_flags.is_empty() {
build.fetch_build_std_dependencies(&targets)?;
} else {
// `Build` doesn't carry the `extra_cargo_args` given to `BuildBuilder`,
// so rustwide would run this fetch without them and fail to parse the

@syphar syphar Aug 18, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I have the impression this should be fixed in rustwide? So fetch_build_std_dependencies will work with extra args directly?

View changes since the review

// manifest. Run the equivalent command ourselves until rustwide can
// forward them here too.
self.toolchain.add_component(&self.workspace, "rust-src")?;
let mut cmd = Command::new(&self.workspace, self.toolchain.cargo())
.args(["fetch", "--manifest-path", "Cargo.toml"])
.args(&host_unstable_flags)
.args(["-Zbuild-std"])
.env("RUSTC_BOOTSTRAP", "1")
.current_directory(build.host_source_dir());
for target in &targets {
cmd = cmd.args(["--target", target]);
}
cmd.run()?;
}
}


Expand All @@ -693,14 +727,16 @@ impl RustwideBuilder {
let _span = info_span!("cargo_generate_lockfile").entered();
Command::new(&self.workspace, self.toolchain.cargo())
.current_directory(build.host_source_dir())
.arg("generate-lockfile")
.args(["generate-lockfile"])
.args(&host_unstable_flags)
.run_capture()?;
}
{
let _span = info_span!("cargo fetch --locked").entered();
Command::new(&self.workspace, self.toolchain.cargo())
.current_directory(build.host_source_dir())
.args(["fetch", "--locked"])
.args(&host_unstable_flags)
.run_capture()?;
}
res =
Expand Down Expand Up @@ -1145,6 +1181,7 @@ impl RustwideBuilder {
&self.workspace,
&self.toolchain,
&build.host_source_dir(),
&metadata.unstable_cargo_flags(),
)?;

let mut rustdoc_flags = vec![
Expand Down Expand Up @@ -2359,6 +2396,52 @@ mod tests {
Ok(())
}

#[test]
#[ignore]
fn test_bindeps_metadata_with_unstable_flags() -> Result<()> {
let env = TestEnvironment::new()?;
let mut builder = env.build_builder()?;
builder.update_toolchain()?;
let crate_path = Path::new("tests/crates/bindeps-test");
let metadata = Metadata::from_crate_root(crate_path)?;
let unstable_flags = metadata.unstable_cargo_flags();

assert!(
load_metadata_from_rustwide(&builder.workspace, &builder.toolchain, crate_path, &[])
.is_err(),
"cargo metadata should fail without -Zbindeps",
);

assert!(
load_metadata_from_rustwide(
&builder.workspace,
&builder.toolchain,
crate_path,
&unstable_flags,
)
.is_ok(),
"cargo metadata should succeed with -Zbindeps",
);

Ok(())
}

#[test]
#[ignore]
fn test_bindeps_crate_full_build() -> Result<()> {
let env = TestEnvironment::new()?;
let mut builder = env.build_builder()?;
builder.update_toolchain()?;

assert!(
builder
.build_local_package(Path::new("tests/crates/bindeps-test"))?
.successful
);

Ok(())
}

#[test]
#[ignore]
fn test_build_with_cpu_limit() -> Result<()> {
Expand Down
14 changes: 14 additions & 0 deletions crates/bin/docs_rs_builder/tests/crates/bindeps-test/Cargo.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
[package]
Comment thread
syphar marked this conversation as resolved.
name = "bindeps-test"
version = "0.1.0"
edition = "2021"

[package.metadata.docs.rs]
cargo-args = ["-Zbindeps"]

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

More a question, because I don't know:

Would this bindeps-test crate fail the docs-build without the changes in this PR? Or would the arg just be added to the main build, and the other calls (cargo metadata) would fail?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

hi again! thanks for your kind words :)

i rebased the branch and removed artifacts from my builds and tests, so the PR now only contains the intended files.

about your question on bindeps-test: without this PR, docs.rs would fail before the main rustdoc build, during host-side cargo commands (cargo metadata / lockfile/fetch path), because the manifest uses artifact dependencies and Cargo requires -Z bindeps for parsing/resolution there. normal docs build command already received cargo-args. the missing part was forwarding the required flag to those host-side commands too.

i also fixed the new audit failure by updating quinn-proto in Cargo.lock

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think this is the last piece missing for me :) Thank you for working on this!

One thing I'm still not sure about (might be lack of knowledge) is if this test crate would really fail to build without this PR?

Wouldn't a cleaner example be one where we actually artifact dependencies? like the one mentioned in #2710?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

done, thanks for the suggestion. changed bindeps-test to use a real artifact dependency, now it fails on cargo metadata without -Z bindeps and succeeds with it. and it demonstrates exactly why forwarding -Zbindeps to host-side cargo commands is needed.


[build-dependencies]
bindeps-helper = { path = "bindeps-helper", artifact = "bin" }

[lib]
name = "bindeps_test"
path = "src/lib.rs"
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
[package]
name = "bindeps-helper"
version = "0.1.0"
edition = "2021"

[[bin]]
name = "bindeps-helper"
path = "src/main.rs"
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
fn main() {}
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
pub fn hello() -> &'static str {
"Hello from bindeps-test!"
}
1 change: 1 addition & 0 deletions crates/bin/docs_rs_web/templates/core/Cargo.toml.example
Original file line number Diff line number Diff line change
Expand Up @@ -48,4 +48,5 @@ rustdoc-args = ["--example-rustdoc-arg"]
# List of command line arguments for `cargo`.
#
# These cannot be a subcommand, they may only be options.
# Of the unstable flags, only `bindeps` also reaches host-side cargo commands.
cargo-args = ["-Z", "build-std"]
7 changes: 7 additions & 0 deletions crates/bin/docs_rs_web/templates/core/about/metadata.html
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,13 @@ <h1>Metadata for custom builds</h1>
{% filter highlight("toml") %}
{%- include "core/Cargo.toml.example" -%}
{% endfilter %}

<p>
For security reasons, docs.rs only forwards a whitelist of unstable cargo flags to
host-side cargo commands (<code>cargo metadata</code>, <code>cargo fetch</code>,
<code>cargo generate-lockfile</code>). Currently, only <code>bindeps</code> is
supported (as <code>-Zbindeps</code> or <code>-Z bindeps</code>).
</p>
</div>
</div>
{%- endblock body %}
66 changes: 66 additions & 0 deletions crates/lib/metadata/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,30 @@ pub struct Metadata {
additional_targets: Vec<String>,
}

impl Metadata {
/// Unstable flags from `cargo_args` that may also be passed to cargo commands we run
/// outside the sandbox. These run with our privileges, so this is a whitelist.
pub fn unstable_cargo_flags(&self) -> Vec<String> {
let mut flags = Vec::new();
let mut iter = self.cargo_args.iter();
while let Some(arg) = iter.next() {
if arg == "-Z" {
if let Some(value) = iter.next()
&& value == "bindeps"
{
flags.push("-Z".to_string());
flags.push(value.clone());
}
} else if let Some(value) = arg.strip_prefix("-Z")
&& value == "bindeps"
{
flags.push(arg.clone());
}
}
flags
}
}

/// The targets that should be built for a crate.
///
/// The `default_target` is the target to be used as the home page for that crate.
Expand Down Expand Up @@ -553,6 +577,48 @@ mod test_parsing {
let metadata = Metadata::from_str(manifest).unwrap();
assert!(!metadata.proc_macro);
}

#[test]
fn test_unstable_cargo_flags() {
let manifest = r#"
[package]
name = "test"
[package.metadata.docs.rs]
cargo-args = ["-Zbindeps", "--some-other-arg"]
"#;
let metadata = Metadata::from_str(manifest).unwrap();
assert_eq!(metadata.unstable_cargo_flags(), vec!["-Zbindeps"]);

let manifest = r#"
[package]
name = "test"
[package.metadata.docs.rs]
cargo-args = ["-Z", "bindeps", "--other"]
"#;
let metadata = Metadata::from_str(manifest).unwrap();
assert_eq!(metadata.unstable_cargo_flags(), vec!["-Z", "bindeps"]);

let manifest = r#"
[package]
name = "test"
[package.metadata.docs.rs]
cargo-args = ["-Zbindeps", "-Z", "build-std", "--offline"]
"#;
let metadata = Metadata::from_str(manifest).unwrap();
assert_eq!(metadata.unstable_cargo_flags(), vec!["-Zbindeps"]);

let manifest = r#"
[package]
name = "test"
[package.metadata.docs.rs]
cargo-args = ["--offline", "--locked"]
"#;
let metadata = Metadata::from_str(manifest).unwrap();
assert!(metadata.unstable_cargo_flags().is_empty());

let metadata = Metadata::default();
assert!(metadata.unstable_cargo_flags().is_empty());
}
}

#[cfg(test)]
Expand Down
Loading