Peter Maydell <peter.mayd...@linaro.org> writes:
> On Thu, 10 Sep 2020 at 14:15, Alex Bennée <alex.ben...@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.ben...@linaro.org>
>> Reviewed-by: Philippe Mathieu-Daudé <f4...@amsat.org>
>> Message-Id: <20200909112742.25730-7-alex.ben...@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
