classification
Title: build_ssl.py is relying on unreliable behaviour of os.popen
Type: compile error Stage: resolved
Components: Build, Windows Versions: Python 3.5, Python 3.4, Python 2.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: tim.golden Nosy List: Gabi.Davar, Waffle.Souffle, brian.curtin, loewis, ocean-city, python-dev, sable, srid, tim.golden, tim.peters, zach.ware
Priority: normal Keywords: patch

Created on 2010-12-21 23:09 by srid, last changed 2014-05-09 17:05 by tim.golden. This issue is now closed.

Files
File name Uploaded Description Edit
issue10752.patch tim.golden, 2014-05-09 09:56 review
Messages (11)
msg124467 - (view) Author: Sridhar Ratnakumar (srid) Date: 2010-12-21 23:09
I noticed that despite ActivePerl being installed, `os.popen(...).close()` returned 1 (see find_working_perl in build_ssl.py), while in actuality that command executed successfully with return code 0; I verified this by using the subprocess module.

Here's a patch:

--- python/PCbuild/build_ssl.py.orig
+++ python/PCbuild/build_ssl.py
@@ -45,11 +45,11 @@
 # Being a Perl dummy, the simplest way I can check is if the "Win32" package
 # is available.
 def find_working_perl(perls):
+    import subprocess
     for perl in perls:
-        fh = os.popen('"%s" -e "use Win32;"' % perl)
-        fh.read()
-        rc = fh.close()
-        if rc:
+        try:
+            subprocess.check_call('"%s" -e "use Win32;"' % perl, shell=True)
+        except subprocess.CalledProcessError:
             continue
         return perl
     print("Can not find a suitable PERL:")
msg148330 - (view) Author: Sébastien Sablé (sable) Date: 2011-11-25 16:06
I had the same issue while compiling Python 2.7 with ActivePerl on windows, and I can confirm that the proposed patch solves the issue.
msg185233 - (view) Author: Gabi Davar (Gabi.Davar) Date: 2013-03-25 20:52
possibly better patch - uses subprocess' quoting logic for argument handling:
diff -r 6fc9103d55f0 PCbuild/build_ssl.py
--- a/PCbuild/build_ssl.py	Sun Mar 24 14:54:25 2013 -0700
+++ b/PCbuild/build_ssl.py	Mon Mar 25 17:28:19 2013 +0200
@@ -47,10 +47,10 @@
 # is available.
 def find_working_perl(perls):
     for perl in perls:
-        fh = os.popen('"%s" -e "use Win32;"' % perl)
-        fh.read()
-        rc = fh.close()
-        if rc:
+        import subprocess
+        try:
+            subprocess.check_call([perl, '-e', 'use Win32;'], shell=True)
+        except subprocess.CalledProcessError:
             continue
         return perl
     print("Can not find a suitable PERL:")
msg204747 - (view) Author: Waffle Souffle (Waffle.Souffle) Date: 2013-11-29 18:24
Replicated with Perl being discovered as follows:
['C:\\perl\\bin\\perl.exe', 'c:\\cygwin\\bin\\perl.exe', 'c:\\perl\\bin\\perl.exe']

C:\perl\bin\perl.exe is installed from ActivePerl-5.16.3.1603-MSWin32-x86-296746.msi

Attempting to compile Python 2.7.6. Installed Python binary used to run build_openssl.py is Python 2.7.1.

As mentioned the call to the file handle's close returns 1 for every perl installation.

In this version of ActiveState Perl the errorlevel from running Perl with a successful command is 0, and with an unsuccessful command is 2.
perl -e "use Win32;"
echo %ERRORLEVEL%
 ==> 0

perl -e "use Win32x;"
echo %ERRORLEVEL%
 ==> 2
msg218007 - (view) Author: Tim Golden (tim.golden) * (Python committer) Date: 2014-05-06 19:18
Zach -- you've done most of the work on the VS projects lately. Would you mind having a look at this one to see if it's still relevant, please?
msg218017 - (view) Author: Zachary Ware (zach.ware) * (Python committer) Date: 2014-05-06 20:29
Still relevant, insofar as having Perl available is relevant.  That is, you only need Perl available if you're using vanilla OpenSSL sources, not if you're using sources pulled from svn.python.org (which most people should do, I would think).  I have never had a problem with build_ssl.py finding Perl, though.

That said, I don't see any harm in properly updating from os.popen to subprocess.

See also issue21141, which aims to make explicit that Perl is only needed to doctor up vanilla OpenSSL source.
msg218145 - (view) Author: Tim Golden (tim.golden) * (Python committer) Date: 2014-05-09 09:56
Here's a patch against build_ssl which uses subprocess.check_output and very slightly simplifies the output. It successfully finds ActivePerl and builds from source; and uses the svn export files when it's not.

I've targetted the development branch; don't know if there's mileage in backporting.
msg218146 - (view) Author: Tim Golden (tim.golden) * (Python committer) Date: 2014-05-09 10:06
I've just looked at issue21141 which is a substantial rework of this
area. This change should be incorporated over there as well / instead.
msg218168 - (view) Author: Zachary Ware (zach.ware) * (Python committer) Date: 2014-05-09 14:48
The patch looks good to me (only tested on a Perl-less machine, though).  It applies cleanly on 3.4, which I think is worth committing.  It still merges forward cleanly to default, post-#21141.  I suspect it would even apply fairly cleanly to 2.7, but I'm not too worried about that backport.
msg218177 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2014-05-09 17:03
New changeset 160f32753b0c by Tim Golden in branch '3.4':
Issue10752 Be more robust when finding a PERL interpreter to build OpenSSL. Initial patch by Gabi Davar
http://hg.python.org/cpython/rev/160f32753b0c

New changeset e492d0ac9abb by Tim Golden in branch 'default':
Issue10752 Be more robust when finding a PERL interpreter to build OpenSSL. Initial patch by Gabi Davar
http://hg.python.org/cpython/rev/e492d0ac9abb
msg218178 - (view) Author: Tim Golden (tim.golden) * (Python committer) Date: 2014-05-09 17:05
Thanks for the check. Committed to 3.4 & default
History
Date User Action Args
2014-05-09 17:05:50tim.goldensetstatus: open -> closed
stage: commit review -> resolved
2014-05-09 17:05:02tim.goldensetresolution: fixed
messages: + msg218178
2014-05-09 17:03:20python-devsetnosy: + python-dev
messages: + msg218177
2014-05-09 14:48:15zach.waresetassignee: tim.golden
messages: + msg218168
stage: patch review -> commit review
2014-05-09 10:06:37tim.goldensetmessages: + msg218146
2014-05-09 09:56:10tim.goldensetfiles: + issue10752.patch
keywords: + patch
messages: + msg218145
2014-05-06 20:29:55zach.waresetmessages: + msg218017
versions: + Python 3.5, - Python 3.3
2014-05-06 19:18:26tim.goldensetnosy: + zach.ware
messages: + msg218007
2013-11-29 18:56:38pitrousetassignee: ocean-city -> (no value)

nosy: + tim.peters
versions: + Python 3.4, - Python 3.2
2013-11-29 18:24:20Waffle.Soufflesetnosy: + Waffle.Souffle
messages: + msg204747
2013-03-25 20:52:53Gabi.Davarsetnosy: + Gabi.Davar
messages: + msg185233
2011-11-26 13:42:26pitrousetnosy: + tim.golden, brian.curtin
stage: patch review

versions: + Python 3.3
2011-11-25 16:06:17sablesetnosy: + sable

messages: + msg148330
versions: + Python 2.7
2010-12-21 23:12:10pitrousetassignee: ocean-city

nosy: + loewis, ocean-city
2010-12-21 23:09:40sridcreate