Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Consider not building std as a dylib. #35

Closed
ehuss opened this issue Sep 6, 2019 · 8 comments · Fixed by rust-lang/cargo#7353
Closed

Consider not building std as a dylib. #35

ehuss opened this issue Sep 6, 2019 · 8 comments · Fixed by rust-lang/cargo#7353
Labels
implementation Implementation exploration and tracking issues

Comments

@ehuss
Copy link
Contributor

ehuss commented Sep 6, 2019

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 rust-lang/rust#56443.

@ehuss ehuss added the implementation Implementation exploration and tracking issues label Sep 6, 2019
@Ericson2314
Copy link

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
Copy link
Member

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
Copy link
Contributor Author

ehuss commented Sep 6, 2019

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 rust-lang/cargo#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
Copy link
Member

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
Copy link
Contributor Author

ehuss commented Sep 6, 2019

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
Copy link
Member

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.

alexcrichton added a commit to alexcrichton/cargo that referenced this issue Sep 11, 2019
This commit forcibly prevents Cargo from building the `std` crate as a
`dylib`, even though libstd upstream lists a `dylib` crate type. We
ideally want a first-class feature for doing this one day, but for now
we can just hack around with the manifests to ensure that the `dylib`
crate type never shows up. Note that this is only supported for libstd,
and it's also all part of the unstable details of building std.

Closes rust-lang/wg-cargo-std-aware#35
bors added a commit to rust-lang/cargo that referenced this issue Sep 11, 2019
Don't build libstd as a `dylib`

This commit forcibly prevents Cargo from building the `std` crate as a
`dylib`, even though libstd upstream lists a `dylib` crate type. We
ideally want a first-class feature for doing this one day, but for now
we can just hack around with the manifests to ensure that the `dylib`
crate type never shows up. Note that this is only supported for libstd,
and it's also all part of the unstable details of building std.

Closes rust-lang/wg-cargo-std-aware#35
@mati865
Copy link

mati865 commented Sep 19, 2019

It was supposed to be closed by rust-lang/cargo#7353 but apparently it didn't work.

@alexcrichton
Copy link
Member

Indeed!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
implementation Implementation exploration and tracking issues
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants