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

[Peter Maydell]

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

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