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