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
