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

Don't unconditionally link libiconv on macOS. #2150

Merged
merged 1 commit into from Apr 29, 2021
Merged

Conversation

goffrie
Copy link
Contributor

@goffrie goffrie commented Apr 28, 2021

This adds #[link(name = "iconv")] to just the iconv symbols so that the
library is only linked if the symbols are used.

#2037 added a build.rs directive to always link libiconv on Apple platforms,
but that shouldn't be necessary. With this change, we can see using otool -L
that a binary that uses an iconv symbol links libiconv, but other binaries
don't.

Note that this can only be seen with a rustc prior to nightly-2021-03-09, as
nightly-2021-03-10 includes rust-lang/rust#82731 which
includes #2037, therefore unconditionally linking all Rust binaries to
libiconv.

This adds `#[link(name = "iconv")]` to just the iconv symbols so that the
library is only linked if the symbols are used.
@rust-highfive
Copy link

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @JohnTitor (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@JohnTitor
Copy link
Member

r? @Amanieu

@rust-highfive rust-highfive assigned Amanieu and unassigned JohnTitor Apr 29, 2021
@Amanieu
Copy link
Member

Amanieu commented Apr 29, 2021

If CI is happy then it's fine.

@bors r+

@bors
Copy link
Contributor

bors commented Apr 29, 2021

📌 Commit 28f07a4 has been approved by Amanieu

@bors
Copy link
Contributor

bors commented Apr 29, 2021

⌛ Testing commit 28f07a4 with merge e584862...

@bors
Copy link
Contributor

bors commented Apr 29, 2021

☀️ Test successful - checks-actions, checks-cirrus-freebsd-11, checks-cirrus-freebsd-12, checks-cirrus-freebsd-13
Approved by: Amanieu
Pushing e584862 to master...

@bors bors merged commit e584862 into rust-lang:master Apr 29, 2021
@danieldk danieldk mentioned this pull request May 8, 2021
10 tasks
danieldk added a commit to danieldk/nixpkgs that referenced this pull request May 8, 2021
Many buildRustPackage-based derivations require libiconv as a build
input on Darwin. This is caused by `libc` unconditionally linking
against libiconv. However, this was recently changed:

rust-lang/libc#2150

We will probably see this dependency disappear over the coming months
once a new `libc` crate version is released and other crates adopt
this new version.

Since this is already an optional dependency and it will disappear
in the coming time, we should probably not unconditionally add it
to all Rust crates.
@winterqt
Copy link

winterqt commented Aug 1, 2021

@goffrie It seems that libiconv is still unconditionally linked to all Rust binaries on macOS:

$ rustc --version
rustc 1.56.0-nightly (1f0a591b3 2021-07-30)
$ cat main.rs
fn main() {}
$ rustc main.rs
$ otool -L main
main:
	/usr/lib/libSystem.B.dylib (compatibility version 1.0.0, current version 1292.100.5)
	/usr/lib/libresolv.9.dylib (compatibility version 1.0.0, current version 1.0.0)
	/usr/lib/libiconv.2.dylib (compatibility version 7.0.0, current version 7.0.0)

The libc version in this compilers std is 0.2.98, which includes this patch. Do you have any idea why this is still happening?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants