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

build: add --without-node-code-cache configure option #30647

Closed
wants to merge 3 commits into from

Conversation

joyeecheung
Copy link
Member

@joyeecheung joyeecheung commented Nov 26, 2019

build: add --without-node-code-cache configure option

So that it's possible to build without code cache (in particular,
without building mkcodecache) for testing.

Refs: #28845

build: do not build mksnapshot and mkcodecache for --shared

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.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the build Issues and PRs related to build files or the CI. label Nov 26, 2019
@nodejs-github-bot
Copy link
Collaborator

So that it's possible to build without code cache (in particular,
without building mkcodecache) for testing.
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
Copy link
Member Author

Added another commit to make sure --shared imply --without-node-code-cache and --without-node-snapshot, and fixed the test.

@nodejs-github-bot
Copy link
Collaborator

@davidhouweling
Copy link

davidhouweling commented Nov 26, 2019

I believe the vcbuild.bat needs to be updated to reflect the new flag. Refer to #28845 (comment) for more info.

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.

LGTM % style nits.

configure.py Outdated Show resolved Hide resolved
Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

Looks good!

@joyeecheung
Copy link
Member Author

joyeecheung commented Nov 26, 2019

I believe the vcbuild.bat needs to be updated to reflect the new flag. Refer to #28845 (comment) for more info.

vcbuild.bat dll uses --shared which should just imply the new flag with this PR?

@nodejs-github-bot
Copy link
Collaborator

joyeecheung added a commit that referenced this pull request Dec 3, 2019
So that it's possible to build without code cache (in particular,
without building mkcodecache) for testing.

PR-URL: #30647
Refs: #28845
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: David Carlier <[email protected]>
Reviewed-By: James M Snell <[email protected]>
joyeecheung added a commit that referenced this pull request Dec 3, 2019
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 <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: David Carlier <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@joyeecheung
Copy link
Member Author

Landed in cc3f2b3...8cf8eb1

@joyeecheung joyeecheung closed this Dec 3, 2019
BridgeAR pushed a commit that referenced this pull request Dec 3, 2019
So that it's possible to build without code cache (in particular,
without building mkcodecache) for testing.

PR-URL: #30647
Refs: #28845
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: David Carlier <[email protected]>
Reviewed-By: James M Snell <[email protected]>
BridgeAR pushed a commit that referenced this pull request Dec 3, 2019
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 <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: David Carlier <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@BridgeAR BridgeAR mentioned this pull request Dec 3, 2019
targos pushed a commit that referenced this pull request Jan 14, 2020
So that it's possible to build without code cache (in particular,
without building mkcodecache) for testing.

PR-URL: #30647
Refs: #28845
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: David Carlier <[email protected]>
Reviewed-By: James M Snell <[email protected]>
targos pushed a commit that referenced this pull request Jan 14, 2020
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 <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: David Carlier <[email protected]>
Reviewed-By: James M Snell <[email protected]>
BethGriggs pushed a commit that referenced this pull request Feb 6, 2020
So that it's possible to build without code cache (in particular,
without building mkcodecache) for testing.

PR-URL: #30647
Refs: #28845
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: David Carlier <[email protected]>
Reviewed-By: James M Snell <[email protected]>
BethGriggs pushed a commit that referenced this pull request Feb 6, 2020
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 <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: David Carlier <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Feb 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants