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

node: fix host build fail #6015

Merged
merged 1 commit into from
May 16, 2018
Merged

node: fix host build fail #6015

merged 1 commit into from
May 16, 2018

Conversation

nxhack
Copy link
Contributor

@nxhack nxhack commented May 7, 2018

Maintainer: @blogic , @ianchi and @ratkaj
Compile tested: ar71xx, LEDE trunk r6835-e495a05 (w fpu emu)

Run tested: NONE

Description:
fix host build failure and made not to use libressl headers

nodejs/node#19196
#5734

Signed-off-by: Hirokazu MORIKAWA morikw2@gmail.com

@ratkaj
Copy link
Contributor

ratkaj commented May 9, 2018

@nxhack I tested this patch on both Ubuntu and Arch, both normal and travis builds and there were no failures. Travis fail might have been caused by DNS issues with git.openwrt.org at the time you created the pull request.

This also fixes error "invalid conversion from" in node_crypto.cc that I was having last 2 days. (host-compile phase)

Try to refresh the pull request and it might pass this time, if it does not timeout...

@nxhack
Copy link
Contributor Author

nxhack commented May 9, 2018

Since travis CI uses the ar71xx SDK, node.js is out of build target. (FPU issue)
Of course, In my environment, I build it with kernel fpu emulation and successfully build.

@ratkaj
Copy link
Contributor

ratkaj commented May 10, 2018

@nxhack i did try builing localy using Travis build scripts and openwrt-sdk-ar71xx-generic_gcc-7.3.0_musl.Linux-x86_64 toolchain that it uses by default.

I did so by running the same scripts that Travis does (inside the packages feed):
./.travis_do.sh download_sdk
./.travis_do.sh test_packages

I doubt that Travis does anything more than this, but correct me if i am wrong, i would like to understand the whole process. Node package also has a mips check. And it compiles with "--with-mips-float-abi=soft" so there should be no failures.

Note that you can modify the .travis_do.sh script and add V=s to build. This is disabled because Travis server can't handle all of the output.
Thanks for this pull request.

@ratkaj
Copy link
Contributor

ratkaj commented May 12, 2018

Tnx nxhack.

Anyone against merging this?

@yousong
Copy link
Member

yousong commented May 13, 2018

Hi, @nxhack , if it's related to systemtap or dtrace, can we just disable it with --without-dtrace?

@@ -129,6 +129,7 @@ HOST_CONFIGURE_ARGS:= \
--dest-os=linux \
--without-snapshot \
--shared-zlib \
--systemtap-includes=$(HOST_BUILD_DIR)/deps/openssl/openssl/include \
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The configure option should really be --shared-openssl-includes?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In node.js' host build we use bundle openssl instead of shared openssl.
Specifying "--shared-openssl-includes” option is ineffective useing bundle openssl.

Host build of node.js now fails after the version of libressl updated on current openwrt.

In order to avoid it, I used a dirty method.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nxhack , please document the intent either in the commit message or properly in the Makefile. Thank you.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As you point out, it is a confusing, bad workaround.

  • When building bundles openssl of node.js, it fails by referring to the header of libressl.

  • "configure script" of node.js generates config.gyp.

                       'include_dirs': [ '/home/ubuntu/Trunk/openwrt/staging_dir/host/include'],
                       'libraries': [ '-L/home/ubuntu/Trunk/openwrt/staging_dir/host/lib',
                                      '-lz']},

I want to change include_dirs.
I will think again whether there is a good hack.

Copy link
Contributor Author

@nxhack nxhack May 16, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK I found, pkg-config was set to build shared zlib. As a workaround, I will change zlib to use bundle version as well.

modify patch.
 nodejs/node#19196

made not to use libressl headers
 fix to include path not to use "host/include"

Signed-off-by: Hirokazu MORIKAWA <morikw2@gmail.com>
@nxhack
Copy link
Contributor Author

nxhack commented May 16, 2018

It became a simple hack. ;)

@yousong yousong merged commit 818770d into openwrt:master May 16, 2018
@yousong
Copy link
Member

yousong commented May 16, 2018

Thank you for the time and efforts!

@nxhack nxhack deleted the fix_host_build branch May 16, 2018 03:49
Sign up for free to 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.

3 participants