On Fri, Nov 04, 2016 at 12:43:51AM +0100, Janne Grunau wrote:
> On 2016-10-25 23:30:50 +0200, Diego Biurrun wrote:
> > On Fri, Sep 09, 2016 at 08:08:55PM +0200, Janne Grunau wrote:
> > > On 2016-09-05 20:31:39 +0200, Diego Biurrun wrote:
> > > > Previously all external library dependencies were added as link-time
> > > > dependencies to all components. This adds bogus dependencies to some
> > > > components, so only add dependencies to components if said components
> > > > actually make use of a dependency.
> > > > ---
> > > >
> > > > This can still be improved a bit I guess. Better function names
> > > > welcome for
> > > > starters. Janne had some more substantive comments. Our pkg-config
> > > > files have
> > > > many more issues, even after this patch...
> > >
> > > I don't think this is the right approach. The fundamental problem in
> > > this patch is that it mixes the checks functions/libraries with the
> > > information which part of libav uses the library. The basic idea would
> > > be to set fn_$(func)_extralibs in check_lib(2) and have components
> > > depend fn_$(func). We can't check for multiple functions in
> > > check_lib(2) after that at once but we don't do it atm. The
> > > dep_extralibs handling in check_deps() needs to be modified (looks like
> > > it is missing in this patch).
> >
> > I'm not sure if we're on the same page here. I attached a very rough sketch
> > of what I interpret to be your idea. It's not exactly turning out simpler,
> > so if I'm going down the wrong path, I'm all ears for better suggestions.
>
> It's more or less what I had in mind. I didn't thought of need to match
> components back to libraries. But I never claimed that it would be
> simpler. It does the right thing wrt to deps. Your previous patch would
> still have linked libavformat against bzlib even then the matroska
> demuxer was disabled (and bzlib is present and not disabled explicitly).
Excellent point; the end result of the current approach is indeed better.
You have convinced me for good that my initial approach was flawed and
your idea is superior.
> > --- a/configure
> > +++ b/configure
> > @@ -1001,10 +1025,11 @@ EOF
> >
> > check_lib(){
> > log check_lib "$@"
> > - headers="$1"
> > - funcs="$2"
> > - shift 2
> > - check_func_headers "$headers" "$funcs" "$@" && add_extralibs "$@"
> > + name="$1"
> > + headers="$2"
> > + func="$3"
> > + shift 3
> > + check_func_headers "$headers" "$func" "$@" && eval
> > ${name}_extralibs="\$@"
>
> I would have used $func as identifier for the extralibs instead of
> introducing an extra identifier.
see below
> > @@ -2352,7 +2378,9 @@ xwma_demuxer_select="riffdec"
> >
> > # indevs / outdevs
> > alsa_indev_deps="alsa_asoundlib_h snd_pcm_htimestamp"
> > +alsa_indev_libs="alsa"
> > alsa_outdev_deps="alsa_asoundlib_h"
> > +alsa_outdev_libs="alsa"
>
> To be honest I find these identifiers confusing. It's probably too short
>
> alsa_outdev_extralibs="alsa_ldflags"
>
> would imho less confusing
You have a point. foo_libs is IMO better than foo_ldflags though.
> > @@ -4756,9 +4785,9 @@ check_header sys/soundcard.h
> > check_header soundcard.h
> >
> > enabled_any alsa_indev alsa_outdev &&
> > - check_lib alsa/asoundlib.h snd_pcm_htimestamp -lasound
> > + check_lib alsa alsa/asoundlib.h snd_pcm_htimestamp -lasound
>
> I'm not really happy about these random additional identifiers. Not sure
> if it is just bikeshedding. Using the function name makes somewhat
> sense. This are the extralibs if you want to use $funtion
I'm not too happy myself but found the additional identifiers more obvious.
If I listed all the functions that we use to check for libraries, I don't
know if you could correctly match them all to their corresponding libraries.
I certainly could not. Besides, the names of these functions change more
often than the library names. Witness OpenSSL where we are now
transitioning from SSL_library_init() to OPENSSL_init_ssl().
Thank you for the insightful - as always - review.
Diego
_______________________________________________
libav-devel mailing list
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel
