diff mbox

[FFmpeg-devel,3/8] build: treat iconv like other autodetected libraries

Message ID 20170728115145.23169-4-u@pkh.me
State Accepted
Commit c9075d2c652bd90a5b559a9fa38dd0fd3de377e7
Headers show

Commit Message

Clément Bœsch July 28, 2017, 11:51 a.m. UTC
From: Clément Bœsch <cboesch@gopro.com>

---
 configure | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Comments

Nicolas George July 28, 2017, 11:57 a.m. UTC | #1
Le decadi 10 thermidor, an CCXXV, Clement Boesch a écrit :
> From: Clément Bœsch <cboesch@gopro.com>
> 
> ---
>  configure | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)

I am not sure exactly what the code does, but I think it must make a
difference between iconv provided by the libc on good Unix systems and
iconv provided by GNU libiconv.

When iconv is provided by the system's libc, then I think it should be
enabled automatically. It does not go against reproducible builds, I
think.

Regards,
Clément Bœsch July 28, 2017, 12:31 p.m. UTC | #2
On Fri, Jul 28, 2017 at 01:57:53PM +0200, Nicolas George wrote:
> Le decadi 10 thermidor, an CCXXV, Clement Boesch a écrit :
> > From: Clément Bœsch <cboesch@gopro.com>
> > 
> > ---
> >  configure | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> I am not sure exactly what the code does, but I think it must make a
> difference between iconv provided by the libc on good Unix systems and
> iconv provided by GNU libiconv.
> 
> When iconv is provided by the system's libc, then I think it should be
> enabled automatically.

I'm not changing the behaviour here, iconv is already autodetected.

Though, --disable-autodetect will disable its auto-detection, be it
a "system" or a randomly installed library.

> It does not go against reproducible builds, I
> think.

Well, being maintained by the base system or through a normal package
doesn't make much difference since the system can also gets updated with a
new iconv (major bumped).

Here are some exceptions I can think of: libc (well, hard to get around
this one) and pthread. AFAIK the later may be replaced by "native" code; I
think clang doesn't rely on a library for this, so I'm assuming there is
no such thing as a link to libpthread in the final program.

So, unless iconv is a native feature of the compiler causing the final
program not to link against any external library, I don't think iconv
should be enabled by default.

Regards,
Nicolas George July 28, 2017, 1:04 p.m. UTC | #3
Le decadi 10 thermidor, an CCXXV, Clement Boesch a écrit :
> Though, --disable-autodetect will disable its auto-detection, be it
> a "system" or a randomly installed library.

Yes, and I think it is a bad idea for iconv.

> Well, being maintained by the base system or through a normal package
> doesn't make much difference since the system can also gets updated with a
> new iconv (major bumped).

No, it cannot. I am not talking about a library, I am talking about the
API. When the iconv API is part of the system, it cannot change its
binary interface, unless the system gets bumped as a whole.

> Here are some exceptions I can think of: libc (well, hard to get around
> this one)

Well, iconv is part of the libc, so that is ok. It should be treated as
such, like getaddrinfo, i.e. in the SYSTEM_FUNCS list.

>	    and pthread. AFAIK the later may be replaced by "native" code; I
> think clang doesn't rely on a library for this, so I'm assuming there is
> no such thing as a link to libpthread in the final program.

I am rather dubious about that. It is not possible to make real threads
without system calls, and inlining system calls would be very very bad.

> So, unless iconv is a native feature XXXXXXXXXXXXXXX causing the final
> program not to link against any external library

It is exactly that on real Unix systems.

Regards,
diff mbox

Patch

diff --git a/configure b/configure
index a2ad72f7f4..5ba171d459 100755
--- a/configure
+++ b/configure
@@ -3596,6 +3596,8 @@  enable_weak cuda cuvid nvenc vda_framework videotoolbox videotoolbox_encoder
 # Enable compression/decompression libraries by default
 enable_weak zlib bzlib lzma
 
+enable_weak iconv
+
 disabled logging && logfile=/dev/null
 
 die_license_disabled() {
@@ -6179,7 +6181,7 @@  int main(void) { return 0; }
 EOF
 
 # Funny iconv installations are not unusual, so check it after all flags have been set
-disabled iconv || check_func_headers iconv.h iconv || check_lib iconv iconv.h iconv -liconv
+enabled iconv && check_func_headers iconv.h iconv || check_lib iconv iconv.h iconv -liconv
 
 enabled debug && add_cflags -g"$debuglevel" && add_asflags -g"$debuglevel"