Add extra_cargo_args support for prepare phase#119
Conversation
syphar
left a comment
syphar
left a comment
There was a problem hiding this comment.
Thanks!
This is missing tests.
we have "integration-like" tests / examples in tests/.
enthropy7
force-pushed
the
extra-cargo-args
branch
from
a2b241d to
fa3e5b1
Compare
|
added and tested :) |
I realized that I totally missed this comment, and in my "reviews requested" this PR didn't pop up. Will recheck. ( generally: I did "some" refactoring in the codebase that means you need to update this PR too |
Test that extra cargo args are forwarded to cargo during the prepare phase: a positive test with --quiet and a negative test with an invalid flag to prove args are actually passed through.
enthropy7
force-pushed
the
extra-cargo-args
branch
from
fa3e5b1 to
406271e
Compare
|
done! :) |
syphar
left a comment
syphar
left a comment
There was a problem hiding this comment.
One small API change, two small things for the tests.
Generally:
Right now we assume that the same extra_cargo_args are accepted by cargo metadata, generate-lockfile andfetch, and probably other commands that come later.
Also I wonder if users of the lib would expect that these commands are also automatically added to other cargo calls in the same build.
But: I think when we do the small things from above, we can merge / release this.
| krate: &'a Crate, | ||
| sandbox: SandboxBuilder, | ||
| patches: Vec<CratePatch>, | ||
| extra_cargo_args: Vec<String>, |
There was a problem hiding this comment.
let's follow the pattern we have in cmd::Command
| extra_cargo_args: Vec<String>, | |
| extra_cargo_args: Vec<OsString>, |
| /// })?; | ||
| /// # Ok(()) | ||
| /// # } | ||
| pub fn extra_cargo_args(mut self, args: Vec<String>) -> Self { |
There was a problem hiding this comment.
same here, the pattern from Command::args()
pub fn extra_cargo_args<S: Into<OsString>>(
mut self,
args: impl IntoIterator<Item = S>,
) -> Self {
self.extra_cargo_args
.extend(args.into_iter().map(Into::into));
self
}| }); | ||
| assert!( | ||
| res.is_err(), | ||
| "expected extra cargo args to cause a prepare failure" |
There was a problem hiding this comment.
can we assert something that ensures that the invalid flag is the actual error? perhaps just reading stdout and checking what's in it?
| runner::run("hello-world", |run| { | ||
| run.build(SandboxBuilder::new().enable_networking(false), |builder| { | ||
| builder | ||
| .extra_cargo_args(vec!["--quiet".into()]) |
There was a problem hiding this comment.
can we somehow assert that this cargo arg is actually working?
Like, checking stdout for the output that this suppresses?
2 participants
this adds an
extra_cargo_args()method toBuildBuilderthat forwards extra arguments to all cargo commands duringPrepare::prepare():validate_manifest,capture_lockfile, andfetch_deps. without this, crates that require unstable cargo flags for manifest parsing (e.g.-Zbindepsfor artifact dependencies) fail at the prepare phase withInvalidCargoTomlSyntax, because there's no way for callers to pass these flags.needed by docs.rs: rust-lang/docs.rs#3111