xiph / flac

Free Lossless Audio Codec
https://xiph.org/flac/
GNU Free Documentation License v1.3
1.58k stars 278 forks source link

`--enable-asm-optimizations` seems to disable the feature according to summary. #686

Closed TurtleWilly closed 2 months ago

TurtleWilly commented 3 months ago

I like to be explicit with my configure options for reproducible and documented builds down the line. So usually I use an --enable-xxx option, if the configure --help lists a --disable-xxx option and "enabled" is the default anyway and I want to make sure the feature is enabled.

flac's configure --help lists a --disable-asm-optimizations option, so to make it explicit and I use --enable-asm-optimizations, something like this:

./configure 
--prefix=/usr/local/silo/flac/1.4.3 
--disable-static
--enable-asm-optimizations
--enable-avx
--with-ogg=/usr/local/silo/libogg/latest
--with-libiconv-prefix=/usr/local/silo/libiconv/latest
--disable-version-from-git
--disable-silent-rules
--disable-debug
LDFLAGS='-Wl,-headerpad_max_install_names -Wl,-dead_strip_dylibs -Wl,-dead_strip'

To my surprise this doesn't work and actually does the opposite and disables the feature:

-=-=-=-=-=-=-=-=-=-= Configuration Complete =-=-=-=-=-=-=-=-=-=-

  Configuration summary :

    FLAC version : ............................ 1.4.3

    Host CPU : ................................ x86_64
    Host Vendor : ............................. apple
    Host OS : ................................. darwin14.5.0

    Compiler is GCC : ......................... no
    Compiler is Clang : ....................... yes
    Asm optimizations : ....................... no
    Ogg/FLAC support : ........................ yes
    Stack protector  : ........................ no
    Fuzzing support (Clang only) : ............ no

If AVX is enabled/disabled is not reported by the summary (missing feature!), so maybe it's the case there too?

As workaround I skipped both --enable-asm-optimizations (afterwards it reports "Asm optimizations : ..... yes"), and --enable-avx and have to hope for the best. But it leaves a uncertainty if flac is properly optimized or not and if things randomly may change with the next build/ version.

An unrelated note: --with-libiconv-prefix= does not work (it still grabs an old libiconv from somewhere else)

ktmf01 commented 3 months ago

De culprit is in this line: https://github.com/xiph/flac/blob/5f6a352921cd58360a6ace049ee26068fd5b888c/configure.ac#L82

-AC_ARG_ENABLE(asm-optimizations, AS_HELP_STRING([--disable-asm-optimizations],[Do not use any CPU specific optimization routines]), asm_opt=no, asm_opt=yes)
+AC_ARG_ENABLE(asm-optimizations, AS_HELP_STRING([--disable-asm-optimizations],[Do not use any CPU specific optimization routines]), asm_opt=${enableval}, asm_opt=yes)

should do the trick. The --enable-avx option does not have a similar problem. You can check this by looking whether #define FLAC__USE_AVX 1 is in the file config.h after running configure.

TurtleWilly commented 3 months ago

@ktmf01 Yes, verifying the config.h is in my documentation for now. 😎

ktmf01 commented 2 months ago

Could you check whether PR #693 fixes the iconv issue?

TurtleWilly commented 2 months ago

@ktmf01 --enable-asm-optimizations seems to work (needed an autoreconf after applying the patch).

--with-libiconv-prefix=/some/path by itself still seems to be a NOP. I only see "-liconv" in a verbose build output. Not -I/some/path/include nor -L/some/path/lib. Result is that it still grabs an old/ outdated /usr/lib/libiconv.2.dylib from OS X, not the wanted up-to-date one from /some/path/lib/libiconv.2.dylib. It's a common problem with libiconv in many packages. It's notoriously fickle.

The workaround is always to specify it all manually like this instead:

./configure \
    --prefix=/usr/local/silo/flac/1.4.3 \
    …
    CFLAGS='-I/usr/local/silo/libiconv/latest/include' \
    LDFLAGS='-L/usr/local/silo/libiconv/latest/lib' \
    LIBS='-liconv'

(I believe this is an GNU iconv upstream problem.)

ktmf01 commented 2 months ago

(I believe this is an GNU iconv upstream problem.)

Yes, I think it is. The only iconv thing in the configuration is the macro, which is copied directly from gnulib. Maybe you can try to get people there to fix, as they are probably way better qualified to do so.