rust-lang / wg-cargo-std-aware

Repo for working on "std aware cargo"
133 stars 8 forks source link

Consider not building std as a dylib. #35

Closed ehuss closed 4 years ago

ehuss commented 4 years ago

The current manifest for std builds it as both an rlib and dylib. The dylib isn't needed for Cargo (AFAIK, it is mainly for librustc_driver.so). We should figure out some way to prevent it from being built.

One problem this causes is that it causes a rebuild of everything when you switch compilers because the dylib does not include a hash in the filename. (dylibs without hashes are generally problematic, but that is a separate issue.)

See also https://github.com/rust-lang/rust/issues/56443.

Ericson2314 commented 4 years ago

The current manifest for std builds it as both an rlib and dylib....AFAIK, it is mainly for librustc_driver.so

Right so some workspace should require std as a dylib, not std itself. Looks like just a straight-up layer violation to me. Only root crates should decide how things are built.

alexcrichton commented 4 years ago

I definitely think we should do this, if only to just make the implementation more conservative. We can always add this back in later but it's really difficult to remove the dylib crate type after it's out there! I think the best way to do this will be to iterate over the libstd dependency graph and just forcibly delete the dylib crate type, we can eventually use something more first-class in Cargo but that's a long ways away (as in, has had basically zero design).

One problem this causes is that it causes a rebuild of everything when you switch compilers because the dylib does not include a hash in the filename. (dylibs without hashes are generally problematic, but that is a separate issue.)

I'm not sure I quite understand this, @ehuss can you clarify?

ehuss commented 4 years ago

I'm not sure I quite understand this, @ehuss can you clarify?

With a dylib, if you do cargo +nightly build, then cargo +stable build, then cargo +nightly build, the third build will rebuild the dylib because there is only one copy in the deps directory, and the previous build stomped on it. If that dylib is libstd, then that means everything will get rebuilt, which defeats the benefit of including the rustc version in the filename hash.

The dylib problem also comes up with having a global shared target directory. If two projects build the same dylib crate, but with different settings (different version, different features, etc.), it will stomp on the existing dylib, forcing a rebuild for the other project.

I thought there was a cargo issue somewhere about this, but I can't find it. (The closest is https://github.com/rust-lang/cargo/issues/6313, but the issue there is a bit more complex, but the root of the problem is that there is no hash in the dylib filename.)

It's been a long while since I looked at this, but IIRC you can't just tell rustc to use -C extra-filename and then have Cargo rename it because that hash gets embedded somewhere, but I may be remembering wrong.

alexcrichton commented 4 years ago

Aha yes that all makes sense!

One thing I'm wondering though, why does libstd not have a hash in its filename? Does it have to do with the dylib crate type being special in Cargo? I think that would solve this issue, right?

(it may be worth a separate issue to put a hash in the filename)

ehuss commented 4 years ago

libstd does have a hash in the filename when __CARGO_DEFAULT_LIB_METADATA is set (cc #34, which you closed 😜 ). Since cargo isn't setting that, it doesn't get a hash. More notes here.

alexcrichton commented 4 years ago

Ah ok, dylib is indeed special! I don't think we need to mix in that value into metadata but moreso we just need to have any metadata at all in there (I think rustc version is already mixed in?). We I think need to add a clause where you linked to the notes that if it's a libstd target that we give it metadata anyway.

mati865 commented 4 years ago

It was supposed to be closed by https://github.com/rust-lang/cargo/pull/7353 but apparently it didn't work.

alexcrichton commented 4 years ago

Indeed!