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

include stubs in shared library #28897

Closed
wants to merge 1 commit into from
Closed

include stubs in shared library #28897

wants to merge 1 commit into from

Conversation

jeroen
Copy link
Contributor

@jeroen jeroen commented Jul 29, 2019 •
edited
Loading

This is needed for external applications to link to shared libnode. Fixes #27431

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

@refack does this seem OK?

@nodejs-github-bot nodejs-github-bot added the build Issues and PRs related to build files or the CI. label Jul 29, 2019
@jeroen
build: include stubs in shared library
0923f58 
This is needed for external applications that link to shared libnode.
Fixes #27431
@jeroen jeroen marked this pull request as ready for review July 29, 2019 21:07
@Trott
Copy link
Member

Trott commented Aug 1, 2019

@nodejs/node-gyp

@nodejs-github-bot
Copy link
Collaborator

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

@Trott Trott requested a review from refack August 1, 2019 05:48
bnoordhuis
bnoordhuis approved these changes Aug 1, 2019
View reviewed changes
Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM.

Trott
Trott approved these changes Aug 1, 2019
View reviewed changes
Copy link
Member

@Trott Trott left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM. I'm not a gyp expert, but CI is green, got another approval from a domain expert, and this would be easy to revert if it came to it. So 👍 .

@Trott
Copy link
Member

Trott commented Aug 1, 2019

Landed in ed138ba

@Trott Trott closed this Aug 1, 2019
Trott pushed a commit to Trott/io.js that referenced this pull request Aug 1, 2019
@jeroen @Trott
build: include stubs in shared library
ed138ba 
This is needed for external applications that link to shared libnode.
Fixes nodejs#27431

PR-URL: nodejs#28897
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@jeroen jeroen mentioned this pull request Aug 1, 2019
Closed
4 tasks
targos pushed a commit that referenced this pull request Aug 2, 2019
@jeroen @targos
build: include stubs in shared library
dcef7b8 
This is needed for external applications that link to shared libnode.
Fixes #27431

PR-URL: #28897
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@BridgeAR BridgeAR mentioned this pull request Aug 6, 2019
Merged
@joyeecheung joyeecheung mentioned this pull request Nov 26, 2019
Closed
joyeecheung added a commit to joyeecheung/node that referenced this pull request Nov 26, 2019
@joyeecheung
build: do not build mksnapshot and mkcodecache for --shared
609fc75 
To build mkcodecache and mksnapshot (they are executables),
we currently build libnode with unresolved symbols, then build
the two exectuables with src/node_snapshot_stub.cc and
src/node_code_cache_stub.cc. Each of them write a C++ file to
disk when being run. We then use the generated C++ files & libnode
(with unresolved symbols) to build the final Node executable.

However, if libnode itself is the final product, then we should
not build it with unresolved symbols.
nodejs#28897 added the two stubs
for the libnode target when the --shared configure option is used,
but it did not get rid of the actions to build and run mksnapshot
and mkcodecache for --shared, so I think to get it working we also
need a patch to make sure --shared imply --without-node-code-cache
and --without-node-snapshot, until we actually fix the TODO so that
mksnapshot and mkcodecache do not use the libnode that way.
@joyeecheung joyeecheung mentioned this pull request Nov 26, 2019
Closed
4 tasks
joyeecheung added a commit that referenced this pull request Dec 3, 2019
@joyeecheung
build: do not build mksnapshot and mkcodecache for --shared
8cf8eb1 
To build mkcodecache and mksnapshot (they are executables),
we currently build libnode with unresolved symbols, then build
the two exectuables with src/node_snapshot_stub.cc and
src/node_code_cache_stub.cc. Each of them write a C++ file to
disk when being run. We then use the generated C++ files & libnode
(with unresolved symbols) to build the final Node executable.

However, if libnode itself is the final product, then we should
not build it with unresolved symbols.
#28897 added the two stubs
for the libnode target when the --shared configure option is used,
but it did not get rid of the actions to build and run mksnapshot
and mkcodecache for --shared, so to get it working we also
need a patch to make sure --shared imply --without-node-code-cache
and --without-node-snapshot, until we actually fix the TODO so that
mksnapshot and mkcodecache do not use the libnode that way.

PR-URL: #30647
Refs: #28845
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
BridgeAR pushed a commit that referenced this pull request Dec 3, 2019
@joyeecheung @BridgeAR
build: do not build mksnapshot and mkcodecache for --shared
648766b 
To build mkcodecache and mksnapshot (they are executables),
we currently build libnode with unresolved symbols, then build
the two exectuables with src/node_snapshot_stub.cc and
src/node_code_cache_stub.cc. Each of them write a C++ file to
disk when being run. We then use the generated C++ files & libnode
(with unresolved symbols) to build the final Node executable.

However, if libnode itself is the final product, then we should
not build it with unresolved symbols.
#28897 added the two stubs
for the libnode target when the --shared configure option is used,
but it did not get rid of the actions to build and run mksnapshot
and mkcodecache for --shared, so to get it working we also
need a patch to make sure --shared imply --without-node-code-cache
and --without-node-snapshot, until we actually fix the TODO so that
mksnapshot and mkcodecache do not use the libnode that way.

PR-URL: #30647
Refs: #28845
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this pull request Jan 14, 2020
@joyeecheung @targos
build: do not build mksnapshot and mkcodecache for --shared
35ce8bb 
To build mkcodecache and mksnapshot (they are executables),
we currently build libnode with unresolved symbols, then build
the two exectuables with src/node_snapshot_stub.cc and
src/node_code_cache_stub.cc. Each of them write a C++ file to
disk when being run. We then use the generated C++ files & libnode
(with unresolved symbols) to build the final Node executable.

However, if libnode itself is the final product, then we should
not build it with unresolved symbols.
#28897 added the two stubs
for the libnode target when the --shared configure option is used,
but it did not get rid of the actions to build and run mksnapshot
and mkcodecache for --shared, so to get it working we also
need a patch to make sure --shared imply --without-node-code-cache
and --without-node-snapshot, until we actually fix the TODO so that
mksnapshot and mkcodecache do not use the libnode that way.

PR-URL: #30647
Refs: #28845
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
BethGriggs pushed a commit that referenced this pull request Feb 6, 2020
@joyeecheung @BethGriggs
build: do not build mksnapshot and mkcodecache for --shared
ca432d7 
To build mkcodecache and mksnapshot (they are executables),
we currently build libnode with unresolved symbols, then build
the two exectuables with src/node_snapshot_stub.cc and
src/node_code_cache_stub.cc. Each of them write a C++ file to
disk when being run. We then use the generated C++ files & libnode
(with unresolved symbols) to build the final Node executable.

However, if libnode itself is the final product, then we should
not build it with unresolved symbols.
#28897 added the two stubs
for the libnode target when the --shared configure option is used,
but it did not get rid of the actions to build and run mksnapshot
and mkcodecache for --shared, so to get it working we also
need a patch to make sure --shared imply --without-node-code-cache
and --without-node-snapshot, until we actually fix the TODO so that
mksnapshot and mkcodecache do not use the libnode that way.

PR-URL: #30647
Refs: #28845
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Trott Trott Trott approved these changes

@bnoordhuis bnoordhuis bnoordhuis approved these changes

@refack refack Awaiting requested review from refack

Assignees
No one assigned
Labels
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.

Link issue with static library of nodejs
4 participants
@jeroen @Trott @nodejs-github-bot @bnoordhuis