Add bindings for iconv calls #2037

Minoru · 2021-01-17T19:48:29Z

No description provided.

rust-highfive · 2021-01-17T19:48:32Z

r? @JohnTitor

(rust-highfive has picked a reviewer for you, use r? to override)

Minoru · 2021-01-17T19:57:20Z

Already looking into the macOS failure!

Minoru · 2021-01-17T22:11:43Z

Sorry, I need a bit of help with this — I either misunderstand cfg_if! macro or target_os attribute.

The original commit, ac6ec1c, failed to build on macOS because it didn't link with iconv library. The next commit, e906ef7, added an appropriate link attribute, but the build still failed — and I don't see -liconv anywhere in the output. Adding a link attribute unconditionally works on macOS, but not other platforms (ddddc18), the latter being expected.

macOS build links with m and c libraries, and the only other place where those could come from is the "Android or OpenBSD" branch. But if I add iconv there, still no -liconv in the output (8d7a53c).

It feels like I'm adding the link attribute to the wrong place. Can anyone point me to the right one?

Minoru · 2021-01-25T15:43:36Z

99202f7 implements a new approach: build.rs emits rustc-link-lib=iconv on Apple platforms. This works, but might be confusing to future programmers, since all the other libraries are configured via the link attributes.

joshtriplett · 2021-01-29T00:47:29Z

src/unix/mod.rs

@@ -298,6 +299,7 @@ cfg_if! {
    } else if #[cfg(feature = "std")] {
        // cargo build, don't pull in anything extra as the libstd dep
        // already pulls in all libs.
+        extern {}


Could you clarify why you've added this?

Ah, sorry, it's a leftover from one of the experiments, 3553014. Will remove this in a second.

Minoru · 2021-01-29T09:57:30Z

Even though this PR passes CI, I'd really like some discussion on whether this even belongs to libc.

On one hand it definitely does, since iconv calls are part of POSIX, and are widely implemented.

On the other hand, the situation is a bit complicated since BDSs can have two implementations at once: one from libc.so (implementing POSIX), the other from libiconv.so (GNU libiconv implementing POSIX plus GNU extensions). libiconv uses different symbol names (libiconv_open() etc.), but the function names are the same, so the caller is none the wiser — the decision is made at linking time. (musl might not implement the GNU extensions either, I didn't check.)

I doubt it's the crate's job to decide which iconv implementation my app should link to, and so there are a few possible solutions:

don't provide these bindings at all, let some other crate do it;
provide a feature flag to pick between "libc iconv" and "libiconv iconv", regardless of the platform;
provide a feature flag to pick between "libc iconv" and "libiconv iconv", just on systems that don't use glibc (I believe the libc "type" is a part of a platform triple);
link to "libc iconv" unconditionally, let someone else provide libiconv bindings;
link to "libiconv iconv" unconditionally, let someone else provide libc.so bindings.

I only discovered all these complications once I opened the PR; if I knew of them in advance, I'd open an issue first. Sorry about that! Happy to close this and re-report as an issue if that's what you'd prefer.

semarie · 2021-01-29T10:00:15Z

at least on OpenBSD (but I think it is true for several others OS), iconv isn't part of system libc (neither part of libraries provided by the system). iconv is only available as third party library.

bors · 2021-02-02T03:37:03Z

☔ The latest upstream changes (presumably #2052) made this pull request unmergeable. Please resolve the merge conflicts.

Minoru · 2021-02-02T12:37:45Z

I still want to have the discussion, so I'll hold off making any changes until it's decided if these bindings have a place in this crate.

JohnTitor · 2021-02-10T03:47:56Z

Thanks for clarifying, hmm, then I doubt we should this to the UNIX module. And the tweak for Apple targets is tricky, as you said, so it should have some comments at least, I believe.

/cc @Amanieu Any thoughts on this?

Amanieu · 2021-02-10T08:49:16Z

I'm thinking option 4 might be the best:

link to "libc iconv" unconditionally, let someone else provide libiconv bindings;

However we should only provide bindings if this API is actually provided by libc. So for example this wouldn't be present on OpenBSD.

EDIT: Just to clarify, by "libc" I mean that the library is provided as part of the base OS. So we would still support it on Apple even though it is in libiconv instead of libc.

FreeBSD-likes all implement iconv: - DragonflyBSD: https://github.com/DragonFlyBSD/DragonFlyBSD/blob/bbb35c81f71fe2a0880a1f8bb77876ee98b63338/include/iconv.h - FreeBSD: https://github.com/freebsd/freebsd-src/blob/a6dc68c0e0f8a24ffaf0b4e78e58141ef7897047/include/iconv.h NetBSD-likes: - NetBSD: https://github.com/NetBSD/src/blob/81a39f60870b617d7fca299c126de6553d78cc5a/include/iconv.h - OpenBSD doesn't implement it macOS: apparently ships a conforming implementation as a separate library: https://stackoverflow.com/questions/57734434/libiconv-or-iconv-undefined-symbol-on-mac-osx/57734435#57734435 Linux: - glibc: https://sourceware.org/git/?p=glibc.git;a=blob;f=iconv/iconv.h;h=fdddf53d99c3046ef9c280db01a425c2f499043e;hb=HEAD - musl: https://git.musl-libc.org/cgit/musl/tree/include/iconv.h?id=455f96857f91d14e193219ca00969354a981c09c

Minoru · 2021-02-10T22:34:14Z

All done, please take a look.

Amanieu · 2021-02-10T23:29:44Z

@bors r+

bors · 2021-02-10T23:29:45Z

📌 Commit 3e4d684 has been approved by Amanieu

bors · 2021-02-10T23:29:54Z

⌛ Testing commit 3e4d684 with merge 2a2196d...

bors · 2021-02-11T00:28:43Z

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

semarie · 2021-02-11T07:22:26Z

libc-test/build.rs

@@ -360,6 +361,7 @@ fn test_openbsd(target: &str) {
        "pthread_np.h",
        "sys/syscall.h",
        "sys/shm.h",
+        "iconv.h",


this break the test on OpenBSD : no iconv.h file here

I just made #2067 to unbreak OpenBSD

Oops, sorry — that's leftovers from the time when the bindings were in the unix mod. The similar problem affects Android, Solari-sh, Redox and a few other targets, I'll submit a PR fixing that.

Done: #2068

Remove unused iconv.h includes These are left over from 3e4d684, which added includes to *all* platforms despite adding bindings only to *some* of them. This already broke OpenBSD which doesn't have iconv.h (fixed by 915d8fa), and is just distasteful, so down with those unused includes. (This is a continuation to #2037 and #2067.)

Don't unconditionally link libiconv on macOS. 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.

rust-highfive assigned JohnTitor Jan 17, 2021

rust-highfive added the S-waiting-on-review label Jan 17, 2021

Minoru mentioned this pull request Jan 28, 2021

Port utils::convert_text() and UTF-8 <-> locale converters to Rust newsboat/newsboat#1455

Merged

joshtriplett reviewed Jan 29, 2021

View reviewed changes

Minoru force-pushed the feature/iconv branch from 5d81ae2 to 3e4d684 Compare February 10, 2021 22:31

bors added S-waiting-on-bors and removed S-waiting-on-review labels Feb 10, 2021

bors merged commit 2a2196d into rust-lang:master Feb 11, 2021

semarie reviewed Feb 11, 2021

View reviewed changes

Minoru deleted the feature/iconv branch February 11, 2021 11:50

Minoru mentioned this pull request Feb 11, 2021

Remove unused iconv.h includes #2068

Merged

goffrie mentioned this pull request Apr 28, 2021

Don't unconditionally link libiconv on macOS. #2150

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add bindings for iconv calls #2037

Add bindings for iconv calls #2037

Minoru commented Jan 17, 2021

rust-highfive commented Jan 17, 2021

Minoru commented Jan 17, 2021

Minoru commented Jan 17, 2021 •

edited

Loading

Minoru commented Jan 25, 2021

joshtriplett Jan 29, 2021

Minoru Jan 29, 2021

Minoru commented Jan 29, 2021

semarie commented Jan 29, 2021

bors commented Feb 2, 2021

Minoru commented Feb 2, 2021

JohnTitor commented Feb 10, 2021

Amanieu commented Feb 10, 2021 •

edited

Loading

Minoru commented Feb 10, 2021

Amanieu commented Feb 10, 2021

bors commented Feb 10, 2021

bors commented Feb 10, 2021

bors commented Feb 11, 2021

semarie Feb 11, 2021

semarie Feb 11, 2021

Minoru Feb 11, 2021

Minoru Feb 11, 2021

Add bindings for iconv calls #2037

Add bindings for iconv calls #2037

Conversation

Minoru commented Jan 17, 2021

rust-highfive commented Jan 17, 2021

Minoru commented Jan 17, 2021

Minoru commented Jan 17, 2021 • edited Loading

Minoru commented Jan 25, 2021

joshtriplett Jan 29, 2021

Choose a reason for hiding this comment

Minoru Jan 29, 2021

Choose a reason for hiding this comment

Minoru commented Jan 29, 2021

semarie commented Jan 29, 2021

bors commented Feb 2, 2021

Minoru commented Feb 2, 2021

JohnTitor commented Feb 10, 2021

Amanieu commented Feb 10, 2021 • edited Loading

Minoru commented Feb 10, 2021

Amanieu commented Feb 10, 2021

bors commented Feb 10, 2021

bors commented Feb 10, 2021

bors commented Feb 11, 2021

semarie Feb 11, 2021

Choose a reason for hiding this comment

semarie Feb 11, 2021

Choose a reason for hiding this comment

Minoru Feb 11, 2021

Choose a reason for hiding this comment

Minoru Feb 11, 2021

Choose a reason for hiding this comment

Minoru commented Jan 17, 2021 •

edited

Loading

Amanieu commented Feb 10, 2021 •

edited

Loading