[v6.x backport] build: refine static and shared lib build by yhwang · Pull Request #19050 · nodejs/node

yhwang · 2018-02-28T02:30:15Z

Refine the static and shared lib build process in order
to integrate static and shared lib verfication into CI.
When building both static and shared lib, we still build
node executable now and it uses the shared and static lib.

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

Refs: #14158
PR-URL: #17604
Reviewed-By: Bartosz Sosnowski bartosz@janeasystems.com
Reviewed-By: Ben Noordhuis info@bnoordhuis.nl
Reviewed-By: Daniel Bevenius daniel.bevenius@gmail.com

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

Affected core subsystem(s)

yhwang · 2018-02-28T02:32:47Z

CI: https://ci.nodejs.org/job/node-test-commit/16546/

mihai-iorga · 2018-02-28T12:18:27Z

@mihai-iorganode_target_type is used in line 1446 below. in here it's just to assign correct value. can you elaborate more about your comment?

oh .. sorry :)

yhwang · 2018-02-28T17:34:23Z

looks like the backport has some difficulties. let me try to fix those build failures in windows and osx.

yhwang · 2018-03-01T06:13:05Z

kicked off another CI to verify Windows/macOS build: https://ci.nodejs.org/job/node-test-commit/16566/

yhwang · 2018-03-02T00:16:32Z

those build failures are weird, since the node_lib target depends on deps/v8_inspector/third_party/v8_inspector/platform/v8_inspector/v8_inspector.gyp:v8_inspector_stl target, the InspectorProtocol.h should be generated. Looks like that v8_inspector_stl target is not triggered. need to figure out why it's not triggered on those platforms.

Let me do one experiment on the v8_inspector.gyp:protocol_sources_stl target and see if it build the header files. CI: https://ci.nodejs.org/job/node-test-commit/16583/

yhwang · 2018-03-02T01:21:44Z

yeah! seems the trick of v8_inspector.gyp:protocol_sources_stl solved issues on some platforms. Next step is to fix the windows failures.

Refine the static and shared lib build process in order to integrate static and shared lib verfication into CI. When building both static and shared lib, we still build node executable now and it uses the shared and static lib. Signed-off-by: Yihong Wang <yh.wang@ibm.com> Refs: nodejs#14158 PR-URL: nodejs#17604 Reviewed-By: Bartosz Sosnowski <bartosz@janeasystems.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>

yhwang · 2018-03-02T22:03:17Z

new CI: https://ci.nodejs.org/job/node-test-commit/16621/ to verify windows build

yhwang · 2018-03-03T00:16:50Z

only failed on arm! the PR is good now.

yhwang · 2018-03-13T02:23:09Z

@bnoordhuis thanks for the review

yhwang · 2018-03-16T00:48:36Z

I am going to land this and before that, let me kick off a CI to check the PR again: https://ci.nodejs.org/job/node-test-commit/16941/

@MylesBorins please let me know if you have any concern or if you want to land it.

MylesBorins · 2018-03-16T15:45:30Z

@yhwang please hold off on landing. Our LTS branches our supposed to only have the LTS team land commits on. The only reason it isn't currently locked is due to the permission model of GitHub

I'm on vacation the rest of this week, but I'll make sure to review and land before the next rc goes out

yhwang · 2018-03-16T21:59:01Z

@MylesBorins I will leave it to you and no rush.

FYI, the failure in the node-test-linux-linked-withoutintl depends on f8063d5 . It's not in v6.x-staging yet.

MylesBorins · 2018-03-20T01:19:34Z

@yhwang looks like there is a centos 5 failure as well... which might be a flake. One thing worth noting... the --without-icu stuff is fixing code that was added in a semver major for 9.x, so I'm not 100% it is the same thing

yhwang · 2018-03-20T01:39:46Z

@MylesBorins thanks for look at this one.

One thing worth noting... the --without-icu stuff is fixing code that was added in a semver major for 9.x

are you talking about the failures in node-test-linux-linked-withoutintl? I think the failures are the same as the issue you open here: nodejs/build#1182

20:54:11 not ok 119 parallel/test-cli-node-options
20:54:11 not ok 1311 inspector/test-inspector
20:54:11 not ok 1372 sequential/test-cluster-inspect-brk

MylesBorins · 2018-03-20T14:42:37Z

@yhwang there was another failure... but ci results are now gone. Running one more time

CI: https://ci.nodejs.org/job/node-test-pull-request/13769/

yhwang · 2018-03-20T18:28:53Z

here are the failed test cases:

10:52:29 not ok 118 parallel/test-cli-node-options
10:52:29 not ok 1312 inspector/test-inspector
10:52:29 not ok 1373 sequential/test-cluster-inspect-brk

still the same as the failures you observed in nodejs/build#1182
At least those failures are not caused by the changes in this PR.

Refine the static and shared lib build process in order to integrate static and shared lib verfication into CI. When building both static and shared lib, we still build node executable now and it uses the shared and static lib. Signed-off-by: Yihong Wang <yh.wang@ibm.com> Refs: #14158 Backport-PR-URL: #19050 PR-URL: #17604 Reviewed-By: Bartosz Sosnowski <bartosz@janeasystems.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>

MylesBorins · 2018-03-21T00:42:11Z

landed in 8427ec6

Refine the static and shared lib build process in order to integrate static and shared lib verfication into CI. When building both static and shared lib, we still build node executable now and it uses the shared and static lib. Signed-off-by: Yihong Wang <yh.wang@ibm.com> Refs: #14158 Backport-PR-URL: #19050 PR-URL: #17604 Reviewed-By: Bartosz Sosnowski <bartosz@janeasystems.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>

nodejs-github-bot added build Issues and PRs related to build files or the CI. v6.x labels Feb 28, 2018

mihai-iorga reviewed Feb 28, 2018
View reviewed changes

yhwang force-pushed the backport-17604-to-v6.x-staging branch from 3430575 to edf0287Compare March 1, 2018 00:02

yhwang force-pushed the backport-17604-to-v6.x-staging branch from edf0287 to 9cf436bCompare March 2, 2018 00:38

gibfahn assigned MylesBorins Mar 2, 2018

yhwang force-pushed the backport-17604-to-v6.x-staging branch from 9cf436b to ee6b76fCompare March 2, 2018 21:10

yhwang force-pushed the backport-17604-to-v6.x-staging branch from ee6b76f to 50754c3Compare March 2, 2018 21:31

bnoordhuis approved these changes Mar 12, 2018
View reviewed changes

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

MylesBorins closed this Mar 21, 2018

yhwang deleted the backport-17604-to-v6.x-staging branch March 21, 2018 16:21

mhdawson mentioned this pull request Apr 12, 2018
Form team/group to define "embedding requirements" nodejs/user-feedback#51
Open

Uh oh!

Uh oh!

Conversation

yhwang commented Feb 28, 2018

Checklist

Affected core subsystem(s)

Uh oh!

yhwang commented Feb 28, 2018

Uh oh!

mihai-iorgaFeb 28, 2018

Choose a reason for hiding this comment

Uh oh!

yhwangFeb 28, 2018

Choose a reason for hiding this comment

Uh oh!

mihai-iorgaMar 1, 2018

Choose a reason for hiding this comment

Uh oh!

yhwang commented Feb 28, 2018

Uh oh!

yhwang commented Mar 1, 2018

Uh oh!

yhwang commented Mar 2, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yhwang commented Mar 2, 2018

Uh oh!

yhwang commented Mar 2, 2018

Uh oh!

yhwang commented Mar 3, 2018

Uh oh!

yhwang commented Mar 13, 2018

Uh oh!

yhwang commented Mar 16, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MylesBorins commented Mar 16, 2018

Uh oh!

yhwang commented Mar 16, 2018

Uh oh!

MylesBorins commented Mar 20, 2018

Uh oh!

yhwang commented Mar 20, 2018

Uh oh!

MylesBorins commented Mar 20, 2018

Uh oh!

yhwang commented Mar 20, 2018

Uh oh!

MylesBorins commented Mar 21, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

yhwang commented Mar 2, 2018•
edited
Loading

yhwang commented Mar 16, 2018•
edited
Loading