diff mbox series

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

Message ID 20200910131504.11341-7-alex.bennee@linaro.org
State Accepted
Commit 2d838d9bae05b76af574330cf6707527568abe03
Headers show
Series testing and other mix fixes | expand

Commit Message

Alex Bennée Sept. 10, 2020, 1:15 p.m. UTC 
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>

-- 
2.20.1

Comments

Peter Maydell Sept. 10, 2020, 8:33 p.m. UTC | #1 
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
Alex Bennée Sept. 11, 2020, 8:05 a.m. UTC | #2 
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
diff mbox series

Patch

diff --git a/configure b/configure
index 4231d56bcc0..2b5492a0d63 100755
--- a/configure
+++ b/configure
@@ -542,6 +542,8 @@  gettext=""
 bogus_os="no"
 malloc_trim=""
 
+deprecated_features=""
+
 # parse CC options first
 for opt do
   optarg=$(expr "x$opt" : 'x[^=]*=\(.*\)')
@@ -1720,26 +1722,25 @@  if [ "$bsd_user" = "yes" ]; then
     mak_wilds="${mak_wilds} $source_path/default-configs/*-bsd-user.mak"
 fi
 
-if test -z "$target_list_exclude"; then
-    for config in $mak_wilds; do
-        default_target_list="${default_target_list} $(basename "$config" .mak)"
-    done
-else
-    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
-        if test "$exclude" = "no"; then
-            default_target_list="${default_target_list} $target"
+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
+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
   ;;
@@ -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}"
   ;;
   riscv32)
     TARGET_BASE_ARCH=riscv
@@ -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