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

Upgrade to openssl-1.0.2a (Part1) #1325

Closed
wants to merge 6 commits into from

Conversation

shigeki
Copy link
Contributor

@shigeki shigeki commented Apr 2, 2015

This is a resubmitted PR of #723. It still stays in 1.0.1m but requires the following 5 changes just before upgrading 1.0.2a.

  • upgrade gyp to use else if conditions as supported in https://codereview.chromium.org/601353002
  • refactor openssl.gyp so as to switch target_arch and OS with "else if" condition
  • move sources, rules and defines into openssl.gypi, openssl-cli.gypi and masm_compile.gypi
  • apply two private patches to fix gyp to run on MacOS without XCode and Fedor's fix for python2.6.
  • add rsa and aes-gcm benchmarks and new hash algorithms in order to compare the performances after upgrade.

I confirmed there is no differences in out/deps/openssl/openssl.target.mk with this changes on several platforms.

CI shows that there are a few new errors on win2008r2 but they are not related to this change. Others are fine except known failures.
https://jenkins-iojs.nodesource.com/job/iojs+any-pr+multi/430/

After this, I will submit the 2nd PR to finish upgrade to 1.0.2a in https://github.com/shigeki/io.js/tree/upgrade_openssl102a.
Preliminary results to show performance gains after upgrade are in https://gist.github.com/shigeki/eefc31f9d82f45f9a8e3 .

R= @bnoordhuis @indutny

Shigeki Ohtsu and others added 3 commits April 2, 2015 21:37
@indutny
Copy link
Member

indutny commented Apr 2, 2015

Some nits otherwise LGTM. @bnoordhuis PTAL too.

@mscdex mscdex added the openssl Issues and PRs related to the OpenSSL dependency. label Apr 2, 2015
@bnoordhuis
Copy link
Member

Left some comments. Mostly LGTM though.

I'm not quite sure why it was necessary to split up openssl.gyp. Can you explain?

@shigeki
Copy link
Contributor Author

shigeki commented Apr 3, 2015

@bnoordhuis @indutny Thanks for reviewing. I will fix them soon.
The reasons why I splited up openssl.gyp are that

  • Sources and defines in openssl.gyp are getting so huge because of increase of supported architectures such as arms and two sets of asm files, one is for the latest avx2 supported compiler and the other is for the older.
  • We can only focus and modify the changes of sources and defines of openssl.gypi in version up and less worries about platform dependent logics that are defined in openssl.gyp.
  • It's easy to add a new supported architecture. For example, adding armv8 is as blow in openssl.gyp, where it's easy to debug and identify issues when they occur.
@@ -69,6 +69,9 @@
                 '<@(openssl_defines_x64_elf)',
               ],
               'sources': ['<@(openssl_sources_x64_elf_gas)'],
+            }, 'target_arch=="arm64"', {
+              'defines': ['<@(openssl_defines_arm64)',],
+              'sources': ['<@(openssl_sources_arm64_linux64_gas)'],
             }, { # else other archtectures does not use asm
               'defines': [
                 'OPENSSL_NO_ASM',

Shigeki Ohtsu added 3 commits April 3, 2015 11:24
Updated gyp has "else if" syntax in condition. Use this for
target_arch and OS switches. Several sources, defines, rules and
libraries variables moved to gypi files.
add sha1, sha512 algorithm and remove md5
@shigeki
Copy link
Contributor Author

shigeki commented Apr 3, 2015

Styles are fixed by checking with gjslint. Other fixes are made according comments and I replied them. The branch are force pushed.

@bnoordhuis Could you give me an another LGTM if you accept my explanations above?

@rvagg
Copy link
Member

rvagg commented Apr 3, 2015

very exciting, I won't lgtm because this is out of my area but does this reenable openssl on armv8 as well? my understanding is that 1.0.2 should give us the support we need there.

@shigeki
Copy link
Contributor Author

shigeki commented Apr 3, 2015

@rvagg The armv8 support with assembler codes has already been done in the forthcoming commits as shigeki@d133ae5 and shigeki@1cca321. Could you install automake and libtool on the armv8 host in Jenkins for libuv tests? After resolving the build failure of libuv in https://jenkins-iojs.nodesource.com/job/libuv+any-pr+multi/nodes=iojs-armv8-ubuntu1404/lastBuild/console, I can work spawnSync failures on armv8 with shigeki@5baf3dd .

@jbergstroem
Copy link
Member

I'm very happy about the split tbh -- makes it so much more manageable.

@bnoordhuis
Copy link
Member

LGTM

@rvagg
Copy link
Member

rvagg commented Apr 3, 2015

@shigeki sorry, I think this is good to go now but the state of libuv tests on armv8 is apparently pretty tragic! https://jenkins-iojs.nodesource.com/job/libuv+any-pr+multi/74/nodes=iojs-armv8-ubuntu1404/console /cc @libuv/owners

@rvagg
Copy link
Member

rvagg commented Apr 3, 2015

perhaps @libuv/owners isn't right here, anyway, cc @saghul

@bnoordhuis
Copy link
Member

@rvagg If you can give me a login for the armv8 bot, I can look into it. I'm 95% sure I know what's wrong but I need some time with strace to confirm.

@indutny
Copy link
Member

indutny commented Apr 3, 2015

LGTM

@rvagg
Copy link
Member

rvagg commented Apr 3, 2015

@bnoordhuis will have to do this later, it's behind a jump-host that's set up just with my creds so I'll probably need to make an ssh tunnel to let you in to it.

@shigeki shigeki mentioned this pull request Apr 4, 2015
@Fishrock123
Copy link
Contributor

@shigeki this doesn't look semver-minor though? It looks fine for a x.y.patch to me?

@shigeki
Copy link
Contributor Author

shigeki commented Apr 4, 2015

@Fishrock123 Yes, it does not contain API changes. But I wonder it makes some confusion as this title is the first part of upgrading of the deps library. If you don't mind, I will merge this right now.

@Fishrock123
Copy link
Contributor

@shigeki Well, the commits themselves are fairly self explanatory, maybe don't squash?

@shigeki
Copy link
Contributor Author

shigeki commented Apr 4, 2015

@Fishrock123 No, I don't squash them. I will merge each ones separately.

shigeki pushed a commit that referenced this pull request Apr 4, 2015
PR-URL: #1325
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
indutny added a commit that referenced this pull request Apr 4, 2015
Fixes: nodejs/node-v0.x-archive#6859
PR-URL: #1325
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Shigeki Ohtsu <ohtsu@iij.ad.jp>
shigeki pushed a commit that referenced this pull request Apr 4, 2015
This issue has already submitted to the upstream in
https://code.google.com/p/gyp/issues/detail?id=477
Use this commit until the upstream is to be fixed.

PR-URL: #1325
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
shigeki pushed a commit that referenced this pull request Apr 4, 2015
Updated gyp has "else if" syntax in condition. Use this for
target_arch and OS switches. Several sources, defines, rules and
libraries variables moved to gypi files.

PR-URL: #1325
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
MylesBorins pushed a commit that referenced this pull request Mar 21, 2016
This LTS release comes with 113 commits, 56 of which are doc related,
18 of which are build / tooling related, 16 of which are test related
and 7 which are benchmark related.

Notable Changes:

* build:
  - Updated Logos for the OSX + Windows installers
    - (Rod Vagg) #5401
    - (Robert Jefe Lindstaedt) #5531
  - New option to select you VS Version in the Windows installer
    - (julien.waechter) #4645
  - Support Visual C++ Build Tools 2015
    - (João Reis) #5627
* tools:
  - Gyp now works on OSX without XCode
    - (Shigeki Ohtsu) #1325
MylesBorins pushed a commit that referenced this pull request Mar 22, 2016
This LTS release comes with 113 commits, 56 of which are doc related,
18 of which are build / tooling related, 16 of which are test related
and 7 which are benchmark related.

Notable Changes:

* build:
  - Updated Logos for the OSX + Windows installers
    - (Rod Vagg) #5401
    - (Robert Jefe Lindstaedt) #5531
  - New option to select your VS Version in the Windows installer
    - (julien.waechter) #4645
  - Support Visual C++ Build Tools 2015
    - (João Reis) #5627
* tools:
  - Gyp now works on OSX without XCode
    - (Shigeki Ohtsu) #1325
MylesBorins pushed a commit that referenced this pull request Mar 22, 2016
This LTS release comes with 113 commits, 56 of which are doc related,
18 of which are build / tooling related, 16 of which are test related
and 7 which are benchmark related.

Notable Changes:

* build:
  - Updated Logos for the OSX + Windows installers
    - (Rod Vagg) #5401
    - (Robert Jefe Lindstaedt) #5531
  - New option to select your VS Version in the Windows installer
    - (julien.waechter) #4645
  - Support Visual C++ Build Tools 2015
    - (João Reis) #5627
* tools:
  - Gyp now works on OSX without XCode
    - (Shigeki Ohtsu) #1325

PR-URL: #5835
bnoordhuis pushed a commit to bnoordhuis/io.js that referenced this pull request Oct 8, 2016
This issue has already submitted to the upstream in
https://code.google.com/p/gyp/issues/detail?id=477
Use this commit until the upstream is to be fixed.

PR-URL: nodejs#1325
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
jBarz pushed a commit to ibmruntimes/node that referenced this pull request Nov 4, 2016
This is a port of eb459c8 ,
used as a floating patch over gyp.

Original commit message:

  This issue has already submitted to the upstream in
  https://code.google.com/p/gyp/issues/detail?id=477
  Use this commit until the upstream is to be fixed.

  PR-URL: nodejs/node#1325
  Reviewed-By: Fedor Indutny <fedor@indutny.com>
  Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

This was ported to v0.10 in nodejs#25857

PR-URL: nodejs/node#2843
Reviewed-By: rvagg - Rod Vagg <rod@vagg.org>
Reviewed-By: orangemocha - Alexis Campailla <orangemocha@nodejs.org>
Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl>
refack pushed a commit that referenced this pull request Apr 18, 2017
This issue has already submitted to the upstream in
https://code.google.com/p/gyp/issues/detail?id=477
Use this commit until the upstream is to be fixed.

PR-URL: #1325
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
refack pushed a commit to refack/GYP3 that referenced this pull request May 21, 2017
Most builds are possible with just the
"Command Line Tools for XCode"
nodejs has extensive experience with this scenario

BUG=gyp:477

nodejs PR-URL: nodejs/node#1325
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@jasnell jasnell mentioned this pull request May 11, 2017
@gibfahn gibfahn mentioned this pull request Jun 15, 2017
3 tasks
@gibfahn
Copy link
Member

gibfahn commented Jun 18, 2017

For some reason e7c3f4a97b is showing on branch-diff for 6.x, not sure why as be65f5f is in the v6.x branch already.

refack pushed a commit to refack/node that referenced this pull request Jul 18, 2017
This issue has already submitted to the upstream in
https://code.google.com/p/gyp/issues/detail?id=477
Use this commit until the upstream is to be fixed.

PR-URL: nodejs#1325
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
refack pushed a commit to refack/node that referenced this pull request Aug 23, 2017
This issue has already submitted to the upstream in
https://code.google.com/p/gyp/issues/detail?id=477
Use this commit until the upstream is to be fixed.

PR-URL: nodejs#1325
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
addaleax pushed a commit to addaleax/ayo that referenced this pull request Aug 25, 2017
This issue has already submitted to the upstream in
https://code.google.com/p/gyp/issues/detail?id=477
Use this commit until the upstream is to be fixed.

PR-URL: nodejs/node#1325
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
addaleax pushed a commit to ayojs/ayo that referenced this pull request Aug 28, 2017
This issue has already submitted to the upstream in
https://code.google.com/p/gyp/issues/detail?id=477
Use this commit until the upstream is to be fixed.

PR-URL: nodejs/node#1325
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
MylesBorins pushed a commit that referenced this pull request Sep 10, 2017
This issue has already submitted to the upstream in
https://code.google.com/p/gyp/issues/detail?id=477
Use this commit until the upstream is to be fixed.

PR-URL: #1325
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@MylesBorins MylesBorins mentioned this pull request Sep 10, 2017
MylesBorins pushed a commit that referenced this pull request Sep 12, 2017
This issue has already submitted to the upstream in
https://code.google.com/p/gyp/issues/detail?id=477
Use this commit until the upstream is to be fixed.

PR-URL: #1325
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
duqingnian pushed a commit to duqingnian/gyp that referenced this pull request Apr 26, 2024
Most builds are possible with just the
"Command Line Tools for XCode"
nodejs has extensive experience with this scenario

BUG=gyp:477

nodejs PR-URL: nodejs/node#1325
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

Change-Id: I89ce12a8c92db6172f2f13d168233504d6de57ab
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
openssl Issues and PRs related to the OpenSSL dependency.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants