[vlc-devel] [PATCH 1/2] package: macosx: forward ac_cv symbols to $@

[David Fuhrmann]

> Am 21.02.2021 um 14:34 schrieb Alexandre Janniaux <ajanni at videolabs.io>:
> 
> Hi,
> 
> On Sun, Feb 21, 2021 at 01:41:10PM +0100, David Fuhrmann wrote:
>> 
>> 
>>> Am 21.02.2021 um 13:22 schrieb Alexandre Janniaux <ajanni at videolabs.io>:
>>> 
>>> Hi,
>>> 
>>> On Sun, Feb 21, 2021 at 07:58:59AM +0100, david.fuhrmann at gmail.com <mailto:david.fuhrmann at gmail.com> wrote:
>>>> 
>>>> 
>>>>> Am 20.02.2021 um 10:21 schrieb Alexandre Janniaux <ajanni at videolabs.io>:
>>>>> 
>>>>> configure will store the command used when reconfiguring as long as the
>>>>> ac_cv_ variables are passed as parameter to the configure execution
>>>>> instead of being an environment variable.
>>>>> 
>>>>> So as to stay compatible with other clients of this script, use export
>>>>> as default command when no argument is given to keep the previous
>>>>> behaviour.
>>>>> ---
>>>>> extras/package/macosx/env.build.sh | 54 +++++++++++++++++-------------
>>>>> 1 file changed, 30 insertions(+), 24 deletions(-)
>>>>> 
>>>>> diff --git a/extras/package/macosx/env.build.sh b/extras/package/macosx/env.build.sh
>>>>> index ef1316465a..fd2f219a29 100755
>>>>> --- a/extras/package/macosx/env.build.sh
>>>>> +++ b/extras/package/macosx/env.build.sh
>>>>> @@ -77,36 +77,42 @@ vlcSetBaseEnvironment() {
>>>>> }
>>>>> 
>>>>> vlcSetSymbolEnvironment() {
>>>>> +    ARGS=( "$@" )
>>>>> +    if [ "$#" -lt 2 ]; then
>>>>> +        ARGS+=( export )
>>>>> +    fi
>>>> 
>>>> Hi, I do not really understand this logic here: why do you care about the arguments of the overall bash script when you are constructing the export line below?
>>>> 
>>>> I think you should rather use a local parameter for this function to decide whether to use export or not. Or just return the list below here, and take care about export or no export in a different place...
>>>> 
>>> 
>>> This patch is wrong as-is (ARGS not used, -lt 2 should be -lt 1),
>>> I'll send a fixed version later with more comments.
>>> 
>>> The logic behind ARGS is just to keep the previous behaviour right,
>>> it can be removed later once every callsite would have been fixed.
>>> This is because I didn't test on macosx, and to be fair I tested on
>>> iOS but probably completely messed up the actual testing since it
>>> wouldn't even work, I'm not sure how it resulted in a correct build
>>> back then.
>>> 
>>> The core idea of this change is to not use export anymore, and be
>>> sure the configuration we are using is kept for later build, mostly
>>> because I usually use make to build and it breaks when there is a
>>> reconf and some files depending on the function where the detection
>>> is overriden here is recompiled. As a side-effect, since I've
>>> integrated the current buildsystem in some XCode project generation
>>> tool, which will be using make, it prevents compilation from this
>>> XCode project to fail randomly when it triggers a reconfiguration.
>> 
>> Yes, I understood the general purpose of this patch, and if this works it would be very cool, indeed. :-)
>> I just did not understand at all, why you need to use $@ and $# in this context.
> 
> With bash functions, $@ expands to all arguments of the function (and
> not the script) and $# expands to the argument count for the function
> (and thus not the script). The confusion might come first from here.
> 
> Then, the pattern is that using a variable for arguments is confusing
> and not that portable, but function can provide this indirection in a
> more comfortable way, though it needs a bit more understanding.
> 
> So it basically transforms
> 
> ```
> vlcSetSymbolEnvironment foo bar
> ```
> 
> Into
> 
> ```
> foo bar ac_cv_foo_bar=no ac_cv_baz=no....
> ```
> 
> Which is the reason I use $@.
> 
> Then, it's possible to do
> 
> ```
> vlcSetSymbolEnvironment export
> ```
> 
> Which will correctly expand to
> 
> ```
> export ac_cv_foo_bar=no ac_cv_baz=no
> ```
> 
> Since the previous usage of the function was just to call
> 
> ```
> vlcSetSymbolEnvironment
> ```
> 
> and expect it to export the environment, I use $# to know whether
> arguments has been given to the function, and default to export
> otherwise.
> 
> It can be written in a leaner form as the following, though it needs
> understanding of default values in bash.
> 
> ```
> vlcSetSymbolEnvironment() {
>    local cmd=${1:-export}; shift
>    "${cmd}" "$@" \
>        ac_cv_foo=no \
>        ....
> }
> ```

Ok, thanks for the explanation.