upgrade gyp and refactor openssl.gyp #723

shigeki · 2015-02-05T13:59:09Z

I would like to use openssl-1.0.2 on iojs but openssl.gyp is so large and complicated that I think it need some refactoring before updating to openssl-1.0.2.

This PR has the following commits as

upgrade gyp to use else if conditions as supported in https://codereview.chromium.org/601353002
move all openssl sources into openssl.gypi
refactor openssl.gyp so as to switch target_arch and OS with "else if" condition and move most of defines to gypi.

Testing is hard to confirm this in all platforms. I made two tests as below.

Comparing all diffs of out/deps/openssl/openssl.target.mk with configure --dest-CPU=[win mac solaris freebsd openbsd linux android] --dest-os=[arm ia32 x32 x64] between before and after this PR and there is no difference in makefiles. In windows, there are small diffs in openssl.vcxprj , but it comes from updating gyp.
Build and make test on linux-x86_64, linux-x86, MacOS-x86_64, Win-x86_64. All tests were passed.

WIP branch for openssl102 is in https://github.com/shigeki/io.js/tree/WIP_upgrade_openssl102 . It's working on only x86_64 of Linux and MacOSX.

CC: @bnoordhuis @indutny

bnoordhuis · 2015-02-05T18:12:27Z

The changes look reasonable to me, let's see what the CI thinks: https://jenkins-iojs.nodesource.com/view/iojs/job/iojs+any-pr+multi/149/

I noticed that OS X 10.10 seems unhappy right from the start: https://jenkins-iojs.nodesource.com/view/iojs/job/iojs+any-pr+multi/149/nodes=osx1010/console

indutny · 2015-02-05T19:25:35Z

I'm fine with update, except some (mostly) style nits.

Fishrock123 · 2015-02-05T19:51:43Z

armv6/7 and freebsd failures appear unrelated.

shigeki · 2015-02-06T02:07:27Z

@bnoordhuis @indutny Thanks for your review. I can reproduce the configure error on my MacOS by uninstalling XCode and using only command line tools. Probably it comes from a upstream bug. I continue to investigate it.

shigeki · 2015-02-06T05:24:00Z

Filed the gyp issues without XCode on MacOS to the upstream in https://code.google.com/p/gyp/issues/detail?id=477

bnoordhuis · 2015-02-06T11:45:46Z

I thought that @indutny's gyp patch addressed that? Or is that something else?

shigeki · 2015-02-06T15:09:54Z

The bug seems to be introduced in https://code.google.com/p/gyp/source/detail?r=1863 which was about 3 months later from the last @indutny 's gyp commit of a05dae2 . I think this is a new one.

indutny · 2015-02-09T20:36:51Z

@bnoordhuis should we land it?

shigeki · 2015-02-10T00:37:38Z

@bnoordhuis @indutny No reply from upstream until now. I've just made 1ee5b6d for a tentative fix of gyp. How is this?

jbergstroem · 2015-03-01T13:00:15Z

Can we get a new run on jenkins please?

Fishrock123 · 2015-03-01T15:47:35Z

New CI run: https://jenkins-iojs.nodesource.com/view/iojs/job/iojs+any-pr+multi/222

rvagg · 2015-03-01T18:01:23Z

don't trust the windows results on run 222, this PR needs to be rebased against v1.x to get the vcbuild.bat updates to make that run the tests properly.

`therefor` is a typo of `therefore`, and was fixed. There were also two places where the website WG was directly linked, where they should have put the WG's name/repo; that was fixed as well. PR-URL: nodejs#1022 Reviewed-By: Mikeal Rogers <[email protected]> Reviewed-By: Christian Tellnes <[email protected]>

fix nodejs#6859

Updated gyp has "else if" syntax in condition. Use this for target_arch and OS switches. Several defines, rules and libraries variables moved to openssl.gypi

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.

shigeki · 2015-03-02T01:40:53Z

@rvagg I've just rebased it. Could you please run tests again?

rvagg · 2015-03-02T01:43:17Z

@shigeki I was tinkering earlier today and I think it'll need more advanced work for armv8, I can build it if I run a ./config so I'll try and diff the config that it makes.

shigeki · 2015-03-02T02:50:23Z

@rvagg Yes, there seems to have an issue of host_arch_cc() on armv8. I'm not sure it is related that I also found is_arch_armv7() does not work properly on armv7l of my RasberryPi2. Its arm_version is 6 as below.

ohtsu@raspberrypi:~/github/shigeki/io.js$ uname -a
Linux raspberrypi 3.18.7-v7+ #755 SMP PREEMPT Thu Feb 12 17:20:48 GMT 2015 armv7l GNU/Linux
ohtsu@raspberrypi:~/github/shigeki/io.js$ gcc --version
gcc (Raspbian 4.8.3-13) 4.8.3
Copyright (C) 2013 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

ohtsu@raspberrypi:~/github/shigeki/io.js$ gcc -dM -E -x c /dev/null |grep ARM_ARCH
#define __ARM_ARCH_ISA_ARM 1
#define __ARM_ARCH_6__ 1
#define __ARM_ARCH_ISA_THUMB 1
#define __ARM_ARCH 6

ohtsu@raspberrypi:~/github/shigeki/io.js$ ./configure
creating  ./icu_config.gypi
...
  'variables': { 'arm_float_abi': 'hard',
                 'arm_fpu': 'vfpv2',
                 'arm_thumb': 0,
                 'arm_version': '6',
                 'host_arch': 'arm',

rvagg · 2015-03-02T03:21:52Z

Yeah, configure needs some work, it's all aarch64 on armv8 now. Slowly shaving yaks on this but making progress.

rvagg · 2015-03-02T04:11:01Z

@shigeki does this help at all? https://gist.github.com/rvagg/3ab9a1a7e16b576caef5 the Makefile and opensslconf.h created by ./config on both ARMv7 and ARMv8.

Currently we only have target_arch='arm' being used in openssl.gyp, we probably need to use arm_version too. An alternative I'm toying with is target_arch='aarch64' since this is essentially a different platform and even OpenSSL treats it that way.

This currently is my diff for configure:

diff --git a/configure b/configure
index d632326..9f237ff 100755
--- a/configure
+++ b/configure
@@ -403,6 +403,11 @@ def cc_macros():
       k[key] = val
   return k

+def is_arch_arm64():
+  """Check for ARMv8-64 instructions"""
+  cc_macros_cache = cc_macros()
+  return ('__aarch64__' in cc_macros_cache)
+

 def is_arch_armv7():
   """Check for ARMv7 instructions"""
@@ -439,6 +444,7 @@ def host_arch_cc():
     '__x86_64__'  : 'x64',
     '__i386__'    : 'ia32',
     '__arm__'     : 'arm',
+    '__aarch64__' : 'arm',
     '__mips__'    : 'mips',
   }

@@ -476,7 +482,11 @@ def configure_arm(o):
   else:
     arm_float_abi = 'default'

-  if is_arch_armv7():
+  if is_arch_arm64():
+    o['variables']['arm_fpu'] = 'vfpv4'
+    o['variables']['arm_version'] = '8'
+  elif is_arch_armv7():
     o['variables']['arm_fpu'] = 'vfpv3'
     o['variables']['arm_version'] = '7'
   else:

shigeki · 2015-03-02T04:42:11Z

@rvagg Thanks. Seeing your gist, I'm very surprised at OpenSSL in armv8 was configured as linux-aarch64 which generates new asm files for only armv8. Can I login to the armv8 host with https://github.com/shigeki.keys ? I'd like to make a new asm/Makefile and a target_arch in openssl.gyp and test it on the real machine.

rvagg · 2015-03-02T04:55:04Z

@shigeki not so simple because it's behind a jump-host on the Linaro cluster. I might be able to set up a tunnel for you to work with but I'd better check out the Linaro terms before I go doing that! Leave it with me, in the mean time feel free to make branches on your io.js fork and point me to them to try out.

shigeki · 2015-03-02T05:03:26Z

@rvagg Okay, I'll make an another branch to work on armv8.

@rvagg

This is a patch from @rvagg in nodejs#723 (comment)

shigeki · 2015-03-03T00:37:49Z

In #1028 , a new target-cpu of arm64 was introduced forcing without-ssl option.

jbergstroem · 2015-03-03T00:40:12Z

@shigeki wouldn't this branch patch that away once it's working as intended?

shigeki · 2015-03-03T00:51:52Z

@jbergstroem Sure. Closing this for now and I will submit a new PR.

jbergstroem · 2015-03-03T01:50:18Z

Just to be clear, I didn't mean that you had to close the issue - just that openssl support for that architecture was disabled because it was broken, and the fact that your patch would make it work again.

shigeki · 2015-03-03T02:03:36Z

@jbergstroem Thanks for clarification. I will create an another branch and submit a new PR after solving of gyp bug and arm64 support because there are too much rebases now.

shigeki · 2015-03-03T02:04:57Z

My gyp fix was also asked to the gyp-developer mailing list in https://groups.google.com/d/msg/gyp-developer/G3pVpn7lDpo/W2H87C9IwksJ

shigeki · 2015-03-03T08:46:50Z

A new branch is https://github.com/shigeki/io.js/tree/upgrade_gyp_refactor_openssl_gyp with supporting arm64 with no-asm openssl-1.0.1. PR will be submitted after resolving spawnSync issue on arm64.

jbergstroem mentioned this pull request Feb 6, 2015

Upgrade to OpenSSL-1.0.2 #589

Closed

shigeki force-pushed the openssl_gyp branch from 50eeaa6 to aa3f986 Compare February 10, 2015 00:16

shigeki force-pushed the openssl_gyp branch from be8b968 to f4c8127 Compare February 25, 2015 15:33

TierneyC and others added 6 commits March 2, 2015 01:42

tools: upgrade gyp to 67000714

046855e

gyp: fix build with python 2.6

17df96d

fix nodejs#6859

deps: mv openssl sources in gyp to openssl.gypi

4db6cf3

deps: refactor openssl.gyp

3aec0eb

Updated gyp has "else if" syntax in condition. Use this for target_arch and OS switches. Several defines, rules and libraries variables moved to openssl.gypi

tools: fix gyp to work MacOSX without XCode

e9b5f84

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.

shigeki force-pushed the openssl_gyp branch from f4c8127 to e9b5f84 Compare March 2, 2015 01:36

rvagg force-pushed the v1.x branch from 21afb23 to dfe7a17 Compare March 2, 2015 01:40

shigeki pushed a commit to shigeki/node that referenced this pull request Mar 2, 2015

build: fix configure for armv8

fbbf691

This is a patch from @rvagg in nodejs#723 (comment)

shigeki closed this Mar 3, 2015

shigeki mentioned this pull request Apr 2, 2015

Upgrade to openssl-1.0.2a (Part1) #1325

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

upgrade gyp and refactor openssl.gyp #723

upgrade gyp and refactor openssl.gyp #723

shigeki commented Feb 5, 2015

bnoordhuis commented Feb 5, 2015

indutny commented Feb 5, 2015

Fishrock123 commented Feb 5, 2015

shigeki commented Feb 6, 2015

shigeki commented Feb 6, 2015

bnoordhuis commented Feb 6, 2015

shigeki commented Feb 6, 2015

indutny commented Feb 9, 2015

shigeki commented Feb 10, 2015

jbergstroem commented Mar 1, 2015

Fishrock123 commented Mar 1, 2015

rvagg commented Mar 1, 2015

shigeki commented Mar 2, 2015

rvagg commented Mar 2, 2015

shigeki commented Mar 2, 2015

rvagg commented Mar 2, 2015

rvagg commented Mar 2, 2015

shigeki commented Mar 2, 2015

rvagg commented Mar 2, 2015

shigeki commented Mar 2, 2015

shigeki commented Mar 3, 2015

jbergstroem commented Mar 3, 2015

shigeki commented Mar 3, 2015

jbergstroem commented Mar 3, 2015

shigeki commented Mar 3, 2015

shigeki commented Mar 3, 2015

shigeki commented Mar 3, 2015

upgrade gyp and refactor openssl.gyp #723

upgrade gyp and refactor openssl.gyp #723

Conversation

shigeki commented Feb 5, 2015

bnoordhuis commented Feb 5, 2015

indutny commented Feb 5, 2015

Fishrock123 commented Feb 5, 2015

shigeki commented Feb 6, 2015

shigeki commented Feb 6, 2015

bnoordhuis commented Feb 6, 2015

shigeki commented Feb 6, 2015

indutny commented Feb 9, 2015

shigeki commented Feb 10, 2015

jbergstroem commented Mar 1, 2015

Fishrock123 commented Mar 1, 2015

rvagg commented Mar 1, 2015

shigeki commented Mar 2, 2015

rvagg commented Mar 2, 2015

shigeki commented Mar 2, 2015

rvagg commented Mar 2, 2015

rvagg commented Mar 2, 2015

shigeki commented Mar 2, 2015

rvagg commented Mar 2, 2015

shigeki commented Mar 2, 2015

shigeki commented Mar 3, 2015

jbergstroem commented Mar 3, 2015

shigeki commented Mar 3, 2015

jbergstroem commented Mar 3, 2015

shigeki commented Mar 3, 2015

shigeki commented Mar 3, 2015

shigeki commented Mar 3, 2015