Re: [libav-devel] [PATCH 1/2 v2] configure: add test_pkg_config()

2017-10-03 Thread James Almer 
On 10/3/2017 9:49 PM, Diego Biurrun wrote:
> On Fri, Sep 29, 2017 at 10:56:08PM -0300, James Almer wrote:
>> --- a/configure
>> +++ b/configure
>> @@ -1039,8 +1039,15 @@ check_pkg_config(){
>>  pkg_libs=$($pkg_config --libs $pkg_config_flags $pkg)
>>  check_func_headers "$headers" "$funcs" $pkg_cflags $pkg_libs "$@" &&
>>  enable $name &&
>> -add_cflags"$pkg_cflags" &&
>> -eval $(sanitize_var_name ${name}_extralibs)="\$pkg_libs"
>> +set_sanitized "${name}_cflags"$pkg_cflags &&
>> +set_sanitized "${name}_extralibs" $pkg_libs
>> +}
>> +
>> +check_pkg_config(){
>> +log check_pkg_config "$@"
>> +pkg="${2%% *}"
>> +test_pkg_config "$@" || return
>> +add_cflags $(get_sanitized "${name}_cflags")
>>  }
> 
> You're setting pkg, but using name. Of course that works because all
> variables are global in this shell script, but it's not exactly pretty
> nonetheless.

Yes, it was a mistake i didn't notice since name is effectively set
globally as you said.

> I'll try to make up my mind which way I want to handle
> the situation.

Want me to send a new version replacing pkg="${2%% *}" with name="$1" as
i suggested in a previous reply?
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel

Re: [libav-devel] [PATCH 1/2 v2] configure: add test_pkg_config()

2017-10-03 Thread Diego Biurrun 
On Fri, Sep 29, 2017 at 10:56:08PM -0300, James Almer wrote:
> --- a/configure
> +++ b/configure
> @@ -1039,8 +1039,15 @@ check_pkg_config(){
>  pkg_libs=$($pkg_config --libs $pkg_config_flags $pkg)
>  check_func_headers "$headers" "$funcs" $pkg_cflags $pkg_libs "$@" &&
>  enable $name &&
> -add_cflags"$pkg_cflags" &&
> -eval $(sanitize_var_name ${name}_extralibs)="\$pkg_libs"
> +set_sanitized "${name}_cflags"$pkg_cflags &&
> +set_sanitized "${name}_extralibs" $pkg_libs
> +}
> +
> +check_pkg_config(){
> +log check_pkg_config "$@"
> +pkg="${2%% *}"
> +test_pkg_config "$@" || return
> +add_cflags $(get_sanitized "${name}_cflags")
>  }

You're setting pkg, but using name. Of course that works because all
variables are global in this shell script, but it's not exactly pretty
nonetheless. I'll try to make up my mind which way I want to handle
the situation.

Diego
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel

Re: [libav-devel] [PATCH 1/2 v2] configure: add test_pkg_config()

2017-10-03 Thread Diego Biurrun 
On Mon, Oct 02, 2017 at 11:49:27AM +0200, Diego Biurrun wrote:
> On Fri, Sep 29, 2017 at 10:56:08PM -0300, James Almer wrote:
> > --- a/configure
> > +++ b/configure
> > @@ -1025,8 +1025,8 @@ check_lib(){
> >  
> > -check_pkg_config(){
> > -log check_pkg_config "$@"
> > +test_pkg_config(){
> > +log test_pkg_config "$@"
> >  name="$1"
> >  pkg_version="$2"
> >  pkg="${2%% *}"
> > @@ -1039,8 +1039,15 @@ check_pkg_config(){
> >  pkg_libs=$($pkg_config --libs $pkg_config_flags $pkg)
> >  check_func_headers "$headers" "$funcs" $pkg_cflags $pkg_libs "$@" &&
> >  enable $name &&
> > -add_cflags"$pkg_cflags" &&
> > -eval $(sanitize_var_name ${name}_extralibs)="\$pkg_libs"
> > +set_sanitized "${name}_cflags"$pkg_cflags &&
> > +set_sanitized "${name}_extralibs" $pkg_libs
> > +}
> > +
> > +check_pkg_config(){
> > +log check_pkg_config "$@"
> > +pkg="${2%% *}"
> > +test_pkg_config "$@" || return
> > +add_cflags $(get_sanitized "${name}_cflags")
> >  }
> 
> At a quick glance, this looks like name and pkg are confused.

No. I should not review patches on the airport after what felt like 6 hours
of sleep in the last three days...

Diego
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel

Re: [libav-devel] [PATCH 1/2 v2] configure: add test_pkg_config()

2017-10-02 Thread James Almer 
On 10/2/2017 6:49 AM, Diego Biurrun wrote:
> On Fri, Sep 29, 2017 at 10:56:08PM -0300, James Almer wrote:
>> --- a/configure
>> +++ b/configure
>> @@ -1025,8 +1025,8 @@ check_lib(){
>>  
>> -check_pkg_config(){
>> -log check_pkg_config "$@"
>> +test_pkg_config(){
>> +log test_pkg_config "$@"
>>  name="$1"
>>  pkg_version="$2"
>>  pkg="${2%% *}"
>> @@ -1039,8 +1039,15 @@ check_pkg_config(){
>>  pkg_libs=$($pkg_config --libs $pkg_config_flags $pkg)
>>  check_func_headers "$headers" "$funcs" $pkg_cflags $pkg_libs "$@" &&
>>  enable $name &&
>> -add_cflags"$pkg_cflags" &&
>> -eval $(sanitize_var_name ${name}_extralibs)="\$pkg_libs"
>> +set_sanitized "${name}_cflags"$pkg_cflags &&
>> +set_sanitized "${name}_extralibs" $pkg_libs
>> +}
>> +
>> +check_pkg_config(){
>> +log check_pkg_config "$@"
>> +pkg="${2%% *}"
>> +test_pkg_config "$@" || return
>> +add_cflags $(get_sanitized "${name}_cflags")
>>  }
> 
> At a quick glance, this looks like name and pkg are confused.

No. I should actually add name="$1" and remove pkg="${2%% *}" here so
it's clear what's using to avoid confusion, even if it's not strictly
needed as $name is already set after calling test_pkg_config().

Look at the code I'm removing and the code I'm adding.

-eval $(sanitize_var_name ${name}_extralibs)="\$pkg_libs"
+set_sanitized "${name}_extralibs" $pkg_libs

These two are virtually the same. I just made it use set_sanitized()
since it's cleaner looking.

test()
+set_sanitized "${name}_cflags"$pkg_cflags &&

check()
-add_cflags"$pkg_cflags"
+add_cflags $(get_sanitized "${name}_cflags")

With this I made sure add_cflags() is still only called in
check_pkg_config() and not in the new test_pkg_config(), which will only
set the package specific cflags and extralibs.
In here $pkg_cflags and ${name}_cflags are always the same, whereas that
can't be said for ${pkg}_cflags (example, all four vpx codecs have pkg
as vpx).

name is what was being used before and what is still being used after
this patch.
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel

Re: [libav-devel] [PATCH 1/2 v2] configure: add test_pkg_config()

2017-10-02 Thread Diego Biurrun 
On Fri, Sep 29, 2017 at 10:56:08PM -0300, James Almer wrote:
> --- a/configure
> +++ b/configure
> @@ -1025,8 +1025,8 @@ check_lib(){
>  
> -check_pkg_config(){
> -log check_pkg_config "$@"
> +test_pkg_config(){
> +log test_pkg_config "$@"
>  name="$1"
>  pkg_version="$2"
>  pkg="${2%% *}"
> @@ -1039,8 +1039,15 @@ check_pkg_config(){
>  pkg_libs=$($pkg_config --libs $pkg_config_flags $pkg)
>  check_func_headers "$headers" "$funcs" $pkg_cflags $pkg_libs "$@" &&
>  enable $name &&
> -add_cflags"$pkg_cflags" &&
> -eval $(sanitize_var_name ${name}_extralibs)="\$pkg_libs"
> +set_sanitized "${name}_cflags"$pkg_cflags &&
> +set_sanitized "${name}_extralibs" $pkg_libs
> +}
> +
> +check_pkg_config(){
> +log check_pkg_config "$@"
> +pkg="${2%% *}"
> +test_pkg_config "$@" || return
> +add_cflags $(get_sanitized "${name}_cflags")
>  }

At a quick glance, this looks like name and pkg are confused.

Diego
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel

[libav-devel] [PATCH 1/2 v2] configure: add test_pkg_config()

2017-09-29 Thread James Almer 
This helper is split off check_pkg_config(), setting only the pkg cflags
and extralibs. This is useful for checks that don't require setting global
cflags, or don't benefit from it.

Signed-off-by: James Almer 
---
Now setting the correct name for the package's _cflags and _extralibs
variables, and not setting global extralibs.

 configure | 15 +++
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/configure b/configure
index a3cfe3768..cd60ae865 100755
--- a/configure
+++ b/configure
@@ -1025,8 +1025,8 @@ check_lib(){
 enable $name && eval ${name}_extralibs="\$@"
 }
 
-check_pkg_config(){
-log check_pkg_config "$@"
+test_pkg_config(){
+log test_pkg_config "$@"
 name="$1"
 pkg_version="$2"
 pkg="${2%% *}"
@@ -1039,8 +1039,15 @@ check_pkg_config(){
 pkg_libs=$($pkg_config --libs $pkg_config_flags $pkg)
 check_func_headers "$headers" "$funcs" $pkg_cflags $pkg_libs "$@" &&
 enable $name &&
-add_cflags"$pkg_cflags" &&
-eval $(sanitize_var_name ${name}_extralibs)="\$pkg_libs"
+set_sanitized "${name}_cflags"$pkg_cflags &&
+set_sanitized "${name}_extralibs" $pkg_libs
+}
+
+check_pkg_config(){
+log check_pkg_config "$@"
+pkg="${2%% *}"
+test_pkg_config "$@" || return
+add_cflags $(get_sanitized "${name}_cflags")
 }
 
 check_exec(){
-- 
2.14.1

___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel