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.

Sign up for GitHub

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

Jump to bottom

[v6.x backport] build: refine static and shared lib build #19050

Closed
wants to merge 1 commit into from
Closed

[v6.x backport] build: refine static and shared lib build #19050

wants to merge 1 commit into from

Conversation

yhwang
Copy link
Member

@yhwang yhwang commented Feb 28, 2018

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)

@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. v6.x labels Feb 28, 2018
@yhwang
Copy link
Member Author

yhwang commented Feb 28, 2018

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

mihai-iorga
mihai-iorga reviewed Feb 28, 2018
View reviewed changes
configure
@@ -942,6 +941,13 @@ def configure_node(o):
else:
o['variables']['coverage'] = 'false'

if options.shared:
o['variables']['node_target_type'] = 'shared_library'
elif options.enable_static:
Copy link

Choose a reason for hiding this comment

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

elseif?

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link

Choose a reason for hiding this comment

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

oh .. sorry :)

@yhwang
Copy link
Member Author

yhwang commented Feb 28, 2018

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

@yhwang yhwang force-pushed the backport-17604-to-v6.x-staging branch from 3430575 to edf0287 Compare March 1, 2018 00:02
@yhwang
Copy link
Member Author

yhwang commented Mar 1, 2018

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

@yhwang
Copy link
Member Author

yhwang commented Mar 2, 2018 •
edited
Loading

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 yhwang force-pushed the backport-17604-to-v6.x-staging branch from edf0287 to 9cf436b Compare March 2, 2018 00:38
@yhwang
Copy link
Member Author

yhwang commented Mar 2, 2018

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

@yhwang yhwang force-pushed the backport-17604-to-v6.x-staging branch from 9cf436b to ee6b76f Compare March 2, 2018 21:10
@yhwang
build: refine static and shared lib build
50754c3 
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 yhwang force-pushed the backport-17604-to-v6.x-staging branch from ee6b76f to 50754c3 Compare March 2, 2018 21:31
@yhwang
Copy link
Member Author

yhwang commented Mar 2, 2018

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

@yhwang
Copy link
Member Author

yhwang commented Mar 3, 2018

only failed on arm! the PR is good now.

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

yhwang commented Mar 13, 2018

@bnoordhuis thanks for the review

@yhwang
Copy link
Member Author

yhwang commented Mar 16, 2018 •
edited
Loading

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

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

yhwang commented Mar 16, 2018

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

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

yhwang commented Mar 20, 2018

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

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

yhwang commented Mar 20, 2018

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.

MylesBorins pushed a commit that referenced this pull request Mar 21, 2018
@yhwang @MylesBorins
build: refine static and shared lib build
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>
@MylesBorins
Copy link
Contributor

landed in 8427ec6

@yhwang yhwang deleted the backport-17604-to-v6.x-staging branch March 21, 2018 16:21
MylesBorins pushed a commit that referenced this pull request Mar 28, 2018
@yhwang @MylesBorins
build: refine static and shared lib build
ddcfffb 
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 pushed a commit that referenced this pull request Mar 30, 2018
@yhwang @MylesBorins
build: refine static and shared lib build
49d8c2e 
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>
@mhdawson mhdawson mentioned this pull request Apr 12, 2018
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mihai-iorga mihai-iorga mihai-iorga left review comments

@bnoordhuis bnoordhuis bnoordhuis approved these changes

Assignees

@MylesBorins MylesBorins

Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. build Issues and PRs related to build files or the CI.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@yhwang @MylesBorins @bnoordhuis @mihai-iorga @nodejs-github-bot