CT-NG 1.11.1: Using Spaces in *_EXTRA_CONFIG

[Benoît THÉBAUDEAU]

Hi Yann E.,

> [Note: Please, wrap long line to <80 columns.]

OK.

> Yes, I should have used past in these sentences. We indeed have to
> support
> both. But at the time I introduced this options (on the very
> beginnings),
> I did not really have time to do both, and I was not aware of any
> real-life
> use-case for spaces in arguments.

OK.

> Now, it appears there *are* valid real-life use-cases, and that we
> must
> support them. We just have to find a convenient way to do:
> - convenient for the end user
> - conveient for the maintenance

Indeed.

> Indeed. That's inherent to how the shell is expected to behave.

Yes.

> I've done a few experiments here, and in the end it turns out to be
> not
> so complex, after all.

OK.

> I really don't like this patch. It can become a nightmare to
> maintain, and
> I am not even sure it is 100% correct (but I did not test it at all
> so far,
> only read parts of it; I will look more thoroughly tomorrow.).

It was only a quick solution to fix the issue. I have not tested all
possible cases, but many on a full toolchain build, without any problem.

> Using eval means we have to be very carefull on double quoting and/or
> escaping, both of which I really don't like. See my comments below.
>
> > ---
> > diff -Nrdup
> > crosstool-ng-1.11.1.orig/scripts/build/binutils/binutils.sh
> > crosstool-ng-1.11.1/scripts/build/binutils/binutils.sh
> > --- crosstool-ng-1.11.1.orig/scripts/build/binutils/binutils.sh
> > 	2011-05-02 18:11:24.000000000 +0200
> > +++ crosstool-ng-1.11.1/scripts/build/binutils/binutils.sh
> > 	2011-05-13 16:25:27.944984290 +0200
> > @@ -58,19 +58,20 @@ do_binutils() {
> >
> >      CT_DoLog DEBUG "Extra config passed: '${extra_config[*]}'"
> >
> > -    CT_DoExecLog CFG                                            \
> > -    CFLAGS="${CT_CFLAGS_FOR_HOST}"                              \
> > -    "${CT_SRC_DIR}/binutils-${CT_BINUTILS_VERSION}/configure"   \
> > -        --build=${CT_BUILD}                                     \
> > -        --host=${CT_HOST}                                       \
> > -        --target=${CT_TARGET}                                   \
> > -        --prefix=${CT_PREFIX_DIR}                               \
> > -        --disable-nls                                           \
> > -        --disable-multilib                                      \
> > -        --disable-werror                                        \
> > -        "${extra_config[@]}"                                    \
> > -        ${CT_ARCH_WITH_FLOAT}                                   \
> > -        ${CT_BINUTILS_EXTRA_CONFIG}                             \
> > +    CT_DoExecLog CFG
> >                                              \
> > +    CFLAGS="${CT_CFLAGS_FOR_HOST}"
> >                                \
> > +    eval
> >                                                          \
> > +    "\"${CT_SRC_DIR}/binutils-${CT_BINUTILS_VERSION}/configure\""
> > \
> > +        --build=${CT_BUILD}
> >                                       \
> > +        --host=${CT_HOST}
> >                                         \
> > +        --target=${CT_TARGET}
> >                                     \
> > +        --prefix=${CT_PREFIX_DIR}
> >                                 \
> > +        --disable-nls
> >                                             \
> > +        --disable-multilib
> >                                        \
> > +        --disable-werror
> >                                          \
> > +        ${extra_config[@]}
> >                                        \
>
> I have to check, but what if any of the extra_config item has spaces
> in
> them? Double quotes? Single quotes? A $ sign? A combination of these?

Basically, an extra quoting level has to be added (i.e. escaped double
quotes have to be added inside the existing double quotes) when using
eval where spaces or special characters might occur. It's as simple as
that. That's all I did in this patch.

For the case of variable arrays the extra quoting level has to be added
for each element of the array, which can as well be automated by a
dedicated function in order not to have to think about eval when assigning
extra_config.

> Really, I am changing my mind, and I think my second suggestion would
> be
> the best (instead if the first one).
>
> Basically, the plan is:
> - in the config.in files, suffix all variables than can be an array
> with
>   _ARRAY, such as: CT_BINUTILS_EXTRA_CONFIG_ARRAY,
> - before using .config (easy to do a Makefiel rule!), sed it into
> .config.2
>   so as to transform assignments to these variables into real array
>   assignments. I have some sed magic here that does the trick very
>   nicely,
> - build scripts that can use an array should handle it natively, with
> the
>   standard bash syntax, eg: "${CT_BINUTILS_EXTRA_CONFIG_ARRAY[@]}",
> - from crosstool-ng.sh, use .config.2 instead of .config, et voilà!
>
> This has a huge advantage in my opinion:
> - it works for many arguments, a single element, and even none at
> all,
> - easy assignement in the menuconfig for end-users,
> - clean and lean code, easy maintenance,
> - .config is still includable from the ct-ng frontend.
>
> The drawback is we can't include .config.2 from the ct-ng frontend,
> as that
> is a Makefile script, and I have to check if we need to handle arrays
> inside
> the frontend (not that I can think of, so this is a minor
> limitation).

As you prefer.

My patch seems to work and it does not seem that difficult to maintain
if a function is added to automatically add a quoting level to extra_config
elements before calling CT_DoExecLog() instead of doing it manually like in
this patch.

Your 2nd suggestion would work too and it may be prettier, but it requires
more work to be implemented, and it has an impact on the infrastructure of
CT-NG.

Choose the solution you think is the more appropriate for you, provided this
issue is addressed.

> Anyway, thanks Benoît for your suggestion! ;-)

You're welcome.

Best regards,
Benoît

--
For unsubscribe information see http://sourceware.org/lists.html#faq