Skip to content

build: include the libuv and zlib into node - #18383

Closed
yhwang wants to merge 1 commit into
nodejs:masterfrom
yhwang:whole-archive-libuv
Closed

build: include the libuv and zlib into node#18383
yhwang wants to merge 1 commit into
nodejs:masterfrom
yhwang:whole-archive-libuv

Conversation

@yhwang

Copy link
Copy Markdown
Member

Add libuv and zlib into node executable and shared lib. Also fix an
issue that openssl is not fully included in node executable for macOS.

fixes#17444

Signed-off-by: Yihong Wang yh.wang@ibm.com

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

build

@nodejs-github-botnodejs-github-bot added the build Issues and PRs related to build files or the CI. label Jan 25, 2018

@addaleaxaddaleax left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@yhwang

Copy link
Copy Markdown
MemberAuthor

This test case failed on Windows: parallel/test-tls-server-verify. I saw someone is fixing it now. Not related to my change.

@yhwangyhwang changed the title Include the libuv and zlib into nodebuild: include the libuv and zlib into nodeJan 26, 2018
Comment threadnode.gypi Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

ldflags applies only to platforms that aren't Windows or MacOS so this is better written as OS != "aix", which I think the intent was? Likewise on line 165.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

yes and changed.

Comment threadnode.gypi Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can you indent here? It's a line (and string) continuation.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

good catch and done

@yhwang
yhwangforce-pushed the whole-archive-libuv branch from a95449d to 44dbb2dCompareJanuary 26, 2018 19:00
@yhwang

Copy link
Copy Markdown
MemberAuthor

updated the change based on @bnoordhuis comments

@bnoordhuisbnoordhuis left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@bnoordhuisbnoordhuis added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 29, 2018
@yhwang

Copy link
Copy Markdown
MemberAuthor

Looks like the error of AIX shows up in other PR. is the AIX env for CI broken?

@bnoordhuis

Copy link
Copy Markdown
Member

Infrastructure issue. Let's try it one more time for good luck: https://ci.nodejs.org/job/node-test-commit/15775/

There's also this but I can't say if it's caused by this PR:

Building addon /home/iojs/build/workspace/node-test-commit-linux/nodes/debian8-64/test/addons-napi/test_make_callback/
gyp info it worked if it ends with ok
gyp info using node-gyp@3.6.2
gyp info using node@10.0.0-pre | linux | x64
gyp info chdir /home/iojs/build/workspace/node-test-commit-linux/nodes/debian8-64/test/addons/heap-profiler/
Aborted
Makefile:326: recipe for target 'test/addons/.buildstamp' failed
make[1]: *** [test/addons/.buildstamp] Error 1

@bnoordhuis

Copy link
Copy Markdown
Member

Not a happy run either... the alpine buildbot failure is most likely a flaky test but I don't immediately see why the linuxone run failed and the aix buildbot still has infrastructure issues.

cc @nodejs/build - known issue?

@yhwang

yhwang commented Jan 30, 2018

Copy link
Copy Markdown
MemberAuthor

the weird thing is I can't tell the root cause from the console log (the linuxone)

@mhdawsonmhdawson left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@mhdawson

Copy link
Copy Markdown
Member

@mhdawson

Copy link
Copy Markdown
Member

This is another CI run, https://ci.nodejs.org/job/node-test-commit/15831/

@mhdawson

Copy link
Copy Markdown
Member

In the last CI run there were failures on debian, but this unrelated PR has the excact same failures: https://ci.nodejs.org/job/node-test-commit-linux/15991/nodes=debian8-64/console. That and the fact that some earlier CI runs for this PR did not have failures on debian mean that I believe the CI run is good.

@BridgeARBridgeAR removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 1, 2018
@BridgeAR

Copy link
Copy Markdown
Member

@yhwang would you be so kind and rebase? :-)

@yhwang
yhwangforce-pushed the whole-archive-libuv branch from 44dbb2d to cae3458CompareFebruary 1, 2018 17:34
@yhwang

Copy link
Copy Markdown
MemberAuthor

@BridgeAR thanks for the reminding. rebase is done. Please kick off a CI.

@yhwang
yhwangforce-pushed the whole-archive-libuv branch from cae3458 to 90d5c48CompareFebruary 1, 2018 18:18
Add libuv and zlib into node executable and shared lib. Also fix an
issue that openssl is not fully included in node executable for macOS.
Fixes: nodejs#17444
Signed-off-by: Yihong Wang <yh.wang@ibm.com>
@yhwang
yhwangforce-pushed the whole-archive-libuv branch from 90d5c48 to 3a24b71CompareFebruary 1, 2018 18:20
@mhdawson

Copy link
Copy Markdown
Member

@BridgeARBridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 1, 2018
@yhwang

Copy link
Copy Markdown
MemberAuthor

one failure: test-http-pipeline-flood. It's a flaky test case.

@addaleax

Copy link
Copy Markdown
Member

Landed in d161625

@addaleaxaddaleax closed this Feb 4, 2018
addaleax pushed a commit that referenced this pull request Feb 4, 2018
Add libuv and zlib into node executable and shared lib. Also fix an
issue that openssl is not fully included in node executable for macOS.
Signed-off-by: Yihong Wang <yh.wang@ibm.com>
Fixes: #17444
PR-URL: #18383
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@addaleaxaddaleax removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 4, 2018
@yhwang
yhwang deleted the whole-archive-libuv branch February 5, 2018 17:26
@MylesBorins

Copy link
Copy Markdown
Contributor

Should this be backported to v9.x-staging? If yes please follow the guide and raise a backport PR, if not let me know or add the dont-land-on label.

yhwang added a commit to yhwang/node that referenced this pull request Feb 22, 2018
Add libuv and zlib into node executable and shared lib. Also fix an
issue that openssl is not fully included in node executable for macOS.
Signed-off-by: Yihong Wang <yh.wang@ibm.com>
Fixes: nodejs#17444
PR-URL: nodejs#18383
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax pushed a commit that referenced this pull request Feb 26, 2018
Add libuv and zlib into node executable and shared lib. Also fix an
issue that openssl is not fully included in node executable for macOS.
Signed-off-by: Yihong Wang <yh.wang@ibm.com>
Fixes: #17444
PR-URL: #18383
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax pushed a commit that referenced this pull request Feb 26, 2018
Add libuv and zlib into node executable and shared lib. Also fix an
issue that openssl is not fully included in node executable for macOS.
Signed-off-by: Yihong Wang <yh.wang@ibm.com>
Fixes: #17444
PR-URL: #18383
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@addaleaxaddaleax mentioned this pull request Feb 27, 2018
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
Add libuv and zlib into node executable and shared lib. Also fix an
issue that openssl is not fully included in node executable for macOS.
Signed-off-by: Yihong Wang <yh.wang@ibm.com>
Fixes: nodejs#17444
PR-URL: nodejs#18383
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@addaleax

Copy link
Copy Markdown
Member

I’ve landed this as 4f21b66 in v8.x-staging because, as I understand it, it applies there as well – let me know if that was a mistake.

addaleax pushed a commit that referenced this pull request Jun 29, 2018
Add libuv and zlib into node executable and shared lib. Also fix an
issue that openssl is not fully included in node executable for macOS.
Signed-off-by: Yihong Wang <yh.wang@ibm.com>
Fixes: #17444
PR-URL: #18383
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorinsMylesBorins mentioned this pull request Jul 9, 2018
rvagg pushed a commit that referenced this pull request Aug 16, 2018
Add libuv and zlib into node executable and shared lib. Also fix an
issue that openssl is not fully included in node executable for macOS.
Signed-off-by: Yihong Wang <yh.wang@ibm.com>
Fixes: #17444
PR-URL: #18383
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

buildIssues and PRs related to build files or the CI.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Link libuv & zlib with --whole-archive

9 participants

@yhwang@bnoordhuis@mhdawson@BridgeAR@addaleax@MylesBorins@jasnell@gireeshpunathil@nodejs-github-bot