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

[Marvin Scholz]

On 23 Feb 2021, at 22:11, David Fuhrmann wrote:

>> 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.

I misunderstood that too, so some explanations as comments
in the script would be a good idea.

Otherwise looks fine, IMO.

>
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel