Re: Rewriting the test of pg_upgrade as a TAP test - take two

On Mon, Apr 02, 2018 at 10:35:09AM -0400, Peter Eisentraut wrote:
> I took a quick look at that part. It appears to be quite invasive, more
> than I would have hoped. Basically, it imposes that from now on all
> program invocations must observe the bindir setting, which someone is
> surely going to forget. The pg_upgrade tests aren't going to exercise
> all possible paths where programs are called, so this is going to lead
> to omissions and inconsistencies -- which will then possibly only be
> found much later by the extensions that Craig was talking about. I'd
> like to see this more isolated, maybe via a path change, or something
> inside system_or_bail or something less invasive and more maintainable.

Hm. PostgresNode.pm uses now a couple of different ways to trigger
commands, which makes the problem harder than it seems:
- system_or_bail
- system_log
- IPC::Run::timeout
- IPC::Run::run

Because of that, and if one wants to minimize the number of places where
bindir is called, would be to modify the command strings themselves.
What I could think of would be to have a wrapper like build_bin_path
which adds $bindir on top of the binary, but that would still cause all
the callers to use this wrapper. The use of this wrapper could still be
forgotten.

Enforcing PATH temporarily in a code path or another implies that the
previous value of PATH needs to be added back, which could be forgotten
as well.

At the end, it seems to me that what I am proposing is themost readable
approach, and with proper documentation things should be handled
finely... Or there is an approach you have in mind I do not foresee?
--
Michael