Re: [kvm-unit-tests PATCH v1 2/5] configure: Display the default processor for arm and arm64

[Andrew Jones <andrew.jones@linux.dev>]

On Wed, Jan 15, 2025 at 09:55:14AM +0000, Alexandru Elisei wrote:
> Hi Drew,
> 
> On Tue, Jan 14, 2025 at 07:51:04PM +0100, Andrew Jones wrote:
> > On Tue, Jan 14, 2025 at 05:17:28PM +0000, Alexandru Elisei wrote:
> > ...
> > > > > +# $arch will have changed when cross-compiling.
> > > > > +[ -z "$processor" ] && processor=$(get_default_processor $arch)
> > > > 
> > > > The fact that $arch and $processor are wrong until they've had a chance to
> > > 
> > > $processor is never wrong. $processor is unset until either the user sets it
> > > with --processor, or until this line. This patch introduces $default_processor
> > > only for the purpose of having an accurate help text, it doesn't change when and
> > > how $processor is assigned.
> > 
> > I should have said "The fact that $arch and $default_processor are wrong..."
> > 
> > > 
> > > > be converted might be another reason for the $do_help idea. But it'll
> > > > always be fragile since another change that does some sort of conversion
> > > > could end up getting added after the '[ $do_help ] && usage' someday.
> > > 
> > > configure needs to distinguish between:
> > > 
> > > 1. The user not having specified --processor when doing ./configure.
> > > 2. The user having set --processor.
> > > 
> > > If 1, then kvm-unit-tests can use the default $processor value for $arch,
> > > which could have also been specified by the user.
> > > 
> > > If 2, then kvm-unit-tests should not touch $processor because that's what the
> > > user wants.
> > > 
> > > Do you see something wrong with that reasoning?
> > 
> > If we output $default_processor in usage() before it's had a chance to be
> > set correctly based on a given cross arch, then it won't display the
> > correct name.
> > 
> > > 
> > > Also, I don't understand why you say it's fragile, since configure doesn't
> > 
> > I wrote "it'll always be fragile" where 'it' refers to the most recent
> > object of my paragraph ("the $do_help idea"). But, TBH, I'm not sure
> > how important it is to get the help text accurate, so we can just not
> > care if we call usage() with the wrong strings sometimes.
> 
> Got it now, thanks for explaining it.
> 
> My opinion is that a help text is there to help the user, and in my experience
> an inaccurate help text can be very frustrating - think comments that say
> one thing, and the code does something else.
> 
> How about this:
> 
> diff --git a/configure b/configure
> index 3ab0ec208e10..5dbe189816b2 100755
> --- a/configure
> +++ b/configure
> @@ -51,7 +51,6 @@ page_size=
>  earlycon=
>  efi=
>  efi_direct=
> -default_processor=$(get_default_processor $arch)
> 
>  # Enable -Werror by default for git repositories only (i.e. developer builds)
>  if [ -e "$srcdir"/.git ]; then
> @@ -61,13 +60,14 @@ else
>  fi
> 
>  usage() {
> +    [ -z "$processor" ] && processor=$(get_default_processor $arch)
>      cat <<-EOF
>         Usage: $0 [options]
> 
>         Options include:
>             --arch=ARCH            architecture to compile for ($arch). ARCH can be one of:
>                                    arm, arm64/aarch64, i386, ppc64, riscv32, riscv64, s390x, x86_64
> -           --processor=PROCESSOR  processor to compile for ($default_processor). For arm and arm64, the
> +           --processor=PROCESSOR  processor to compile for ($processor). For arm and arm64, the
>                                    value 'max' is special and it will be passed directly to
>                                    qemu, bypassing the compiler. In this case, --cflags can be
>                                    used to compile for a specific processor.
> 
> Should be accurate enough, as far as I can tell. And I don't think there's
> a need for $do_help: if the user does ./configure --help --arch=arm64, then
> I think it's reasonable to expect that --help will be interpreted before
> --arch is parsed.
>

Sounds good.

Thanks,
drew