bindeps failing build fix #3111
Conversation
github-actions
bot
added
the
S-waiting-on-review
|
r? @syphar take a look please if you have time :> |
Already saw it, needed to finish a bigger refactor (#3113). Also need to check the implications of adding this argument to all commands. Give me a couple of days. Until then: |
thanks! about link - it’s false detection as i remember, but i will check it again |
in what way? It's rustfmt failing, so you have to run it locally. Also check if you're using the latest rust stable. |
oh, sorry then, i’m currently working on many projects, something mixed up in my brain. will fix this if a new few hours |
enthropy7
force-pushed
the
main
branch
3 times, most recently
from
962dca4 to
f2f2850
Compare
|
starting to look into this |
syphar
left a comment
syphar
left a comment
There was a problem hiding this comment.
I need to do more research here before we can merge this.
To explain:
we have two ways of running cargo commands here.
- "safe" commands are run directly on the docs.rs server.
- "unsafe" commands are run inside a docker container
in the code:
Command::newis on the host,- stuff behind
prepare_sandboxor in methods accepting a&Buildis sandboxed
All commands where a crate author could run their own code (like in a build script) in any way have to be sandboxed, otherwise it would be possible for one crate author to affect the build output of other crates, so also what's hosted / distributed via docs.rs
For the current commands that are run on the host (generate-lockfile, fetch, etc) we know that there is no way you can affect the host in any way.
What I don't know: Does bindeps or any other unstable cargo feature open up these possibilities? If yes, we need to also sandbox these commands.
I'll try to get someone from the cargo team to provide more insight.
| @@ -0,0 +1,12 @@ | |||
| [package] | |||
There was a problem hiding this comment.
this crate should be moved to the "new" location
crates/bin/docs_rs_builder/tests/crates/
|
just a note: please don't start sandboxing for now, generally sandboxed commands run slower, and we would have to keep the sandbox limits in mind. :) |
This commit adds tests that demonstrate the problem: crates using unstable cargo features like `-Zbindeps` fail to build because the unstable flags from `cargo-args` are not passed to `cargo metadata` and `cargo fetch` commands. - Add test_bindeps_cargo_args_parsing() to verify cargo-args parsing - Add test_bindeps_separate_args_parsing() for -Z bindeps style - Add test_bindeps_crate_fails_without_unstable_flags() integration test that EXPECTS FAILURE (demonstrates the issue) - Add test_load_metadata_signature_doesnt_accept_unstable_flags() to document the issue - Add test crate tests/crates/bindeps-test/ with bindeps configuration These tests demonstrate the issue: 1. load_metadata_from_rustwide() doesn't accept unstable flags parameter 2. cargo metadata/fetch are called without -Z flags from cargo-args 3. This causes builds to fail for crates using bindeps See: rust-lang#2710
enthropy7
force-pushed
the
main
branch
4 times, most recently
from
6298fe6 to
be755ba
Compare
This commit fixes issue rust-lang#2710 by ensuring that unstable cargo flags (e.g., `-Zbindeps`) from `cargo-args` are passed to ALL cargo commands, not just `cargo rustdoc`. Changes: - Add `Metadata::unstable_cargo_flags()` method to extract `-Z*` flags from `cargo-args` - Update `load_metadata_from_rustwide()` to accept `unstable_flags` parameter and pass them to `cargo metadata` - Update `build_local_package()` to read metadata and pass unstable flags - Update `execute_build()` to pass unstable flags to `cargo metadata` - Update fallback path (`cargo generate-lockfile`, `cargo fetch`) to pass unstable flags - Remove unused `.peekable()` in `unstable_cargo_flags()` method - Apply rustfmt formatting - Update test to expect success (with fix)
|
@enthropy7 generally: Like this I can directly see what exactly changed after the last review. ( for long-running reviews I even recommend not rebasing on main until before the merge). |
ok, thanks, that's just practice i learnt from cargo rules of commit :))) |
2 participants
fixed #2710 by ensuring unstable cargo flags (e.g., -
Zbindeps) from[package.metadata.docs.rs]cargo-argsare passed to all cargo commands, before it was onlyrustdoc. AddedMetadata::unstable_cargo_flags()to extract -Z* flags and updatedload_metadata_from_rustwide()and all cargo invocations (metadata, fetch, generate-lockfile, rustdoc)to accept and forward these flags. This lets crates using unstable features build on docs.rs. change is minimal, backward compatible, and follows existing patterns, covering all cargo commands to prevent failures at any stage. all tests added in 2 commits. 1-st demonstrate how it was before, 2-nd - how it works now (well).