[FFmpeg-devel] configure: add libm ldflags globally

Submitted by James Almer on Oct. 14, 2017, 3:59 p.m.

Details

Message ID 20171014155926.5276-1-jamrial@gmail.com
State New
Headers show

Commit Message

James Almer Oct. 14, 2017, 3:59 p.m.
It's used by every library, and by making it global we simplify a lot
of checks.

Signed-off-by: James Almer <jamrial@gmail.com>
---
 configure | 18 +++++++-----------
 1 file changed, 7 insertions(+), 11 deletions(-)

Comments

James Almer Oct. 16, 2017, 4:31 p.m.
On 10/14/2017 12:59 PM, James Almer wrote:
> It's used by every library, and by making it global we simplify a lot
> of checks.
> 
> Signed-off-by: James Almer <jamrial@gmail.com>
> ---
>  configure | 18 +++++++-----------
>  1 file changed, 7 insertions(+), 11 deletions(-)
> 
> diff --git a/configure b/configure
> index 22026ef832..a0bfb269e8 100755
> --- a/configure
> +++ b/configure
> @@ -3332,23 +3332,17 @@ cws2fws_extralibs="zlib_extralibs"
>  
>  # libraries, in linking order
>  avcodec_deps="avutil"
> -avcodec_suggest="libm"
>  avcodec_select="null_bsf"
>  avdevice_deps="avformat avcodec avutil"
> -avdevice_suggest="libm"
>  avfilter_deps="avutil"
> -avfilter_suggest="libm"
>  avformat_deps="avcodec avutil"
> -avformat_suggest="libm network"
> +avformat_suggest="network"
>  avresample_deps="avutil"
> -avresample_suggest="libm"
> -avutil_suggest="clock_gettime libm libdrm libmfx opencl user32 vaapi videotoolbox corefoundation corevideo coremedia wincrypt"
> +avutil_suggest="clock_gettime libdrm libmfx opencl user32 vaapi videotoolbox corefoundation corevideo coremedia wincrypt"
>  postproc_deps="avutil gpl"
> -postproc_suggest="libm"
>  swresample_deps="avutil"
> -swresample_suggest="libm libsoxr"
> +swresample_suggest="libsoxr"
>  swscale_deps="avutil"
> -swscale_suggest="libm"
>  
>  avcodec_extralibs="pthreads_extralibs iconv_extralibs"
>  avfilter_extralibs="pthreads_extralibs"
> @@ -5944,7 +5938,9 @@ enabled  lzma && check_lib lzma   lzma.h lzma_version_number -llzma
>  # On some systems dynamic loading requires no extra linker flags
>  check_lib libdl dlfcn.h "dlopen dlsym" || check_lib libdl dlfcn.h "dlopen dlsym" -ldl
>  
> -check_lib libm math.h sin -lm
> +# Add -lm to global extralibs if required. Every library uses it, and it simplifies
> +# several of the external library checks below.
> +check_lib libm math.h sin -lm && add_extralibs $libm_extralibs
>  
>  atan2f_args=2
>  copysign_args=2
> @@ -6098,7 +6094,7 @@ enabled libx264           && { use_pkg_config libx264 x264 "stdint.h x264.h" x26
>                                 enable libx262; }
>  enabled libx265           && require_pkg_config libx265 x265 x265.h x265_api_get &&
>                               require_cpp_condition x265.h "X265_BUILD >= 68"
> -enabled libxavs           && require libxavs "stdint.h xavs.h" xavs_encoder_encode "-lxavs $pthreads_extralibs $libm_extralibs"
> +enabled libxavs           && require libxavs "stdint.h xavs.h" xavs_encoder_encode "-lxavs $pthreads_extralibs"
>  enabled libxvid           && require libxvid xvid.h xvid_global -lxvidcore
>  enabled libzimg           && require_pkg_config libzimg "zimg >= 2.3.0" zimg.h zimg_get_api_version
>  enabled libzmq            && require_pkg_config libzmq libzmq zmq.h zmq_ctx_new

Ping for this and the other two patches in the thread.
Jan Ekström Oct. 16, 2017, 5:36 p.m.
On Mon, Oct 16, 2017 at 7:31 PM, James Almer <jamrial@gmail.com> wrote:
> On 10/14/2017 12:59 PM, James Almer wrote:
>> It's used by every library, and by making it global we simplify a lot
>> of checks.
>>

LGTM.

I dislike the fact that we have to fix issues caused by 3rd party
libraries' pkg-config files but at this point the simplification for
things we also utilize kind of makes sense.

Jan
James Almer Oct. 16, 2017, 9:02 p.m.
On 10/16/2017 2:36 PM, Jan Ekstrom wrote:
> On Mon, Oct 16, 2017 at 7:31 PM, James Almer <jamrial@gmail.com> wrote:
>> On 10/14/2017 12:59 PM, James Almer wrote:
>>> It's used by every library, and by making it global we simplify a lot
>>> of checks.
>>>
> 
> LGTM.
> 
> I dislike the fact that we have to fix issues caused by 3rd party
> libraries' pkg-config files but at this point the simplification for
> things we also utilize kind of makes sense.

Some failures are indeed pkg-config files not listing libm and pthreads
as required, but with others we're doing custom checks, the require()
ones, where we're the ones that should state what libraries to link to
in static builds. This is the case with libilbc, libmysofa and
apparently libmp3lame.

The remaining issue after this patchset is applied is linking to the C++
standard library with some external deps. According to Jörn Heusipp, the
libopenmpt maintainer, it's not something that can be safely included in
a pkg-config file, and by hardcoding a -lstdc++ ldflag in configure
checks we're apparently breaking detection on systems where the compiler
uses libc++, like it seems to be the case with Clang on MacOS.

We were hardcoding -lstdc++ to the check of some libraries even before
the commit that made extralibs module-specific rather than global, and
after it i had to do it with a couple more, but if that's not the
correct approach then i don't what should be done.
Perhaps it's something the user will have to manually deal with using
--extra-libs.
Hendrik Leppkes Oct. 16, 2017, 9:38 p.m.
On Mon, Oct 16, 2017 at 11:02 PM, James Almer <jamrial@gmail.com> wrote:
> On 10/16/2017 2:36 PM, Jan Ekstrom wrote:
>> On Mon, Oct 16, 2017 at 7:31 PM, James Almer <jamrial@gmail.com> wrote:
>>> On 10/14/2017 12:59 PM, James Almer wrote:
>>>> It's used by every library, and by making it global we simplify a lot
>>>> of checks.
>>>>
>>
>> LGTM.
>>
>> I dislike the fact that we have to fix issues caused by 3rd party
>> libraries' pkg-config files but at this point the simplification for
>> things we also utilize kind of makes sense.
>
> Some failures are indeed pkg-config files not listing libm and pthreads
> as required, but with others we're doing custom checks, the require()
> ones, where we're the ones that should state what libraries to link to
> in static builds. This is the case with libilbc, libmysofa and
> apparently libmp3lame.
>
> The remaining issue after this patchset is applied is linking to the C++
> standard library with some external deps. According to Jörn Heusipp, the
> libopenmpt maintainer, it's not something that can be safely included in
> a pkg-config file, and by hardcoding a -lstdc++ ldflag in configure
> checks we're apparently breaking detection on systems where the compiler
> uses libc++, like it seems to be the case with Clang on MacOS.
>

Perhaps such libraries shouldn't hardcode -lstdc++ in there, but
dynamically put whichever C++ library they built against in there
instead?
Its not like you can actually use a static library build with another
C++ library, that *may* randomly work, but its not something anyone
should be doing.

- Hendrik
Jan Ekström Oct. 16, 2017, 10:13 p.m.
On Tue, Oct 17, 2017 at 12:38 AM, Hendrik Leppkes <h.leppkes@gmail.com> wrote:
> Perhaps such libraries shouldn't hardcode -lstdc++ in there, but
> dynamically put whichever C++ library they built against in there
> instead?
> Its not like you can actually use a static library build with another
> C++ library, that *may* randomly work, but its not something anyone
> should be doing.
>
> - Hendrik

Yup,

The project should know which C++ runtime it is built against and put
that into the .pc file's Libs.private. If the project doesn't have an
idea on how to automate this, a solution like the one utilized by zimg
is valid as well:

* Check for an environment variable during configuration and set the
default to `-lstdc++`
(https://github.com/sekrit-twc/zimg/blob/ae673e02c8e934915e05daeb120c7cb1b500a0a4/configure.ac#L55..L56)
* Set that value in your pkg-config template
(https://github.com/sekrit-twc/zimg/blob/ae673e02c8e934915e05daeb120c7cb1b500a0a4/zimg.pc.in#L13)

This way in case the value is incorrect the user can set the
environment variable, and then get what is needed.


Jan
Carl Eugen Hoyos Oct. 17, 2017, 1:04 a.m.
2017-10-16 23:02 GMT+02:00 James Almer <jamrial@gmail.com>:
> and by hardcoding a -lstdc++ ldflag in configure checks we're
> apparently breaking detection on systems where the compiler
> uses libc++, like it seems to be the case with Clang on MacOS.

Did you test this?
I ask because I was never able to reproduce (it
was claimed here before).

Carl Eugen
James Almer Oct. 17, 2017, 1:32 a.m.
On 10/16/2017 10:04 PM, Carl Eugen Hoyos wrote:
> 2017-10-16 23:02 GMT+02:00 James Almer <jamrial@gmail.com>:
>> and by hardcoding a -lstdc++ ldflag in configure checks we're
>> apparently breaking detection on systems where the compiler
>> uses libc++, like it seems to be the case with Clang on MacOS.
> 
> Did you test this?
> I ask because I was never able to reproduce (it
> was claimed here before).
> 
> Carl Eugen

No, i can't since i lack such a system. Otherwise I'd have already
removed all the old hardcoded checks for -lstdc++ and never added new ones.
Jörn Heusipp Oct. 17, 2017, 8:01 a.m.
On 10/16/2017 11:38 PM, Hendrik Leppkes wrote:
> On Mon, Oct 16, 2017 at 11:02 PM, James Almer <jamrial@gmail.com> wrote:

>> The remaining issue after this patchset is applied is linking to the C++
>> standard library with some external deps. According to Jörn Heusipp, the
>> libopenmpt maintainer, it's not something that can be safely included in
>> a pkg-config file, and by hardcoding a -lstdc++ ldflag in configure
>> checks we're apparently breaking detection on systems where the compiler
>> uses libc++, like it seems to be the case with Clang on MacOS.

> Perhaps such libraries shouldn't hardcode -lstdc++ in there, but
> dynamically put whichever C++ library they built against in there
> instead?

The problem here is, that we (libopenmpt) do not actually know what the 
name of the standard library is. It is chosen basically by the toolchain 
and there is no standard way to ask it what it uses. There is also no 
standard autoconf check which determines it (but the more I think about 
the issue, the more I think there actually needs to be one).

> Its not like you can actually use a static library build with another
> C++ library, that *may* randomly work, but its not something anyone
> should be doing.

Correct, mismatching C++ standard libraries will not work.

Regards,
Jörn (libopenmpt maintainer)
Jörn Heusipp Oct. 17, 2017, 8:01 a.m.
On 10/17/2017 12:13 AM, Jan Ekstrom wrote:
> On Tue, Oct 17, 2017 at 12:38 AM, Hendrik Leppkes <h.leppkes@gmail.com> wrote:
>> Perhaps such libraries shouldn't hardcode -lstdc++ in there, but
>> dynamically put whichever C++ library they built against in there
>> instead?
>> Its not like you can actually use a static library build with another
>> C++ library, that *may* randomly work, but its not something anyone
>> should be doing.
>>
>> - Hendrik
> 
> Yup,
> 
> The project should know which C++ runtime it is built against and put
> that into the .pc file's Libs.private. If the project doesn't have an
> idea on how to automate this, a solution like the one utilized by zimg
> is valid as well:
> 
> * Check for an environment variable during configuration and set the
> default to `-lstdc++`
> (https://github.com/sekrit-twc/zimg/blob/ae673e02c8e934915e05daeb120c7cb1b500a0a4/configure.ac#L55..L56)
> * Set that value in your pkg-config template
> (https://github.com/sekrit-twc/zimg/blob/ae673e02c8e934915e05daeb120c7cb1b500a0a4/zimg.pc.in#L13)

As already explained in another mail, I actually have no idea how to 
reliably determine the name of the C++ standard library. (see 
https://lists.freedesktop.org/archives/pkg-config/2016-August/001055.html 
on the pkg-config mailing list for a similar discussion).

Do you happen to know of any other libraries that do exactly that (use 
of STL_LIBS variable)? If this is somewhat common practice I could add 
support for that variable to libopenmpt.
If it is not, I am considering adding a configure option to libopenmpt, 
something like "--pkgconfig-static-cxxstdlib=-lstdc++" (and also add 
proper documentation about this issue with static linking).
However I do not like defaulting it to some guess (libstdc++), as a 
wrong value here is worse than no value (additional libs can be added 
easily by libraries or programs that use libopenmpt, removing/ignoring 
wrong libs from the .pc file is harder and will cause even more trouble).

For reference: https://bugs.openmpt.org/view.php?id=1045 (libopenmpt issue)

Regards,
Jörn (libopenmpt maintainer)
Jörn Heusipp Oct. 17, 2017, 8:01 a.m.
On 10/17/2017 03:04 AM, Carl Eugen Hoyos wrote:
> 2017-10-16 23:02 GMT+02:00 James Almer <jamrial@gmail.com>:
>> and by hardcoding a -lstdc++ ldflag in configure checks we're
>> apparently breaking detection on systems where the compiler
>> uses libc++, like it seems to be the case with Clang on MacOS.
> 
> Did you test this?
> I ask because I was never able to reproduce (it
> was claimed here before).

My FreeBSD (10.3, rather minimal install) system does not have any 
libstdc++. "-lstdc++" thus simply fails to link and clearly is the wrong 
thing to do.

I cannot test on macOS.

Regards,
Jörn (libopenmpt maintainer)

Patch hide | download patch | download mbox

diff --git a/configure b/configure
index 22026ef832..a0bfb269e8 100755
--- a/configure
+++ b/configure
@@ -3332,23 +3332,17 @@  cws2fws_extralibs="zlib_extralibs"
 
 # libraries, in linking order
 avcodec_deps="avutil"
-avcodec_suggest="libm"
 avcodec_select="null_bsf"
 avdevice_deps="avformat avcodec avutil"
-avdevice_suggest="libm"
 avfilter_deps="avutil"
-avfilter_suggest="libm"
 avformat_deps="avcodec avutil"
-avformat_suggest="libm network"
+avformat_suggest="network"
 avresample_deps="avutil"
-avresample_suggest="libm"
-avutil_suggest="clock_gettime libm libdrm libmfx opencl user32 vaapi videotoolbox corefoundation corevideo coremedia wincrypt"
+avutil_suggest="clock_gettime libdrm libmfx opencl user32 vaapi videotoolbox corefoundation corevideo coremedia wincrypt"
 postproc_deps="avutil gpl"
-postproc_suggest="libm"
 swresample_deps="avutil"
-swresample_suggest="libm libsoxr"
+swresample_suggest="libsoxr"
 swscale_deps="avutil"
-swscale_suggest="libm"
 
 avcodec_extralibs="pthreads_extralibs iconv_extralibs"
 avfilter_extralibs="pthreads_extralibs"
@@ -5944,7 +5938,9 @@  enabled  lzma && check_lib lzma   lzma.h lzma_version_number -llzma
 # On some systems dynamic loading requires no extra linker flags
 check_lib libdl dlfcn.h "dlopen dlsym" || check_lib libdl dlfcn.h "dlopen dlsym" -ldl
 
-check_lib libm math.h sin -lm
+# Add -lm to global extralibs if required. Every library uses it, and it simplifies
+# several of the external library checks below.
+check_lib libm math.h sin -lm && add_extralibs $libm_extralibs
 
 atan2f_args=2
 copysign_args=2
@@ -6098,7 +6094,7 @@  enabled libx264           && { use_pkg_config libx264 x264 "stdint.h x264.h" x26
                                enable libx262; }
 enabled libx265           && require_pkg_config libx265 x265 x265.h x265_api_get &&
                              require_cpp_condition x265.h "X265_BUILD >= 68"
-enabled libxavs           && require libxavs "stdint.h xavs.h" xavs_encoder_encode "-lxavs $pthreads_extralibs $libm_extralibs"
+enabled libxavs           && require libxavs "stdint.h xavs.h" xavs_encoder_encode "-lxavs $pthreads_extralibs"
 enabled libxvid           && require libxvid xvid.h xvid_global -lxvidcore
 enabled libzimg           && require_pkg_config libzimg "zimg >= 2.3.0" zimg.h zimg_get_api_version
 enabled libzmq            && require_pkg_config libzmq libzmq zmq.h zmq_ctx_new