diff mbox

[FFmpeg-devel,3/3] configure: fail if autodetect-libraries are requested but not found

Message ID 354655d7-339a-a897-f1ca-5234e8a86424@googlemail.com
State Accepted
Headers show

Commit Message

Andreas Cadhalpun Dec. 1, 2016, 11:44 p.m. UTC 
On 01.12.2016 01:34, Michael Niedermayer wrote:
> On Thu, Dec 01, 2016 at 12:22:50AM +0100, Andreas Cadhalpun wrote:
>> @@ -6381,6 +6410,11 @@ for thread in $THREADS_LIST; do
>>      fi
>>  done
>>  
>> +# Check if requested libraries were found.
>> +for lib in $AUTODETECT_LIBS; do
>> +    requested $lib && ! enabled $lib && die "ERROR: $lib requested but not found";
>> +done
> 
> This must be after check_deps as that can disable vaapi

I agree, but...

> unless i miss something

...it is already after check_deps. ;)

> also in the same light i think things like:
> 
> enabled vaapi &&
>     check_lib va/va.h vaInitialize -lva ||
>     disable vaapi
> 
> should disable vaapi through having a entry in vaapi_deps= ...

There is already va_va_h in vaapi_deps and I'm not sure, what else
you'd like to have there. However, it seems independent of this patch.

> And then check_deps should check for requested when disabling things
> that way it should be possible to print why something is being disabled
> If that works that extra information should be quite usefull

More information never hurts, so I added some specific error messages
in check_deps.

> also off topic but this can eventually be extended to cover more
> things than just these libs

Possibly.

> actually i wanted to cleanup configure and that stuff but
> its one of these things that kept sliding down on my todo ..

That's probably because as long as configure works a cleanup
isn't really important...

On 01.12.2016 09:48, Carl Eugen Hoyos wrote:
> Needs a Changelog

Added, updated patch is attached.

> and a NEWS entry.

As in "put the Changelog entry on the website"?
Fine for me if you think that'd be useful.

> Have you tested the patch?

Of course, and I made sure that the newly added functionality actually works,
however I can't possibly test all combinations of configure options in all
sorts of environments for regressions, so help in testing e.g. the more
exotic ones would be welcome.

> It was tried before and the result were different regressions iirc.

Can you provide links to previous attempts and their regressions so that
I can make sure they don't happen with my patch?

Best regards,
Andreas

Comments

Michael Niedermayer Dec. 2, 2016, 6:20 p.m. UTC | #1 
On Fri, Dec 02, 2016 at 12:44:41AM +0100, Andreas Cadhalpun wrote:
> On 01.12.2016 01:34, Michael Niedermayer wrote:
> > On Thu, Dec 01, 2016 at 12:22:50AM +0100, Andreas Cadhalpun wrote:
> >> @@ -6381,6 +6410,11 @@ for thread in $THREADS_LIST; do
> >>      fi
> >>  done
> >>  
> >> +# Check if requested libraries were found.
> >> +for lib in $AUTODETECT_LIBS; do
> >> +    requested $lib && ! enabled $lib && die "ERROR: $lib requested but not found";
> >> +done
> > 
> > This must be after check_deps as that can disable vaapi
> 
> I agree, but...
> 
> > unless i miss something
> 
> ...it is already after check_deps. ;)
> 

> > also in the same light i think things like:
> > 
> > enabled vaapi &&
> >     check_lib va/va.h vaInitialize -lva ||
> >     disable vaapi
> > 
> > should disable vaapi through having a entry in vaapi_deps= ...
> 
> There is already va_va_h in vaapi_deps and I'm not sure, what else
> you'd like to have there.

i see a "disable vaapi" here but that should be done by the dependency
or why is it done here ?


> However, it seems independent of this patch.

yes


iam ok with the patch if other are and its tested

thx

[...]
Andreas Cadhalpun Dec. 2, 2016, 10:24 p.m. UTC | #2 
On 02.12.2016 19:20, Michael Niedermayer wrote:
> On Fri, Dec 02, 2016 at 12:44:41AM +0100, Andreas Cadhalpun wrote:
>> There is already va_va_h in vaapi_deps and I'm not sure, what else
>> you'd like to have there.
> 
> i see a "disable vaapi" here but that should be done by the dependency
> or why is it done here ?

Now I see what you mean.

>> However, it seems independent of this patch.
> 
> yes
> 
> 
> iam ok with the patch if other are and its tested

I've tested for all AUTODETECT_LIBS that enabling the when configure
doesn't find them triggers a configure failure after the patch.
I've also check for a few libs that all combinations of --enable,
--disable, no configure flag and library installed or not does the
right thing.

Do you think there is something else that should be tested?

Best regards,
Andreas
Michael Niedermayer Dec. 3, 2016, 12:25 a.m. UTC | #3 
On Fri, Dec 02, 2016 at 11:24:12PM +0100, Andreas Cadhalpun wrote:
> On 02.12.2016 19:20, Michael Niedermayer wrote:
> > On Fri, Dec 02, 2016 at 12:44:41AM +0100, Andreas Cadhalpun wrote:
> >> There is already va_va_h in vaapi_deps and I'm not sure, what else
> >> you'd like to have there.
> > 
> > i see a "disable vaapi" here but that should be done by the dependency
> > or why is it done here ?
> 
> Now I see what you mean.
> 
> >> However, it seems independent of this patch.
> > 
> > yes
> > 
> > 
> > iam ok with the patch if other are and its tested
> 
> I've tested for all AUTODETECT_LIBS that enabling the when configure
> doesn't find them triggers a configure failure after the patch.
> I've also check for a few libs that all combinations of --enable,
> --disable, no configure flag and library installed or not does the
> right thing.
> 

> Do you think there is something else that should be tested?

i dont know, maybe give others a day to comment before pushing
in case someone has an idea

[...]
Andreas Cadhalpun Dec. 10, 2016, 6:45 p.m. UTC | #4 
On 03.12.2016 01:25, Michael Niedermayer wrote:
> On Fri, Dec 02, 2016 at 11:24:12PM +0100, Andreas Cadhalpun wrote:
>> On 02.12.2016 19:20, Michael Niedermayer wrote:
>>> iam ok with the patch if other are and its tested
>>
>> I've tested for all AUTODETECT_LIBS that enabling the when configure
>> doesn't find them triggers a configure failure after the patch.
>> I've also check for a few libs that all combinations of --enable,
>> --disable, no configure flag and library installed or not does the
>> right thing.
>>
> 
>> Do you think there is something else that should be tested?
> 
> i dont know, maybe give others a day to comment before pushing
> in case someone has an idea

After a week without further comments I've pushed this now.

Best regards,
Andreas
diff mbox

Patch

From 68a5545becacd8c3c9d26b16e2ce9aa236e66d6b Mon Sep 17 00:00:00 2001
From: Andreas Cadhalpun <Andreas.Cadhalpun@googlemail.com>
Date: Wed, 30 Nov 2016 23:50:17 +0100
Subject: [PATCH 3/3] configure: fail if autodetect-libraries are requested but
 not found

Signed-off-by: Andreas Cadhalpun <Andreas.Cadhalpun@googlemail.com>
---
 Changelog |  1 +
 configure | 78 +++++++++++++++++++++++++++++++++++++++++++++------------------
 2 files changed, 57 insertions(+), 22 deletions(-)

diff --git a/Changelog b/Changelog
index 71c9273..21d7ed4 100644
--- a/Changelog
+++ b/Changelog
@@ -6,6 +6,7 @@  version <next>:
 - add internal ebur128 library, remove external libebur128 dependency
 - Pro-MPEG CoP #3-R2 FEC protocol
 - premultiply video filter
+- configure now fails if autodetect-libraries are requested but not found
 
 version 3.2:
 - libopenmpt demuxer
diff --git a/configure b/configure
index c0f31a7..2747694 100755
--- a/configure
+++ b/configure
@@ -597,6 +597,13 @@  popvar(){
     done
 }
 
+request(){
+    for var in $*; do
+        eval ${var}_requested=yes
+        eval $var=
+    done
+}
+
 enable(){
     set_all yes $*
 }
@@ -653,6 +660,11 @@  enable_deep_weak(){
     done
 }
 
+requested(){
+    test "${1#!}" = "$1" && op='=' || op=!=
+    eval test "x\$${1#!}_requested" $op "xyes"
+}
+
 enabled(){
     test "${1#!}" = "$1" && op='=' || op=!=
     eval test "x\$${1#!}" $op "xyes"
@@ -724,9 +736,9 @@  do_check_deps(){
 
         [ -n "$dep_ifa" ] && { enabled_all $dep_ifa && enable_weak $cfg; }
         [ -n "$dep_ifn" ] && { enabled_any $dep_ifn && enable_weak $cfg; }
-        enabled_all  $dep_all || disable $cfg
-        enabled_any  $dep_any || disable $cfg
-        disabled_any $dep_sel && disable $cfg
+        enabled_all  $dep_all || { disable $cfg && requested $cfg && die "ERROR: $cfg requested, but not all dependencies are satisfied: $dep_all"; }
+        enabled_any  $dep_any || { disable $cfg && requested $cfg && die "ERROR: $cfg requested, but not any dependency is satisfied: $dep_any"; }
+        disabled_any $dep_sel && { disable $cfg && requested $cfg && die "ERROR: $cfg requested, but some selected dependency is unsatisfied: $dep_sel"; }
 
         if enabled $cfg; then
             enable_deep $dep_sel
@@ -1481,10 +1493,25 @@  EXAMPLE_LIST="
     transcode_aac_example
     transcoding_example
 "
+EXTERNAL_AUTODETECT_LIBRARY_LIST="
+    bzlib
+    iconv
+    libxcb
+    libxcb_shm
+    libxcb_shape
+    libxcb_xfixes
+    lzma
+    schannel
+    sdl
+    sdl2
+    securetransport
+    xlib
+    zlib
+"
 
 EXTERNAL_LIBRARY_LIST="
+    $EXTERNAL_AUTODETECT_LIBRARY_LIST
     avisynth
-    bzlib
     chromaprint
     crystalhd
     decklink
@@ -1492,7 +1519,6 @@  EXTERNAL_LIBRARY_LIST="
     gcrypt
     gmp
     gnutls
-    iconv
     jni
     ladspa
     libass
@@ -1545,42 +1571,26 @@  EXTERNAL_LIBRARY_LIST="
     libx264
     libx265
     libxavs
-    libxcb
-    libxcb_shm
-    libxcb_shape
-    libxcb_xfixes
     libxvid
     libzimg
     libzmq
     libzvbi
-    lzma
     mediacodec
     netcdf
     openal
     opencl
     opengl
     openssl
-    schannel
-    sdl
-    sdl2
-    securetransport
     videotoolbox
     x11grab
-    xlib
-    zlib
 "
-
-HWACCEL_LIBRARY_LIST="
+HWACCEL_AUTODETECT_LIBRARY_LIST="
     audiotoolbox
     cuda
     cuvid
     d3d11va
     dxva2
-    libmfx
-    libnpp
-    mmal
     nvenc
-    omx
     vaapi
     vda
     vdpau
@@ -1588,6 +1598,14 @@  HWACCEL_LIBRARY_LIST="
     xvmc
 "
 
+HWACCEL_LIBRARY_LIST="
+    $HWACCEL_AUTODETECT_LIBRARY_LIST
+    libmfx
+    libnpp
+    mmal
+    omx
+"
+
 DOCUMENT_LIST="
     doc
     htmlpages
@@ -1684,6 +1702,12 @@  ATOMICS_LIST="
     atomics_win32
 "
 
+AUTODETECT_LIBS="
+    $EXTERNAL_AUTODETECT_LIBRARY_LIST
+    $HWACCEL_AUTODETECT_LIBRARY_LIST
+    $THREADS_LIST
+"
+
 ARCH_LIST="
     aarch64
     alpha
@@ -3482,6 +3506,11 @@  for e in $env; do
     eval "export $e"
 done
 
+# Mark specifically enabled, but normally autodetected libraries as requested.
+for lib in $AUTODETECT_LIBS; do
+    enabled $lib && request $lib
+done
+
 # Enable platform codecs by default.
 # Enable hwaccels by default.
 for lib in audiotoolbox d3d11va dxva2 vaapi vda vdpau videotoolbox_hwaccel xvmc xlib \
@@ -6379,6 +6408,11 @@  for thread in $THREADS_LIST; do
     fi
 done
 
+# Check if requested libraries were found.
+for lib in $AUTODETECT_LIBS; do
+    requested $lib && ! enabled $lib && die "ERROR: $lib requested but not found";
+done
+
 enabled zlib && add_cppflags -DZLIB_CONST
 
 # conditional library dependencies, in linking order
-- 
2.10.2