-
Notifications
You must be signed in to change notification settings - Fork 112
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
Support build with openssl-1.1.x #433
Support build with openssl-1.1.x #433
Conversation
@takumakume Thank you for your PR. These patches almost look good to me. Could you try to build on Vagrant using ngx_mruby vagrant environment? |
Thank you for check! I confirmed that the test passed with vagrant. commit: dd51be5 vagrant@vagrant:~/ngx_mruby$ sh test.sh
:
: (snip)
:
Total: 92
OK: 92
KO: 0
Crash: 0
Time: 17.225444 seconds
"[28403] exit worker process from inline code"
"ngx_mruby: STREAM: mruby_stream_exit_worker_code"
ngx_mruby testing ... Done
test.sh ... successful
vagrant@vagrant:~/ngx_mruby$ ./build/nginx/sbin/nginx -V
nginx version: nginx/1.17.4
built by gcc 4.9.3 (Ubuntu 4.9.3-13ubuntu2)
built with OpenSSL 1.1.1 11 Sep 2018
TLS SNI support enabled
configure arguments: --add-module=/home/vagrant/ngx_mruby --add-module=/home/vagrant/ngx_mruby/dependence/ngx_devel_kit --prefix=/home/vagrant/ngx_mruby/build/nginx --with-debug --with-http_stub_status_module --with-http_ssl_module --with-cc-opt='-g -O0' --with-stream --without-stream_access_module |
Do we need the new openssl test in vagrant? |
Or ngx_mruby vagrant already supports openssl-1.1.x, right? |
As you said. It may not need for everyone.
The current vagrant image is not supported openssl-1.1.x.
|
If ngx_mruby test supports openssl 1.1.x by default, ngx_mruby vagrant test also should support openssl 1.1.x, or ngx_mruby test supports both openssl 1.0.x and 1.1.x for compatibility. |
@yyamano How do you think? |
I don't use openssl with ngx_mruby, so no strong opinions.
|
I agree. I am difficult to judge. If support openssl-1.0.x, be able to test for openssl-1.0.x using the travis
Yes! I understand. |
@yyamano Thank you for your suggestion. I agree with you. So @takumakume , cloud you implement the vagrant configuration which uses openssl 1.1.x for test as Travis CI? |
Installed same OpenSSL as Travis CI in vagrant:
And test passed in vagrant. commit: 6403fcf |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Logs are very important.
misc/provision.sh
Outdated
tar -xzf openssl-${openssl_version}.tar.gz | ||
rm openssl-${openssl_version}.tar.gz | ||
cd openssl-${openssl_version}* | ||
./config --prefix=/usr/local --shared zlib -fPIC >> /dev/null 2>&1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you remove these logs? These logs are important to investigate the error.
Travis limited the number of stdout line, but the local build is no limitation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand.
Fixed: e88ed19
Thank you for your proposal! Merged this PR now. |
Thank you for check!! |
This PR will be able to build the ngx_mruby with openssl-1.1.x.
enable-tlsext
in openssl-1.1 later)LD
, Required here: https://github.com/mruby/mruby/blob/b7af9c47b3f6ec656f7e2d7ee4c1d11cda667748/mrbgems/mruby-bin-mrbc/mrbgem.rake#L12Dockerfile.travis_emulate
CI using openssl-1.0.x is gone.
I thought about doing both 1.0 and 1.1, but I thought it would be complicated.
Instead,
I performed a test overwriting this fix with 1.0.2. passed it.
takumakume#1
Pull-Request Check List
src/
.test/
. Please see about test docs.docs/
if you change the features such as build system, Ruby methods, class and nginx directives.