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

[Alexandre Janniaux]

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 \
        ....
}
```

I'll make sure it's clearer in the next version

Regards,
--
Alexandre Janniaux
Videolabs