Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 4 commits into from Jun 20, 2017

Conversation

jkloth
Copy link
Contributor

@jkloth jkloth commented May 25, 2017

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
Copy link
Contributor Author

jkloth commented May 25, 2017

A couple of questions.

  1. 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.

  2. 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
Copy link
Member

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.

@@ -1,7 +1,7 @@
<?xml version="1.0" encoding="Windows-1252"?>
<VisualStudioProject
ProjectType="Visual C++"
Version="9,00"
Version="9.00"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -1,4 +1,5 @@
#! /usr/bin/env python3
# -*- coding: utf8 -*- for <2.7>/PC/VS9.0/build_ssl.py
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

jkloth commented Jun 19, 2017

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
Copy link
Member

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
Copy link
Contributor Author

jkloth commented Jun 20, 2017

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
Copy link
Member

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
Copy link
Contributor Author

jkloth commented Jun 20, 2017

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

@jkloth
Copy link
Contributor Author

jkloth commented Jun 20, 2017

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.

@vstinner vstinner merged commit ebbccea into python:2.7 Jun 20, 2017
@jkloth jkloth deleted the issue30368 branch December 11, 2018 16:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants