Re: [PATCH v3 0/5] meson: wire up support for benchmarks

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 24, 2025, 11:13 UTC
Message-ID: <xmqqwmb9vmlw.fsf@gitster.g>
In-Reply-To: <aAm-pcIfrFt6mjlV@pks.im>

Patrick Steinhardt <ps@pks.im> writes:

Show 18 quoted lines

>> I was more expecting something simple like:
>> 
>> if time.found()
>>   ...
>> else
>>   error('Benchmarking requires the `time` command')
>> endif
>> 
>> in the same way as in meson.build elsewhere we have things like:
>> 
>> if not msgfmt.found() and gettext_option.enabled()
>>   error('Internationalization via libintl requires msgfmt')
>> endif
>
> But erroring out by default doesn't really feel nice to the general
> developer. I'd claim that 99% of the time, developers will only end up
> running the test suite, never the benchmarks. So the default should
> match that and not require GNU time to be available, if you ask me.

Maybe a stupid question, but does "time" a shell built-in count when "if time.found()" is evaluated?

    $ type --all time
    time is a shell keyword
    time is /usr/bin/time
    time is /bin/time

Show 9 quoted lines

> An alternative could be to have a 'benchmarks' feature option wired up.
> This feature would be set to 'auto', and then we can detect time like
> this:
>
>     time = find_program('time', dirs: program_path, required: get_option('benchmarks'))
>
> This will then auto-enable or -disable the feature depending on the
> availability of GNU time. But if the user passes `-Dbenchmarks=enabled`,
> we would fail the setup in case the program wasn't found.

Sounds quite sensible. Do what most users expect by default without failing, let users give explicit preference, and when the explicitly expressed preference cannot be fulfilled, loudly error out. I like it.

Thanks.