Re: install doesn't work on HEAD

2018-06-05 Thread Amit Kapila
On Tue, Jun 5, 2018 at 7:49 PM, Ashutosh Sharma 
wrote:

> Hi,
>
> I am not able to reproduce this issue on HEAD. Seems like the
> following git-commit fixes it,
>
> commit 01deec5f8ae64b5120cc8c93d54fe0e19e477b02
> Author: Andrew Dunstan 
> Date:   Mon May 28 16:44:13 2018 -0400
>
> Return a value from Install.pm's lcopy function
>
>
Yes, you are right, it is fixed after this.  I missed to refresh my
branch.  Thanks for pointing out.

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com


Re: install doesn't work on HEAD

2018-06-05 Thread Ashutosh Sharma
Hi,


On Tue, Jun 5, 2018 at 7:08 PM, Amit Kapila  wrote:
>
> Hi,
>
> On my win-7, I am facing $SUBJECT.  I am consistently getting below error:
> Copying build output files...Could not copy postgres.exe
>  at install.pl line 26.
>
> On digging, I found that it started failing with commit 3a7cc727 (Don't fall 
> off the end of perl functions).  It seems that we have added empty 'return' 
> in all of the functions which seems to break one of the case:
>
> lcopy("$conf\\$pf\\$pf.$ext", "$target\\$dir\\$pf.$ext")
> || croak "Could not copy $pf.$ext\n";
>
> I think the return from lcopy implies 0 (or undef) which cause it to fail.  
> By reading couple of articles on net [1][2] related to this, it seems we 
> can't blindly return in all cases.  On changing, the lcopy as below, install 
> appears to be working.
>
> @@ -40,7 +40,7 @@ sub lcopy
> copy($src, $target)
>   || confess "Could not copy $src to $target\n";
> -   return;
> +   return 1;
>  }
>
> I have randomly check some of the other functions where this patch has added 
> 'return' and those seem to be fine.
>
> Is it by any chance related Perl version or some other settings?  If not, 
> then we should do something for it.
>
> [1] - https://perlmaven.com/how-to-return-undef-from-a-function
> [2] - 
> http://search.cpan.org/dist/Perl-Critic/lib/Perl/Critic/Policy/Subroutines/RequireFinalReturn.pm

I am not able to reproduce this issue on HEAD. Seems like the
following git-commit fixes it,

commit 01deec5f8ae64b5120cc8c93d54fe0e19e477b02
Author: Andrew Dunstan 
Date:   Mon May 28 16:44:13 2018 -0400

Return a value from Install.pm's lcopy function

Commit 3a7cc727c was a little over eager about adding an explicit return
to this function, whose value is checked in most call sites. This change
reverses that and returns the expected value explicitly. It also adds a
check to the one call site lacking one.

-- 
With Regards,
Ashutosh Sharma
EnterpriseDB:http://www.enterprisedb.com

>
>
> --
> With Regards,
> Amit Kapila.
> EnterpriseDB: http://www.enterprisedb.com



install doesn't work on HEAD

2018-06-05 Thread Amit Kapila
Hi,

On my win-7, I am facing $SUBJECT.  I am consistently getting below error:
Copying build output files...Could not copy postgres.exe
 at install.pl line 26.

On digging, I found that it started failing with commit 3a7cc727 (Don't
fall off the end of perl functions).  It seems that we have added empty
'return' in all of the functions which seems to break one of the case:

lcopy("$conf\\$pf\\$pf.$ext", "$target\\$dir\\$pf.$ext")
|| croak "Could not copy $pf.$ext\n";

I think the return from lcopy implies 0 (or undef) which cause it to fail.
By reading couple of articles on net [1][2] related to this, it seems we
can't blindly return in all cases.  On changing, the lcopy as below,
install appears to be working.

@@ -40,7 +40,7 @@ sub lcopy
copy($src, $target)
  || confess "Could not copy $src to $target\n";
-   return;
+   return 1;
 }

I have randomly check some of the other functions where this patch has
added 'return' and those seem to be fine.

Is it by any chance related Perl version or some other settings?  If not,
then we should do something for it.

[1] - https://perlmaven.com/how-to-return-undef-from-a-function
[2] -
http://search.cpan.org/dist/Perl-Critic/lib/Perl/Critic/Policy/Subroutines/RequireFinalReturn.pm

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com