bpo-30368: Update build_ssl.py to restore Perl-less building by jkloth · Pull Request #1805 · python/cpython

jkloth · 2017-05-25T00:28:10Z

OpenSSL 1.0.2 releases changed how files are copied in the makefile,
thus causing Perl to be required even for Python's "prepared" OpenSSL.
Now build_ssl.py does the requisite copies before running nmake.

jkloth · 2017-05-25T00:41:11Z

A couple of questions.

My fresh run of prepare_ssl.py generates a buildinf.h that contains both platform (VC-WIN32 & VC-WIN64A) sections, the one from externals does not. If the externals one did, the code for buildinf.h could be removed from build_ssl.py.
The include directories were changed in prepare_ssl.py (from "inc{suffix}" to "include{suffix}"). If the VS9 project files were also updated to that, another few lines could be removed from build_ssl.py

If (2) happens it would be a great time to fix (1) as well since changes for (2) require a "forced" OpenSSL rebuild on the buildbots, AFAIK.

vstinner · 2017-06-19T13:24:19Z

Your change has now conflicts and I have comments, but at least it works :-)

I tested your change on Windows with Visual Studio 2008.

Test 1 using build.bat: _ssl is correctly built

git clean -fdx  # Remove all untracked files
PC\VS9.0\build.bat -e -d -p x64
PC\VS9.0\python_d.exe -c "import ssl"

Test 2 using VS GUI:

Run git clean -fdx
Open the VS2008 project
Build the project: _ssl is not available, built failed
Build again the project: _ssl is compiled
Run Python from VS, type "import _ssl": module correctly imported

Oh, it seems like Perl is installed on my Windows. But without this change, I was unable to build _ssl for Python 2.7 in VS2008.

vstinner · 2017-06-19T13:24:44Z

PC/VS9.0/_hashlib.vcproj

I don't understand these changes: 9,00 replaced with 9.00. Revert them?

That's changed by VisualStudio itself in accordance with machine's region settings. U.S. decimal point vs. European? decimal.

vstinner · 2017-06-19T13:26:32Z

PCbuild/prepare_ssl.py

Please just write "# coding: utf8" to avoid issues.

I opted for the Emacs/VI style mode-line to allow for trailing comment as to not have those editors complain. If that is a non-issue for them, even better.

The this is recommended form of the encoding declaration per:
https://docs.python.org/3/reference/lexical_analysis.html#encoding-declarations
unless you are referring to the trailing comment.

The trailing comment is there to keep the line from being deleted again.

The recommanded format is "# -- coding: --", not "# -- coding: -- for <2.7>/PC/VS9.0/build_ssl.py". I'm concerned by the trailing comment.

I firmly believe that a comment is needed as this encoding decl has been removed once already. Not to mention that, with it removed, there will be no discernible issues with its removal until an updated OpenSSL is pushed to the buildbots. Especially due to the fact this is in a seldom used corner of the Python build process.

"this encoding decl has been removed once already" I don't understand why someone would removing this encoding declaration, the file is encoded to UTF-8.

If you want to keep a comment, just put it on the following line, no?

prepare_ssl.py is (currently) only used when pushing a new OpenSSL to the externals repo. Its purportedly to be run by python3 (see shebang), as all Python (Windows) releases use the same externals.

I generally prefer inline comments for "notes", especially compared to following comments, but not strongly enough to hold up this inclusion.

jkloth · 2017-06-19T15:36:06Z

OK, I'm at wits end. Git is giving me fits trying to resolve this. I always end up with an error on push about fast-forwards. This should be SO much easier than this. All that changed was line-endings.

vstinner · 2017-06-20T14:40:53Z

What happened to this PR? Now you modified 197 files :-)

OpenSSL 1.0.2 releases changed how files are copied in the makefile, thus causing Perl to be required even for Python's "prepared" OpenSSL. Now build_ssl.py does the requisite copies before running nmake.

* Updates SSL-linking projects to use the new include{suffix} directory * build_ssl.py now only copies those files not handled by prepare_ssl.py

* Update SSL-linking projects to use the new include{suffix} directory

jkloth · 2017-06-20T15:38:06Z

After sleeping on it and, apparently, the right google-fu, the command needed to merge changes made to EOL is:

git pull --rebase -s recursive -X renormalize

I believe that this should be mentioned somewhere in the devguide alongside a note about changes to .gitattributes wrt eol changes. This caused some serious hair-pulling.

Update: missed the --rebase on the first attempt :)

serhiy-storchaka · 2017-06-20T15:49:29Z

I always use just git merge master. Or, more detailed:

git checkout master
git pull upstream
git push origin
git checkout feature-branch
git merge master
# if there are conflicts, resolve conflicts and run
# git commit -a
git push origin

In rare cases, if the PR still was not reviewed, I can use git rebase master.

jkloth · 2017-06-20T16:03:26Z

I did those steps, however on pushing to origin, I ended up with errors regarding non-fast-forwards.

jkloth · 2017-06-20T16:37:25Z

Regarding the build tests, your test 2 (w/GUI) missed the step of get_externals (which build.bat does for you). git clean -fdx removes the downloaded externals so building in the GUI should fail without that step. The readme(s) could be improved there, but that is for another issue.

the-knights-who-say-ni added the CLA signed label May 25, 2017

vstinner requested changes Jun 19, 2017

View reviewed changes

jkloth added 3 commits June 20, 2017 09:33

bpo-30368: Update build_ssl.py to restore Perl-less building

30d75b8

OpenSSL 1.0.2 releases changed how files are copied in the makefile, thus causing Perl to be required even for Python's "prepared" OpenSSL. Now build_ssl.py does the requisite copies before running nmake.

bpo-30368: Update build_ssl.py to use prepared OpenSSL

0cde7f7

* Updates SSL-linking projects to use the new include{suffix} directory * build_ssl.py now only copies those files not handled by prepare_ssl.py

* bpo-30368: Update build_ssl.py to use prepared OpenSSL

a435a5a

* Update SSL-linking projects to use the new include{suffix} directory

jkloth force-pushed the issue30368 branch from 1493681 to a435a5a Compare June 20, 2017 15:35

Move comment to following line

90fe69f

vstinner approved these changes Jun 20, 2017

View reviewed changes

vstinner merged commit ebbccea into python:2.7 Jun 20, 2017

jkloth deleted the issue30368 branch December 11, 2018 16:38

Uh oh!

Conversation

jkloth commented May 25, 2017

Uh oh!

jkloth commented May 25, 2017

Uh oh!

vstinner commented Jun 19, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkloth commented Jun 19, 2017

Uh oh!

vstinner commented Jun 20, 2017

Uh oh!

jkloth commented Jun 20, 2017

Uh oh!

serhiy-storchaka commented Jun 20, 2017

Uh oh!

jkloth commented Jun 20, 2017

Uh oh!

jkloth commented Jun 20, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants