MSVC pl-perl error message is not verbose enough
Hello folks,
I've got a small patch I'd like to submit for consideration.
The scenario involves MSVC builds, where a user can do the following:
1) Install ActivePerl's latest (5.22 or 5.24).
2) Add this line to their config.pl file (in src/tools/msvc):
$config->{perl} = "C:\\Perl64";
This will enable pl-perl as part of the build configuration
3) Try to run "build" using MSVC
However, users that do this will end up with a curious error:
"Could not identify perl library version at src/tools/msvc/Mkvcbuild.pm
line 583."
This is a confusing error to see, especially since ActivePerl was just
installed.
It makes debugging a little difficult (path issue? Perl installation
issue? Code issue?).
Other folks have seen this error before and been similiarly confused:
/messages/by-id/DUB126-W931DEA250B21C3A48DCABAD10C0@phx.gbl
I've actually followed up with that submitter, and he had said that after a
couple of days, they gave up on finding the solution for this error and
tried a different path.
The problem is that a default installation of ActivePerl installs
perl524.dll, but not perl524.lib. I'm not sure if there's any way to get
ActivePerl to install the lib file as part of its installation process, so
I resorted to generating my own .lib file from the .dll that was
installed. After doing that, and placing the .lib file in
C:\Perl64\lib\CORE, the error goes away.
The user does at this point have to modify the ActivePerl config file so
that it will use MSVC compiler formats instead of gcc by modifying
C:\Perl64\lib\CORE\config.h
from:
#define PERL_STATIC_INLINE static __inline__
to
#define PERL_STATIC_INLINE static __inline
After that, it works.
I understand there's a lot of stuff out of the postgres community's hands
here; we can't expect to solve ActivePerl problems. However, I think that
the error message in the MSVC scripts isn't verbose enough to tell people
what to look for. If their perl skills aren't very good, it may take some
learning just to figure out what the problem is.
Because of this, I've submitted a small patch which fixes the verbosity of
the error message to actually explain what's missing. I hope that this
patch will be considered for the community, and it would be nice if it was
back-patched.
Attached is my patch for review.
Thank you,
-John Harvey
Attachments:
msvc_pl_perl_error_verbose.patchapplication/octet-stream; name=msvc_pl_perl_error_verbose.patchDownload
From 8675330680a5d1c8ffae0565a9ba4382fdee908a Mon Sep 17 00:00:00 2001
From: "John K. Harvey" <john.harvey@crunchydata.com>
Date: Tue, 26 Jul 2016 11:07:15 -0400
Subject: [PATCH] MSVC pl-perl error message is not verbose enough
---
src/tools/msvc/Mkvcbuild.pm | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/src/tools/msvc/Mkvcbuild.pm b/src/tools/msvc/Mkvcbuild.pm
index fe905d3..bb73485 100644
--- a/src/tools/msvc/Mkvcbuild.pm
+++ b/src/tools/msvc/Mkvcbuild.pm
@@ -587,7 +587,9 @@ sub mkvcbuild
}
else
{
- die "could not identify perl library version";
+ die 'Could not find suitable perl*.lib file (not DLL) in Perl search-path: ' .
+ $solution->{options}->{perl} .
+ '\lib\CORE';
}
# Add transform module dependent on plperl
--
2.5.0
On Wed, Jul 27, 2016 at 12:41 AM, John Harvey
<john.harvey@crunchydata.com> wrote:
Because of this, I've submitted a small patch which fixes the verbosity of
the error message to actually explain what's missing. I hope that this
patch will be considered for the community, and it would be nice if it was
back-patched.
Improving this error message a bit looks like a good idea to me.
Looking at the code to figure out what build.pl is looking for is a
bit a pain for users just willing to compile the code, so if we can
avoid such lookups with a cheap way, let's do it.
Instead of your patch, I'd suggest saving $solution->{options}->{perl}
. '\lib\CORE\perl*.lib' in a variable and then raise an error based on
that. See attached.
--
Michael
Attachments:
msvc_pl_perl_error_verbose_v2.patchapplication/x-patch; name=msvc_pl_perl_error_verbose_v2.patchDownload
diff --git a/src/tools/msvc/Mkvcbuild.pm b/src/tools/msvc/Mkvcbuild.pm
index fe905d3..5fad939 100644
--- a/src/tools/msvc/Mkvcbuild.pm
+++ b/src/tools/msvc/Mkvcbuild.pm
@@ -578,16 +578,17 @@ sub mkvcbuild
}
}
$plperl->AddReference($postgres);
+ my $perl_path = $solution->{options}->{perl} . '\lib\CORE\perl*.lib';
my @perl_libs =
grep { /perl\d+.lib$/ }
- glob($solution->{options}->{perl} . '\lib\CORE\perl*.lib');
+ glob($perl_path);
if (@perl_libs == 1)
{
$plperl->AddLibrary($perl_libs[0]);
}
else
{
- die "could not identify perl library version";
+ die "could not identify perl library version matching pattern $perl_path\n";
}
# Add transform module dependent on plperl
On Tue, Jul 26, 2016 at 9:44 PM, Michael Paquier <michael.paquier@gmail.com>
wrote:
On Wed, Jul 27, 2016 at 12:41 AM, John Harvey
<john.harvey@crunchydata.com> wrote:Because of this, I've submitted a small patch which fixes the verbosity
of
the error message to actually explain what's missing. I hope that this
patch will be considered for the community, and it would be nice if itwas
back-patched.
Improving this error message a bit looks like a good idea to me.
Looking at the code to figure out what build.pl is looking for is a
bit a pain for users just willing to compile the code, so if we can
avoid such lookups with a cheap way, let's do it.Instead of your patch, I'd suggest saving $solution->{options}->{perl}
. '\lib\CORE\perl*.lib' in a variable and then raise an error based on
that. See attached.
Looks good to me. I think this is a cleaner approach.
I've just verified that it works correctly, so I'm happy with it.
-John
On Tue, Jul 26, 2016 at 9:44 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:
On Wed, Jul 27, 2016 at 12:41 AM, John Harvey
<john.harvey@crunchydata.com> wrote:Because of this, I've submitted a small patch which fixes the verbosity of
the error message to actually explain what's missing. I hope that this
patch will be considered for the community, and it would be nice if it was
back-patched.Improving this error message a bit looks like a good idea to me.
Looking at the code to figure out what build.pl is looking for is a
bit a pain for users just willing to compile the code, so if we can
avoid such lookups with a cheap way, let's do it.Instead of your patch, I'd suggest saving $solution->{options}->{perl}
. '\lib\CORE\perl*.lib' in a variable and then raise an error based on
that. See attached.
Did you add this to the next CommitFest?
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Tue, Aug 2, 2016 at 2:08 AM, Robert Haas <robertmhaas@gmail.com> wrote:
Did you add this to the next CommitFest?
I have added it here:
https://commitfest.postgresql.org/10/691/
John, it would be good if you could get a community account and add
your name to this patch as its author. I could not find you.
--
Michael
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Mon, Aug 1, 2016 at 9:39 PM, Michael Paquier <michael.paquier@gmail.com>
wrote:
On Tue, Aug 2, 2016 at 2:08 AM, Robert Haas <robertmhaas@gmail.com> wrote:
Did you add this to the next CommitFest?
I have added it here:
https://commitfest.postgresql.org/10/691/
John, it would be good if you could get a community account and add
your name to this patch as its author. I could not find you.
Done. If there's anything else that I should do, please let me know. I'd
be glad to help out.
Thanks!
-John
On 08/02/2016 08:18 PM, John Harvey wrote:
On Mon, Aug 1, 2016 at 9:39 PM, Michael Paquier <michael.paquier@gmail.com>
wrote:On Tue, Aug 2, 2016 at 2:08 AM, Robert Haas <robertmhaas@gmail.com> wrote:
Did you add this to the next CommitFest?
I have added it here:
https://commitfest.postgresql.org/10/691/
John, it would be good if you could get a community account and add
your name to this patch as its author. I could not find you.Done. If there's anything else that I should do, please let me know. I'd
be glad to help out.
Committed, thanks!
- Heikki
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers