Perl coding error in msvc build system?
I'm not sure whether the following coding actually detects any errors:
Solution.pm:
open(P, "cl /? 2>&1 |") || die "cl command not found";
VSObjectFactory.pm:
open(P, "nmake /? 2>&1 |")
|| croak
"Unable to determine Visual Studio version: The nmake command wasn't found.";
The problem is that because of the shell special characters, the command
is run through the shell, but the shell ignores the exit status of
anything but the last command in the pipe. And the perlopentut man page
says "In such a case, the "open" call will only indicate failure if Perl
can't even run the shell.". I can confirm that on a *nix system. It
might be different on Windows, but it doesn't seem so.
The whole thing could probably be written in a less complicated way
using backticks, like
--- a/src/tools/msvc/Solution.pm
+++ b/src/tools/msvc/Solution.pm
@@ -72,16 +72,13 @@ sub DeterminePlatform # Examine CL help output to determine if we are in 32 or 64-bit mode.
$self->{platform} = 'Win32';
- open(P, "cl /? 2>&1 |") || die "cl command not found";
- while (<P>)
+ my $output = `cl /? 2>&1`;
+ $? >> 8 == 0 or die "cl command not found";
+ if ($output =~ /^\/favor:<.+AMD64/)
{
- if (/^\/favor:<.+AMD64/)
- {
- $self->{platform} = 'x64';
- last;
- }
+ $self->{platform} = 'x64';
+ last;
}
- close(P);
print "Detected hardware platform: $self->{platform}\n";
}--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
At 2014-06-03 22:30:50 -0400, peter_e@gmx.net wrote:
I'm not sure whether the following coding actually detects any errors:
Solution.pm:
open(P, "cl /? 2>&1 |") || die "cl command not found";
Since nobody with a Windows system has commented, I'm just writing to
say that from a Perl perspective, I agree with your analysis and the
patch looks perfectly sensible.
-- Abhijit
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 01/23/2015 03:17 AM, Abhijit Menon-Sen wrote:
At 2014-06-03 22:30:50 -0400, peter_e@gmx.net wrote:
I'm not sure whether the following coding actually detects any errors:
Solution.pm:
open(P, "cl /? 2>&1 |") || die "cl command not found";
Since nobody with a Windows system has commented, I'm just writing to
say that from a Perl perspective, I agree with your analysis and the
patch looks perfectly sensible.
Not quite. This line:
if ($output =~ /^\/favor:<.+AMD64/)
needs an m modifier on the regex, I think, so that the ^ matches any
beginning of line, not just the first.
cheers
andrew
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Why is the "last;" still there? AFAICS it matters in the old coding
because of the while, but I don't think there is an equivalent looping
construct in the new code.
--
�lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 01/23/2015 03:00 PM, Alvaro Herrera wrote:
Why is the "last;" still there? AFAICS it matters in the old coding
because of the while, but I don't think there is an equivalent looping
construct in the new code.
You're right, I missed that. It shouldn't be there, and will produce an
error like
Can't "last" outside a loop block
cheers
andrew
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Am 23.01.2015 um 09:17 schrieb Abhijit Menon-Sen:
At 2014-06-03 22:30:50 -0400, peter_e@gmx.net wrote:
I'm not sure whether the following coding actually detects any errors:
Solution.pm:
open(P, "cl /? 2>&1 |") || die "cl command not found";
Since nobody with a Windows system has commented, I'm just writing to
say that from a Perl perspective, I agree with your analysis and the
patch looks perfectly sensible.
I can confirm it on my Windows system.
Calling build from a console without nmake in the path I always get:
Unable to determine Visual Studio version: The nmake version could not
be determined. at src/tools/msvc/Mkvcbuild.pm line 63.
This means that the following construct in VSObjectFactory.pm doesn't
have the desired effect.
open(P, "nmake /? 2>&1 |")
|| croak
"Unable to determine Visual Studio version: The nmake command wasn't
found.";
On the other hand complicacy is in the eye of the beholder.
Perl constructs like the following get quite a few wtf's
(http://www.osnews.com/story/19266/WTFs_m) from a simple-minded person
like me.
$? >> 8 == 0 or die "cl command not found";
However as it fixes a confirmed problem and as maintainance of perl code
is an issue of its own, please go ahead.
Regards,
Brar
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Fri, Jan 23, 2015 at 4:17 PM, Brar Piening <brar@gmx.de> wrote:
Am 23.01.2015 um 09:17 schrieb Abhijit Menon-Sen:
At 2014-06-03 22:30:50 -0400, peter_e@gmx.net wrote:
I'm not sure whether the following coding actually detects any errors:
Solution.pm:
open(P, "cl /? 2>&1 |") || die "cl command not found";
Since nobody with a Windows system has commented, I'm just writing to
say that from a Perl perspective, I agree with your analysis and the
patch looks perfectly sensible.I can confirm it on my Windows system.
Calling build from a console without nmake in the path I always get:
Unable to determine Visual Studio version: The nmake version could not be
determined. at src/tools/msvc/Mkvcbuild.pm line 63.This means that the following construct in VSObjectFactory.pm doesn't have
the desired effect.
open(P, "nmake /? 2>&1 |")
|| croak
"Unable to determine Visual Studio version: The nmake command wasn't
found.";On the other hand complicacy is in the eye of the beholder.
Perl constructs like the following get quite a few wtf's
(http://www.osnews.com/story/19266/WTFs_m) from a simple-minded person like
me.
$? >> 8 == 0 or die "cl command not found";However as it fixes a confirmed problem and as maintainance of perl code is
an issue of its own, please go ahead.
This patch been reviewed by 4 people, resulting in 2 minor suggestions
for changes (adding an m modifier, and removing a bogus last).
It has 2 clear upvotes and 0 downvotes.
I think it should be revised along the lines suggested and committed
without further ado.
--
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
This patch been reviewed by 4 people, resulting in 2 minor suggestions
for changes (adding an m modifier, and removing a bogus last).It has 2 clear upvotes and 0 downvotes.
I think it should be revised along the lines suggested and committed
without further ado.
My patch actually only covered the first of the two faulty locations I
pointed out. Attached is a patch that also fixes the second one. I
noticed that DetermineVisualStudioVersion() is never called with an
argument, so I removed that branch altogether.
Attachments:
msvc-open-pipe-v2.patchtext/x-patch; name=msvc-open-pipe-v2.patchDownload
diff --git a/src/tools/msvc/Solution.pm b/src/tools/msvc/Solution.pm
index 39e41f6..714585f 100644
--- a/src/tools/msvc/Solution.pm
+++ b/src/tools/msvc/Solution.pm
@@ -71,17 +71,9 @@ sub DeterminePlatform
my $self = shift;
# Examine CL help output to determine if we are in 32 or 64-bit mode.
- $self->{platform} = 'Win32';
- open(P, "cl /? 2>&1 |") || die "cl command not found";
- while (<P>)
- {
- if (/^\/favor:<.+AMD64/)
- {
- $self->{platform} = 'x64';
- last;
- }
- }
- close(P);
+ my $output = `cl /? 2>&1`;
+ $? >> 8 == 0 or die "cl command not found";
+ $self->{platform} = ($output =~ /^\/favor:<.+AMD64/m) ? 'x64' : 'Win32';
print "Detected hardware platform: $self->{platform}\n";
}
diff --git a/src/tools/msvc/VSObjectFactory.pm b/src/tools/msvc/VSObjectFactory.pm
index d255bec..b83af40 100644
--- a/src/tools/msvc/VSObjectFactory.pm
+++ b/src/tools/msvc/VSObjectFactory.pm
@@ -92,30 +92,16 @@ sub CreateProject
sub DetermineVisualStudioVersion
{
- my $nmakeVersion = shift;
-
- if (!defined($nmakeVersion))
- {
-
-# Determine version of nmake command, to set proper version of visual studio
-# we use nmake as it has existed for a long time and still exists in current visual studio versions
- open(P, "nmake /? 2>&1 |")
- || croak
-"Unable to determine Visual Studio version: The nmake command wasn't found.";
- while (<P>)
- {
- chomp;
- if (/(\d+)\.(\d+)\.\d+(\.\d+)?$/)
- {
- return _GetVisualStudioVersion($1, $2);
- }
- }
- close(P);
- }
- elsif ($nmakeVersion =~ /(\d+)\.(\d+)\.\d+(\.\d+)?$/)
+ # To determine version of Visual Studio we use nmake as it has
+ # existed for a long time and still exists in current Visual
+ # Studio versions.
+ my $output = `nmake /? 2>&1`;
+ $? >> 8 == 0 or croak "Unable to determine Visual Studio version: The nmake command wasn't found.";
+ if ($output =~ /(\d+)\.(\d+)\.\d+(\.\d+)?$/m)
{
return _GetVisualStudioVersion($1, $2);
}
+
croak
"Unable to determine Visual Studio version: The nmake version could not be determined.";
}
On Tue, Feb 17, 2015 at 6:34 AM, Peter Eisentraut <peter_e@gmx.net> wrote:
This patch been reviewed by 4 people, resulting in 2 minor suggestions
for changes (adding an m modifier, and removing a bogus last).It has 2 clear upvotes and 0 downvotes.
I think it should be revised along the lines suggested and committed
without further ado.My patch actually only covered the first of the two faulty locations I
pointed out. Attached is a patch that also fixes the second one. I
noticed that DetermineVisualStudioVersion() is never called with an
argument, so I removed that branch altogether.
+1 for those simplifications. And FWIW, it passed my series of tests
with MSVC 2010.
--
Michael--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers