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

From: Patrick Steinhardt <ps@pks.im>
Date: Apr 24, 2025, 13:49 UTC
Message-ID: <aApBbArLEq9dSIYr@pks.im>
In-Reply-To: <xmqqwmb9vmlw.fsf@gitster.g>

On Thu, Apr 24, 2025 at 04:13:15AM -0700, Junio C Hamano wrote:

Show 28 quoted lines

> Patrick Steinhardt <ps@pks.im> writes:
> 
> >> 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

No, it doesn't, and that is intentional. Our benchmarks explicitly require GNU time because it surfaces more information than the shell builtin.

We even have a `USR_BIN_TIME` prereq in our test lib, which is unfortunately way too limiting for some platforms like NixOS. Maybe I should use that as an excuse to also refactor this prereq while at it to instead use a path-based lookup of time. On the other hand it's only used by a single test anyway, so it doesn't really feel worth it.

Patrick