Re: [PULL 06/10] configure: don't enable ppc64abi32-linux-user by default

[Alex Bennée]

Peter Maydell <peter.maydell@linaro.org> writes:

> On Thu, 10 Sep 2020 at 14:15, Alex Bennée <alex.bennee@linaro.org> wrote:
>>
>> The user can still enable this explicitly but they will get a warning
>> at the end of configure for their troubles. This also drops any builds
>> of ppc64abi32 from our CI tests.
>>
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> Message-Id: <20200909112742.25730-7-alex.bennee@linaro.org>
>
> I know this is in a pullreq at this point, but I just got round
> to looking at it, and it has some odd logic in it so I figured
> I'd give my review comments anyway.
>
>> +if test -z "$target_list_exclude" -a -z "$target_list"; then
>> +    # if the user doesn't specify anything lets skip deprecating stuff
>> +    target_list_exclude=ppc64abi32-linux-user
>
> Doesn't this have the slightly curious logic
> that if we have more than one deprecated target then
> configure with --target-list-exclude=something-else
> will actually build more targets than configure without that
> exclude option (because it will exclude the something-else
> but stop excluding the deprecated targets)? I would have
> expected the deprecated targets to be not compiled unless
> the user explicitly enabled them. I think that would be
> something more like
>
>    deprecated_targets_list=ppc64abi32-linux-user
>    if test -z "$target_list"; then
>        target_list_exclude="$target_list_exclude,$deprecated_targets_list"
>    fi
>
> I suppose we would ideally like an "enable all including
> the deprecated stuff", and that gets messy because there's
> no way to do it except listing everything explicitly I think...

Yeah - we could make it smoother although I think the only real users of
--target-list-exclude are the CI systems and they all explicitly exclude
ppc64abi32 when they do it.

Do you want me to re-spin with those changes?

>
>> +fi
>> +
>> +exclude_list=$(echo "$target_list_exclude" | sed -e 's/,/ /g')
>> +for config in $mak_wilds; do
>> +    target="$(basename "$config" .mak)"
>> +    exclude="no"
>> +    for excl in $exclude_list; do
>> +        if test "$excl" = "$target"; then
>> +            exclude="yes"
>> +            break;
>>          fi
>>      done
>> -fi
>> +    if test "$exclude" = "no"; then
>> +        default_target_list="${default_target_list} $target"
>> +    fi
>> +done
>>
>>  # Enumerate public trace backends for --help output
>>  trace_backend_list=$(echo $(grep -le '^PUBLIC = True$' 
>> "$source_path"/scripts/tracetool/backend/*.py | sed -e 
>> 's/^.*\/\(.*\)\.py$/\1/'))
>> @@ -7557,7 +7558,7 @@ TARGET_SYSTBL=""
>>  case "$target_name" in
>>    i386)
>>      mttcg="yes"
>> -       gdb_xml_files="i386-32bit.xml"
>> +    gdb_xml_files="i386-32bit.xml"
>>      TARGET_SYSTBL_ABI=i386
>>      TARGET_SYSTBL=syscall_32.tbl
>>    ;;
>
> (unrelated change ;-))
>
>> @@ -7667,6 +7668,7 @@ case "$target_name" in
>>      TARGET_SYSTBL_ABI=common,nospu,32
>>      echo "TARGET_ABI32=y" >> $config_target_mak
>>      gdb_xml_files="power64-core.xml power-fpu.xml power-altivec.xml 
>> power-spe.xml power-vsx.xml"
>> +    deprecated_features="ppc64abi32 ${deprecated_features}"
>
> Maybe prefer
>     add_to deprecated_features ppc64abi32
>
> ?
>
>>    ;;
>>    riscv32)
>>      TARGET_BASE_ARCH=riscv
>
> If we just made the deprecation warning be printed by
> generic logic whenever a deprecated target ended up in
> the enabled list then it would be easier to add other deprecated
> targets to the list, as you wouldn't thae also have to find some
> other part of configure like this to set a deprecated_features variable.
>
> (I'll send a patch to add lm32-softmmu,tilegx-linux-user,unicore32-softmmu
> to the deprecated-target list later once we have the mechanism in place.)
>
>> @@ -8011,6 +8013,12 @@ fi
>>  touch ninjatool.stamp
>>  fi
>>
>> +if test -n "${deprecated_features}"; then
>> +    echo "Warning, deprecated features enabled."
>> +    echo "Please see docs/system/deprecated.rst"
>> +    echo "  features: ${deprecated_features}"
>> +fi
>> +
>>  # Save the configure command line for later reuse.
>>  cat <<EOD >config.status
>>  #!/bin/sh
>> --
>
> thanks
> -- PMM


-- 
Alex Bennée