Re: [ping] Re: [patch v2] support for multiarch systems

[Ian Lance Taylor <iant at google dot com>]

On Fri, Nov 2, 2012 at 6:26 AM, Matthias Klose <doko@ubuntu.com> wrote:
>
>> +ifeq ($(enable_multiarch),yes)
>> +  if_multiarch = $(1)
>> +else ifeq ($(enable_multiarch),auto-detect)
>> +  if_multiarch = $(if $(wildcard $(shell echo
>> $(SYSTEM_HEADER_DIR))/../../usr/lib/*/crti.o),$(1))
>> +else
>> +  if_multiarch =
>> +endif
>>
>> This is nicely tricky but I don't understand why you are doing this in
>> such a confusing way.  Why not simply set the names in config.gcc?
>> What information do you have at make time that you don't have at
>> configure time?
>
>
> this is for the auto case. SYSTEM_HEADER_DIR is not accessible/available
> from
> config.gcc. This is the same approach as taken for MULTILIB_OSDDIRNAMES.
> Also setting the names for the multiarch directories in the same place as
> done for for the MULTILIB_OSDIRNAMES seems to be better choice.

SYSTEM_HEADER_DIR is available in gcc/configure.ac, and you could do
the $(if $(wildcard $(shell echo
>> $(SYSTEM_HEADER_DIR))/../../usr/lib/*/crti.o) there.  I can't see any reason to do it at make time rather than configure time.

> +	      if (*q2 == ':')
> +		end = q2;

Don't reuse end here.  Use a new variable.

Please update the comment for set_multilib_dir.  It's not a very good
comment, but it does describe the syntax, and you seem to be changing
the syntax.

> -      if (this_path[0] == '.' && this_path[1] == ':')
> +      if (this_path[0] == '.' && this_path[1] == ':' && this_path[2] != ':')

The existing comment here is reasonably clear.  Please update the
comment to show why this change is correct.

> +Specify wether to enable or disable multiarch support.  The default is

s/wether/whether/

> +to detect for glibc start files in a multiarch location, and enable it

s/detect/check/ or something along those lines.

Ian