Conversation
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.
|
Some nits otherwise LGTM. @bnoordhuis PTAL too. |
|
Left some comments. Mostly LGTM though. I'm not quite sure why it was necessary to split up openssl.gyp. Can you explain? |
|
@bnoordhuis @indutny Thanks for reviewing. I will fix them soon.
@@ -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', |
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
d7f9be8 to
6b3c90c
Compare
|
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? |
|
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. |
|
@rvagg The armv8 support with assembler codes has already been done in the forthcoming commits as shigeki@d133ae5 and shigeki@1cca321. Could you install |
|
I'm very happy about the split tbh -- makes it so much more manageable. |
|
LGTM |
|
@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 |
|
perhaps @libuv/owners isn't right here, anyway, cc @saghul |
|
@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. |
|
LGTM |
|
@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 this doesn't look semver-minor though? It looks fine for a x.y.patch to me? |
|
@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. |
|
@shigeki Well, the commits themselves are fairly self explanatory, maybe don't squash? |
|
@Fishrock123 No, I don't squash them. I will merge each ones separately. |
PR-URL: #1325 Reviewed-By: Fedor Indutny <fedor@indutny.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
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>
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>
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>
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
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
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
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>
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>
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>
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>
|
For some reason e7c3f4a97b is showing on branch-diff for 6.x, not sure why as be65f5f is in the v6.x branch already. |
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>
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>
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 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 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>
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>
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
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.
I confirmed there is no differences in
out/deps/openssl/openssl.target.mkwith 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