Closed pnkfelix closed 3 months ago
ehuss
commented
11 months ago It should be working. Can you share your exact .cargo/config.toml file? Is your .cargo/config.toml in the project directory where you are running cargo-bisect-rustc? What is the output of cargo-bisect-rustc -vv -- build --verbose? Are you sure there aren't any environment variables (like RUSTFLAGS) that would take precedence over your config file?
pnkfelix
commented
11 months ago Since this involves a bunch of files, let me see if I can put it into a repo for the repro.
pnkfelix
commented
11 months ago (Hmm, I am now seeing evidence that it is doing something with the config file. Namely, when I added build.rustflags = "-Zwhoa" via the config file, all the builds fail, and when I remove it, then I see some builds succeed. So there must be something about the specific build flag I was trying to use, namely `rustflags="-Clink-arg=-Wl,--no-undefined-version", that was behaving in an unexpected fashion. That, or it was user error on my part. I'll figure out which case I'm in.)
pnkfelix
commented
11 months ago Here's the repository I'm using for reproduction of the problem I'm seeing: https://github.com/pnkfelix/cbr-issue-284-demo
When I run cargo bisect rustc, it ends up flagging a regression in nightly-2023-03-12 (and then tracks the problem down further from there). The problem is that the flagging of the regression there only happens if you are not passing the rustflags that I have requested. And I have confirmed that manually invoking cargo build for the nightlies in question, with that .cargo/config.toml in place, does show the regression continuing to earlier than 2023-03-12.
pnkfelix
commented
11 months ago To be clear: At this point I am forced to acknowledge that something must be happening with the .cargo/config.toml, because when I put in nonsense as the rustflags setting (likerustflags=-Zwhoa), it causes the build to fail reliably via cargo-bisect-rustc.
So there must be some really weird interaction with the specific rustflags setting I happen to be using? It is goofy.
ehuss
commented
11 months ago The reason you are seeing different behavior when running manually is because you are not passing the --target flag. When you don't pass the --target flag, rustflags are passed to all rustc invocations. When you do pass the --target flag, rustflags are not passed to host invocations (proc-macros, build scripts, and their dependencies). cargo-bisect-rustc always passes --target.
This happens because cargo tries to share dependencies when --target is not specified to reduce build times.
ehuss
commented
11 months ago BTW, if you need to pass rustflags to proc-macros even when --target is used, you can do something like this:
[target.'x86_64-unknown-linux-gnu']
rustflags="-Clink-arg=-Wl,--no-undefined-version"
[host]
rustflags="-Clink-arg=-Wl,--no-undefined-version"And pass the -Zhost-config -Ztarget-applies-to-host flags.
pnkfelix
commented
11 months ago Hmm, okay. I can totally understand how this arises now.
I'm trying to figure out what is the best way to address overall issue here.
In particular:
cargo-bisect-rustc always passes --targetcargo-bisect-rustc doc ever says that it always passed --targetHere's the heart of my complaint: The usage model that I think we try to promote is: "If you're doing cargo build, you can just do cargo-bisect-rustc build to bisect the same experience.
I can absolutely understand there being deviations between what actually happens versus the illusion we are trying to present to users.
... Maybe all I want here is some section of the doc where we spell out ways in which cargo CMD may deviate from cargo-bisect-rustc CMD ? Even as an FAQ entry or something?
ehuss
commented
11 months ago I think at least documenting it would be good. One place I forgot to mention this is in https://rust-lang.github.io/cargo-bisect-rustc/usage.html#scripting, where it should be mentioned that scripts don't need to specify --target because CARGO_BUILD_TARGET is set (similar to how the toolchain also doesn't need to be specified). If you want to add an FAQ, that would be good, too. I'm not sure how likely that is to help the general case, since I assume almost nobody is going to read the entire documentation and retain every nuance.
It seems a little strange to me that it unconditionally sets the target, but it looks like that was done in part for https://github.com/cross-rs/cross/issues/699, where cross would break because rustup does not support components for custom toolchains, and would error when cross ran rustup target list. That seems like a bit of a convoluted issue involving several other tools. I could see a reasonable alternative would be to stop unconditionally setting CARGO_BUILD_TARGET when --target is not specified, and document that cross users need to set it in a script (there could be another "Example" chapter on using cross). How does that sound?
pnkfelix
commented
11 months ago I'm not sure how likely that is to help the general case, since I assume almost nobody is going to read the entire documentation and retain every nuance.
I agree with this point. That is, we should adjust the documentation to reflect what the tool does (though that might itself change, as you outline in your second paragraph), but we cannot rely on people to read the docs.
However, that does lead me to this question: Is there any verbosity level at which cargo bisect rustc will print out the contextual parameters it is setting (i.e. what environment variables it is setting and what command line switches it is passing)? I think that could have helped me track this down, if I had thought to use it, but as far as I can tell no matter how many levels of --verbose I pass along, I don't get feedback about this --target/CARGO_BUILD_TARGET business...
I could see a reasonable alternative would be to stop unconditionally setting CARGO_BUILD_TARGET when
--targetis not specified, and document thatcrossusers need to set it in a script (there could be another "Example" chapter on usingcross).
I'll have to think about this. It sounds plausible on its surface. What's the migration path for helping current users deal with the changes here, it is just a matter of including it in the release notes for the new version, or are there more direct ways to give useful feedback to the people relying on the current behavior?
ehuss
commented
11 months ago However, that does lead me to this question: Is there any verbosity level at which cargo bisect rustc will print out the contextual parameters it is setting
I don't think so. Improving that also seems good to me.
What's the migration path for helping current users deal with the changes here, it is just a matter of including it in the release notes for the new version, or are there more direct ways to give useful feedback to the people relying on the current behavior?
I'm not sure. I don't have a sense of how often people are depending on the current behavior. I can't think of anything besides cross that would be affected, but I was surprised to see that it was affected.
can cargo-bisect-rustc respect the .cargo/config.toml? (Should it already be doing so?)
As part of dissecting https://github.com/rust-lang/rust/issues/111888, I was looking into adding a linker flag to the rustc invocations via a
.cargo/config.tomlfile, as described here: https://doc.rust-lang.org/cargo/reference/config.html#buildrustflagsBut it seems from my local experiments that while my manual invocations of
cargoare pulling in the config settings, the invocations thatcargo-bisect-rustcemits do not seem to be including the rustflags specified there.