pgsql: Skip \password TAP test on old IPC::Run versions

On 2023-04-08 Sa 09:57, Daniel Gustafsson wrote:

Skip \password TAP test on old IPC::Run versions

IPC::Run versions prior to 0.98 cause the interactive session to time out,
so SKIP the test in case these versions are detected (they are within the
base requirement for our TAP tests in general). Error reported by the BF
and investigation by Tom Lane.

Discussion:/messages/by-id/414A86BD-986B-48A7-A1E4-EEBCE5AF08CB@yesql.se

Stylistic nitpick: It's not necessary to have 2 evals here:

+   skip "IO::Pty and IPC::Run >= 0.98 required", 1 unless
+       (eval { require IO::Pty; } && eval { $IPC::Run::VERSION >= 
'0.98' });

just say "eval { require IO::Pty; return $IPC::Run::VERSION >= '0.98'; }"

cheers

andrew

--
Andrew Dunstan
EDB:https://www.enterprisedb.com

Andrew Dunstan <andrew@dunslane.net> writes:

Stylistic nitpick: It's not necessary to have 2 evals here:

+   skip "IO::Pty and IPC::Run >= 0.98 required", 1 unless
+       (eval { require IO::Pty; } && eval { $IPC::Run::VERSION >= 
'0.98' });

just say "eval { require IO::Pty; return $IPC::Run::VERSION >= '0.98'; }"

What I was wondering about was if it's safe to depend on the VERSION
being fully numeric. Can't we write "use IPC::Run 0.98;" and let
some other code manage the version comparison?

regards, tom lane

On 8 Apr 2023, at 18:23, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Andrew Dunstan <andrew@dunslane.net> writes:

Stylistic nitpick: It's not necessary to have 2 evals here:

+   skip "IO::Pty and IPC::Run >= 0.98 required", 1 unless
+       (eval { require IO::Pty; } && eval { $IPC::Run::VERSION >= 
'0.98' });

just say "eval { require IO::Pty; return $IPC::Run::VERSION >= '0.98'; }"

What I was wondering about was if it's safe to depend on the VERSION
being fully numeric.

Reading documentation online pointed at this being the established way, and
trying to read the Perl5 code it seems to essentially turn whatever is set in
$VERSION into a numeric.

Can't we write "use IPC::Run 0.98;" and let
some other code manage the version comparison?

We can, but that AFAIK (Andrew might have a better answer) requires the below
diff which I think leaves some readability to be desired:

-   (eval { require IO::Pty; } && eval { $IPC::Run::VERSION >= '0.98' });
+   (eval { require IO::Pty; } && !!eval { IPC::Run->VERSION('0.98'); 1 });

This is needed since ->VERSION die()'s if the version isn't satisfied. That
seems to work fine though, and will ensure that the UNIVERSAL version method
does the version comparison. We can of course expand the comment on why that
construct is needed.

--
Daniel Gustafsson

Daniel Gustafsson <daniel@yesql.se> writes:

On 8 Apr 2023, at 18:23, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Can't we write "use IPC::Run 0.98;" and let
some other code manage the version comparison?

We can, but that AFAIK (Andrew might have a better answer) requires the below
diff which I think leaves some readability to be desired:

-   (eval { require IO::Pty; } && eval { $IPC::Run::VERSION >= '0.98' });
+   (eval { require IO::Pty; } && !!eval { IPC::Run->VERSION('0.98'); 1 });

Maybe I'm missing something, but I was envisioning

eval { require IO::Pty; use IPC::Run 0.98; }

with no need to do more than check if the eval traps an error.

regards, tom lane

On 2023-04-08 Sa 16:20, Daniel Gustafsson wrote:

On 8 Apr 2023, at 18:23, Tom Lane<tgl@sss.pgh.pa.us> wrote:

Andrew Dunstan<andrew@dunslane.net> writes:

Stylistic nitpick: It's not necessary to have 2 evals here:
+   skip "IO::Pty and IPC::Run >= 0.98 required", 1 unless
+       (eval { require IO::Pty; } && eval { $IPC::Run::VERSION >=
'0.98' });
just say "eval { require IO::Pty; return $IPC::Run::VERSION >= '0.98'; }"

What I was wondering about was if it's safe to depend on the VERSION
being fully numeric.

Reading documentation online pointed at this being the established way, and
trying to read the Perl5 code it seems to essentially turn whatever is set in
$VERSION into a numeric.

Can't we write "use IPC::Run 0.98;" and let
some other code manage the version comparison?

We can, but that AFAIK (Andrew might have a better answer) requires the below
diff which I think leaves some readability to be desired:

-   (eval { require IO::Pty; } && eval { $IPC::Run::VERSION >= '0.98' });
+   (eval { require IO::Pty; } && !!eval { IPC::Run->VERSION('0.98'); 1 });

This is needed since ->VERSION die()'s if the version isn't satisfied. That
seems to work fine though, and will ensure that the UNIVERSAL version method
does the version comparison. We can of course expand the comment on why that
construct is needed.

I still don't understand why you're using two evals here. This should be
sufficient and seems simpler to me:

  unless eval { require IO:Pty; IPC::Run->VERSION('0.98'); }

cheers

andrew

--
Andrew Dunstan
EDB:https://www.enterprisedb.com

On 2023-04-08 Sa 16:30, Tom Lane wrote:

Daniel Gustafsson<daniel@yesql.se> writes:

On 8 Apr 2023, at 18:23, Tom Lane<tgl@sss.pgh.pa.us> wrote:

Can't we write "use IPC::Run 0.98;" and let
some other code manage the version comparison?

We can, but that AFAIK (Andrew might have a better answer) requires the below
diff which I think leaves some readability to be desired:
-   (eval { require IO::Pty; } && eval { $IPC::Run::VERSION >= '0.98' });
+   (eval { require IO::Pty; } && !!eval { IPC::Run->VERSION('0.98'); 1 });

Maybe I'm missing something, but I was envisioning

eval { require IO::Pty; use IPC::Run 0.98; }

with no need to do more than check if the eval traps an error.

You need to be careful with "use". It is executed in the compile phase,
so I'd avoid it here.

cheers

andrew

--
Andrew Dunstan
EDB:https://www.enterprisedb.com

On 8 Apr 2023, at 22:37, Andrew Dunstan <andrew@dunslane.net> wrote:
On 2023-04-08 Sa 16:20, Daniel Gustafsson wrote:

On 8 Apr 2023, at 18:23, Tom Lane <tgl@sss.pgh.pa.us>
wrote:

Andrew Dunstan
<andrew@dunslane.net>
writes:

Stylistic nitpick: It's not necessary to have 2 evals here:

+   skip "IO::Pty and IPC::Run >= 0.98 required", 1 unless
+       (eval { require IO::Pty; } && eval { $IPC::Run::VERSION >= 
'0.98' });

just say "eval { require IO::Pty; return $IPC::Run::VERSION >= '0.98'; }"

What I was wondering about was if it's safe to depend on the VERSION
being fully numeric.

Reading documentation online pointed at this being the established way, and
trying to read the Perl5 code it seems to essentially turn whatever is set in
$VERSION into a numeric.

Can't we write "use IPC::Run 0.98;" and let
some other code manage the version comparison?

We can, but that AFAIK (Andrew might have a better answer) requires the below
diff which I think leaves some readability to be desired:

-   (eval { require IO::Pty; } && eval { $IPC::Run::VERSION >= '0.98' });
+   (eval { require IO::Pty; } && !!eval { IPC::Run->VERSION('0.98'); 1 });

This is needed since ->VERSION die()'s if the version isn't satisfied. That
seems to work fine though, and will ensure that the UNIVERSAL version method
does the version comparison. We can of course expand the comment on why that
construct is needed.

I still don't understand why you're using two evals here. This should be sufficient and seems simpler to me:

unless eval { require IO:Pty; IPC::Run->VERSION('0.98'); }

Because I was trying to get "use IPC::Run" to work and that required multiple
evals, and got stuck on that track. I agree that your version is better and
does indeed work (and offloads version comparison to the UNIVERSAL class), so
I'll go ahead with the below.

-   (eval { require IO::Pty; } && eval { $IPC::Run::VERSION >= '0.98' });
+   eval { require IO::Pty; IPC::Run->VERSION('0.98'); };

--
Daniel Gustafsson