Skip to content

Conversation

@shigeki
Copy link
Contributor

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

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

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

@Fishrock123
Copy link
Contributor

armv6/7 and freebsd failures appear unrelated.

@shigeki
Copy link
ContributorAuthor

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

@jbergstroemjbergstroem mentioned this pull request Feb 6, 2015
@shigeki
Copy link
ContributorAuthor

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

@bnoordhuis
Copy link
Member

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

@shigeki
Copy link
ContributorAuthor

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

@bnoordhuis should we land it?

@shigeki
Copy link
ContributorAuthor

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

@jbergstroem
Copy link
Member

Can we get a new run on jenkins please?

@Fishrock123
Copy link
Contributor

@rvagg
Copy link
Member

rvagg commented Mar 1, 2015

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.

TierneyCand others added 6 commits March 2, 2015 01:42
`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]>
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
Copy link
ContributorAuthor

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

@rvagg
Copy link
Member

rvagg commented Mar 2, 2015

@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
Copy link
ContributorAuthor

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

rvagg commented Mar 2, 2015

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

@rvagg
Copy link
Member

rvagg commented Mar 2, 2015

@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
Copy link
ContributorAuthor

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

rvagg commented Mar 2, 2015

@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
Copy link
ContributorAuthor

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

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

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

@jbergstroem
Copy link
Member

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

@shigeki
Copy link
ContributorAuthor

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

@shigekishigeki closed this Mar 3, 2015
@jbergstroem
Copy link
Member

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

@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
Copy link
ContributorAuthor

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

@shigeki
Copy link
ContributorAuthor

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.

Sign up for freeto 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.

7 participants

@shigeki@bnoordhuis@indutny@Fishrock123@jbergstroem@rvagg@TierneyC