1547196 - Vendoring crates with rust-toolchain configuration causes build to fail on OS X

Status: RESOLVED FIXED

(Firefox Build System :: Toolchains, defect)

Description

[Mark Goodwin [:mgoodwin]]

On attempting to vendor rust_cascade, I encountered an issue with a dependency:

17:28.97 Compiling rust_cascade v0.2.0
17:29.08 error[E0514]: found crate bitvec compiled by an incompatible version of rustc
17:29.08 --> /Users/markgoodwin/mozilla-source/mozilla-central/third_party/rust/rust_cascade/src/lib.rs:1:1
17:29.08 |
17:29.08 1 | extern crate bitvec;
17:29.08 | ^^^^^^^^^^^^^^^^^^^^
17:29.08 |
17:29.08 = help: please recompile that crate using this compiler (rustc 1.34.0-nightly (00aae71f5 2019-02-25))
17:29.08 = note: the following crate versions were found:
17:29.08 crate bitvec compiled by rustc 1.31.0 (abe02cefd 2018-12-04): /Users/markgoodwin/mozilla-source/mozilla-central/obj-x86_64-apple-darwin18.5.0/x86_64-apple-darwin/release/deps/libbitvec-d2752a1f636cdac0.rlib
17:29.08 error: aborting due to previous error
17:29.09 For more information about this error, try rustc --explain E0514.
17:29.09 error: Could not compile rust_cascade.

On further investigation, rustc 1.31 was being used because the bitvec crate specifies minimum versions using a rust-toolchain file (see https://github.com/myrrlyn/bitvec).

I verified this was the actual cause of the issue by creating a fork of bitvec with no toolchain specified in a rust-toolchain file.

Strangely, this issue only seems to affect my OS X machines - Linux builds work as expected

Comment 1

[Mark Goodwin [:mgoodwin]]

Due to the fact that some builds invoke cargo with --frozen (e.g. Linux x64 debug Spidermonkey builds), we can't work around this by vendering from git branches. What can I do to help unblock things here?

Comment 2

[Nathan Froyd [:froydnj]]

(In reply to Mark Goodwin [:mgoodwin] from comment #1)

Due to the fact that some builds invoke cargo with --frozen (e.g. Linux x64 debug Spidermonkey builds), we can't work around this by vendering from git branches. What can I do to help unblock things here?

I don't understand this concern; we vendor plenty of crates from git branches (https://searchfox.org/mozilla-central/source/Cargo.toml#59-71). We also invoke basically all of our builds with --frozen; some of the Spidermonkey builds are the exception:

https://searchfox.org/mozilla-central/source/config/makefiles/rust.mk#22-28

Can you elaborate on your concern? We ought to be able to fork bitvec, remove rust-toolchain, and vendor from that branch instead.

Alex, do you know who might be able to help us here? It looks like rustup's magic toolchain overriding is getting in the way here:

https://github.com/rust-lang/rustup.rs#override-precedence

My impression is that this behavior is only triggered when cargo is really rustup in disguise. Since we've had problems with that, we go to some lengths to make sure we have the "real" cargo:

https://searchfox.org/mozilla-central/source/build/moz.configure/rust.configure#25-59

Do we have to apply the same treatment to rustc as well? In any case, since Mark is seeing different behavior on OS X and Mac, that would seem to imply that rustup...isn't the problem? Unless cargo has some kind of subtle bug here?

Comment 3

[Alex Crichton [:acrichto]]

Oh dear, this sounds bad! I was curious about why we haven't seen reports of this happening all over the place (as it seems like it would be quite common), so I did a bit of digging. You're right is that this is only triggered when the cargo used is the real Cargo, not the rustup wrapper. If you use the rustup wrapper it will implicitly set the RUSTUP_TOOLCHAIN env var which has higher precedence than rust-toolchain files. That would also explain why this is OSX only because that's the only platform where the rustup wrapper is avoided!

I wonder if perhaps RUSTUP_TOOLCHAIN should be set when the raw cargo is used? Otherwise I'm not sure best to fix this other than to recommend temporarily to crates to not check in rust-toolchain files which isn't super feasible...

Comment 4

[Nathan Froyd [:froydnj]]

(In reply to Alex Crichton [:acrichto] from comment #3)

Oh dear, this sounds bad! I was curious about why we haven't seen reports of this happening all over the place (as it seems like it would be quite common), so I did a bit of digging. You're right is that this is only triggered when the cargo used is the real Cargo, not the rustup wrapper. If you use the rustup wrapper it will implicitly set the RUSTUP_TOOLCHAIN env var which has higher precedence than rust-toolchain files. That would also explain why this is OSX only because that's the only platform where the rustup wrapper is avoided!

I am confused. Why is this triggered only when cargo is the "real" cargo? AFAICT, the "real" cargo shouldn't be paying any attention to rust-toolchain files; that's only done when the rustup wrapper is in play. So we seem to have a situation where:

1. cargo is looking at the rust-toolchain file.
2. cargo is the "real" cargo.

which seems impossible. Unless we should also be "unwrapping" rustc to remove the rustup wrapper as well? Trying the same sort of trick from:

https://searchfox.org/mozilla-central/source/build/moz.configure/rust.configure#25-59

with rustc suggests that might be happening. I'll try porting that patch over and Mark can see if it fixes the problem.

Comment 5

[Mark Goodwin [:mgoodwin]]

(In reply to Nathan Froyd [:froydnj] from comment #2)

(In reply to Mark Goodwin [:mgoodwin] from comment #1)

Due to the fact that some builds invoke cargo with --frozen (e.g. Linux x64 debug Spidermonkey builds), we can't work around this by vendering from git branches. What can I do to help unblock things here?

I don't understand this concern; we vendor plenty of crates from git branches (https://searchfox.org/mozilla-central/source/Cargo.toml#59-71). We also invoke basically all of our builds with --frozen; some of the Spidermonkey builds are the exception:

I misunderstood the failure that caused my "vendor from Git" to fail. No matter; I found a preferable workaround (for the short term) to the bitvec vendoring issue (though we'll likely want to use this in future since bit-vec is now no longer actively developed).

Comment 6

[Alex Crichton [:acrichto]]

Oh sorry so to clarify neither Cargo no rustc actually looks at these rust-toolchain files, it's only rustup which does that. The bug happens when you're executing the real Cargo paried with a rustup wrapper for rustc. The rustup wrapper for rustc will read rust-toolchain files since RUSTUP_TOOLCHAIN isn't specified in the environment (when typically it is if cargo is invoked as a rustup wrapper).

So when Cargo is the real cargo and it's invoking a rustup wrapper rustc, then rustc will read rust-toolchain and switch automatically. I believe it would work to remove the rustup wrapper around rustc as well because then nothing would read the rust-toolchain file

Comment 7

[Nathan Froyd [:froydnj]]

Attachment: Bug 1547196 - mock `subprocess.call` for python configure tests; r=#build

This method isn't called in our current test configuration, but
subsequent patches will activate codepaths in tests that call
subprocess.call.

Comment 8

[Nathan Froyd [:froydnj]]

Attachment: Bug 1547196 - remove rustup wrapper from `rustc` as well as `cargo`; r=glandium

Having rustc be rustup's wrapper for rustc means that we may
silently honor rustup's override mechanisms. We noticed this first on
OS X, where we use the "real" cargo but rustup's rustc wrapper,
and problems ensued when cargo thought it was using one version of
rustc, but actually wound up using something different.

It seems better to avoid silently interposing rustup's toolchain
override mechanisms everywhere, rather than having to special-case OS
X. So let's factor out a general mechanism for removing the wrappers
rustup provides and use that for both rustc and cargo. The tests
need adjusting because we weren't triggering the unwrapping cases
before; we don't yet test the case where we really do need to unwrap.
That test can be left for a future patch.

Depends on D29530

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c9f1a1db8fa2
remove rustup wrapper from `rustc` as well as `cargo`; r=glandium

Comment 10

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/c9f1a1db8fa2