diff mbox

[FFmpeg-devel] configure: <fflib>_deps: validate, reduce sensitivity

Message ID 1937160471.4193679.1535495251907@mail.yahoo.com
State New
Headers show

Commit Message

avih Aug. 28, 2018, 10:27 p.m. UTC
I analyzed the linkage failures at:
http://ffmpeg.org/pipermail/ffmpeg-devel/2018-August/233674.html

After finding their immediate cause - expecting a specific output
order from unique() - I realized that there might be a related issue
where devs are expected to maintain <fflib>_deps (e.g. avfilter_deps)
in linking order. This can be awkward and bug-prone if the list is
not empty. I noticed at least one such existing order bug.

This patch makes it easier to add to these lists, in any order,
while also addressing few smaller issues and adding validation.

More details at the commit message of the attached patch.

Avi

Comments

Michael Niedermayer Aug. 30, 2018, 11:45 p.m. UTC | #1
On Tue, Aug 28, 2018 at 10:27:31PM +0000, avih wrote:
> I analyzed the linkage failures at:
> http://ffmpeg.org/pipermail/ffmpeg-devel/2018-August/233674.html
> 
> After finding their immediate cause - expecting a specific output
> order from unique() - I realized that there might be a related issue
> where devs are expected to maintain <fflib>_deps (e.g. avfilter_deps)
> in linking order. This can be awkward and bug-prone if the list is
> not empty. I noticed at least one such existing order bug.
> 
> This patch makes it easier to add to these lists, in any order,
> while also addressing few smaller issues and adding validation.
> 
> More details at the commit message of the attached patch.
> 
> Avi

>  configure |   46 ++++++++++++++++++++++++++++++++++++----------
>  1 file changed, 36 insertions(+), 10 deletions(-)
> 2333311c452a411b782b92743cc0df83746b4c89  0001-configure-fflib-_deps-validate-reduce-sensitivity.patch
> From 4940963648cb431f1a8e827adefb58d318186b38 Mon Sep 17 00:00:00 2001
> From: "Avi Halachmi (:avih)" <avihpit@yahoo.com>
> Date: Tue, 28 Aug 2018 17:14:55 +0300
> Subject: [PATCH] configure: <fflib>_deps: validate, reduce sensitivity

will apply

thanks

[...]
diff mbox

Patch

From 4940963648cb431f1a8e827adefb58d318186b38 Mon Sep 17 00:00:00 2001
From: "Avi Halachmi (:avih)" <avihpit@yahoo.com>
Date: Tue, 28 Aug 2018 17:14:55 +0300
Subject: [PATCH] configure: <fflib>_deps: validate, reduce sensitivity

- Allow to add deps in any order rather than "in linking order".
- Expand deps chains as required rather than just once.
- Validate that there are no cycles.
- Validate that [after expansion] deps are limited to other fflibs.
- Remove expectation for a specific output order of unique().

Previously when adding items to <fflib>_deps, developers were
required to add them in linking order. This can be awkward and
bug-prone, especially when a list is not empty, e.g. when adding
conditional deps.

It also implicitly expected unique() to keep the last instance of
recurring items such that these lists maintain their linking order
after removing duplicate items.

This patch mainly allows to add deps in any order by keeping just
one master list in linking order, and then reordering all the
<fflib>_deps lists to align with the master list order.

This master list is LIBRARY_LIST itself, where otherwise its order
doesn't matter.

The patch also removes a limit where these deps lists were expanded
only once. This could have resulted in incomplete expanded lists,
or forcing devs to add already-deducable deps to avoid this issue.

Note: it is possible to deduce the master list order automatically
from the deps lists, but in this case it's probably not worth the
added complexity, even if minor. Maintaining one list should be OK.
---
 configure | 46 ++++++++++++++++++++++++++++++++++++----------
 1 file changed, 36 insertions(+), 10 deletions(-)

diff --git a/configure b/configure
index 3cf94129..8bbcd530 100755
--- a/configure
+++ b/configure
@@ -1818,16 +1818,17 @@  FEATURE_LIST="
     swscale_alpha
 "
 
+# this list should be kept in linking order
 LIBRARY_LIST="
-    avcodec
     avdevice
     avfilter
+    swscale
+    postproc
     avformat
+    avcodec
+    swresample
     avresample
     avutil
-    postproc
-    swresample
-    swscale
 "
 
 LICENSE_LIST="
@@ -3491,7 +3492,7 @@  vaapi_transcode_example_deps="avcodec avformat avutil h264_vaapi_encoder"
 cpu_init_extralibs="pthreads_extralibs"
 cws2fws_extralibs="zlib_extralibs"
 
-# libraries, in linking order
+# libraries, in any order
 avcodec_deps="avutil"
 avcodec_suggest="libm"
 avcodec_select="null_bsf"
@@ -6846,7 +6847,7 @@  done
 
 enabled zlib && add_cppflags -DZLIB_CONST
 
-# conditional library dependencies, in linking order
+# conditional library dependencies, in any order
 enabled afftfilt_filter     && prepend avfilter_deps "avcodec"
 enabled afir_filter         && prepend avfilter_deps "avcodec"
 enabled amovie_filter       && prepend avfilter_deps "avformat avcodec"
@@ -6889,11 +6890,36 @@  enabled sdl2_outdev     && add_cflags $(filter_out '-Dmain=SDL_main' $sdl2_cflag
 
 enabled opus_decoder    && prepend avcodec_deps "swresample"
 
+# reorder the items at var $1 to align with the items order at var $2 .
+# die if an item at $1 is not at $2 .
+reorder_by(){
+    eval rb_in=\$$1
+    eval rb_ordered=\$$2
+
+    for rb in $rb_in; do
+        is_in $rb $rb_ordered || die "$rb at \$$1 is not at \$$2"
+    done
+
+    rb_out=
+    for rb in $rb_ordered; do
+        is_in $rb $rb_in && rb_out="$rb_out$rb "
+    done
+    eval $1=\$rb_out
+}
+
+# deps-expand fflib $1:  N x {append all expanded deps; unique}
+# within a set of N items, N expansions are enough to expose a cycle.
 expand_deps(){
-    lib_deps=${1}_deps
-    eval "deps=\$$lib_deps"
-    append $lib_deps $(map 'eval echo \$${v}_deps' $deps)
-    unique $lib_deps
+    unique ${1}_deps  # required for the early break test.
+    for dummy in $LIBRARY_LIST; do  # N iteratios
+        eval deps=\$${1}_deps
+        append ${1}_deps $(map 'eval echo \$${v}_deps' $deps)
+        unique ${1}_deps
+        eval '[ ${#deps} = ${#'${1}_deps'} ]' && break  # doesn't expand anymore
+    done
+
+    eval is_in $1 \$${1}_deps && die "Dependency cycle at ${1}_deps"
+    reorder_by ${1}_deps LIBRARY_LIST  # linking order is expected later
 }
 
 #we have to remove gpl from the deps here as some code assumes all lib deps are libs
-- 
2.17.1