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: gyp node_base #23439

Closed
wants to merge 9 commits into from
Closed

build: gyp node_base #23439

wants to merge 9 commits into from

Conversation

refack
Copy link
Contributor

@refack refack commented Oct 12, 2018

Motivation:

  1. node_base as an independant target, that we can run just to make sure the code in /src/ compiles.
  2. Easy to compose with code_cache_stub.cc to build an mkcodecache exe. Then later relink with the generated `code_cache_gen.cc.
  3. Easier to run "whole program" analysis
  4. Move to /src/node_base.gypi to make node.gyp easier to read.
  5. Maybe solve AIX specific linking issue

side-fixes:

  • eliminate rename_node_bin_win by naming the node lib file on Windows libnode.lib
  • enable PCH for the node sources
  • explicitly define Environment::context() so it can safely reside in two translation units. (otherwise undefined behavior emerges)
  • minor .h cleanup to ensure node_lib builds correctly.
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Oct 12, 2018
@refack refack self-assigned this Oct 12, 2018
@refack refack added windows Issues and PRs related to the Windows platform. build Issues and PRs related to build files or the CI. wip Issues and PRs that are still a work in progress. labels Oct 12, 2018
@refack
Copy link
Contributor Author

refack commented Oct 12, 2018

Sanity CI: https://ci.nodejs.org/job/node-test-pull-request/17747/
https://ci.nodejs.org/job/node-test-pull-request/17748/
/CC @nodejs/build-files @nodejs/gyp @nodejs/platform-windows

@refack refack force-pushed the create-node_base branch 3 times, most recently from db8bdb6 to 682c932 Compare October 13, 2018 23:02
@refack refack removed the wip Issues and PRs that are still a work in progress. label Oct 13, 2018
@refack
Copy link
Contributor Author

refack commented Oct 13, 2018

CI: https://ci.nodejs.org/job/node-test-pull-request/17838/
https://ci.nodejs.org/job/node-test-commit/22309/

If I could get some review, this refactoring makes it for a much better experience in MSVS. It's now possible to only compile node_base and get a result within a minute, if you changes compile of not.

@refack
Copy link
Contributor Author

refack commented Oct 15, 2018

@refack refack mentioned this pull request Oct 16, 2018
@refack
Copy link
Contributor Author

refack commented Oct 23, 2018

On Windows and SmartOS Getting:

20:39:21 gyp ERR! UNCAUGHT EXCEPTION 
20:39:21 gyp ERR! stack ReferenceError: DTRACE_NET_STREAM_END is not defined
20:39:21 gyp ERR! stack     at Socket.end (net.js:550:3)
20:39:21 gyp ERR! stack     at findVS2017 (c:\workspace\node-test-binary-windows\deps\npm\node_modules\node-gyp\lib\find-vs2017.js:43:15)
20:39:21 gyp ERR! stack     at c:\workspace\node-test-binary-windows\deps\npm\node_modules\node-gyp\lib\configure.js:94:9
20:39:21 gyp ERR! stack     at c:\workspace\node-test-binary-windows\deps\npm\node_modules\mkdirp\index.js:30:20
20:39:21 gyp ERR! stack     at FSReqCallback.oncomplete (fs.js:148:20)
20:39:21 gyp ERR! System Windows_NT 10.0.10240

Which is a linking issue...

@refack
Copy link
Contributor Author

refack commented Oct 23, 2018

On macOS:

20:18:59   rm -f /Users/iojs/build/workspace/node-test-commit-osx/nodes/osx1011/out/Release/libnode.a && ./gyp-mac-tool filter-libtool libtool -Wl,-force_load,/Users/iojs/build/workspace/node-test-commit-osx/nodes/osx1011/out/Release/libnode_base.a -static -o /Users/iojs/build/workspace/node-test-commit-osx/nodes/osx1011/out/Release/libnode.a /Users/iojs/build/workspace/node-test-commit-osx/nodes/osx1011/out/Release/obj.target/node_lib/src/node_code_cache_stub.o /Users/iojs/build/workspace/node-test-commit-osx/nodes/osx1011/out/Release/obj.target/node_lib/src/node_dtrace.o /Users/iojs/build/workspace/node-test-commit-osx/nodes/osx1011/out/Release/obj.target/node_lib/gen/node_javascript.o
20:18:59 error: /Library/Developer/CommandLineTools/usr/bin/libtool: unknown option character `W' in: -Wl,-force_load,/Users/iojs/build/workspace/node-test-commit-osx/nodes/osx1011/out/Release/libnode_base.a

Which means you can't "force load" with libtool.

@richardlau
Copy link
Member

Maybe solve AIX specific linking issue

@refack Any more details on what the linking issue was/is?

@refack
Copy link
Contributor Author

refack commented Oct 23, 2018

Any more details on what the linking issue was/is?

There's a bunch of code in node.gyp that specific to AIX

node/node.gyp

Lines 925 to 972 in f1b9546

'conditions': [
[ 'OS=="aix" and node_shared=="true"', {
'variables': {'real_os_name': '<!(uname -s)',},
'targets': [
{
'target_name': 'node_aix_shared',
'type': 'shared_library',
'product_name': '<(node_core_target_name)',
'ldflags': [ '--shared' ],
'product_extension': '<(shlib_suffix)',
'conditions': [
['target_arch=="ppc64"', {
'ldflags': [
'-Wl,-blibpath:/usr/lib:/lib:'
'/opt/freeware/lib/pthread/ppc64'
],
}],
['target_arch=="ppc"', {
'ldflags': [
'-Wl,-blibpath:/usr/lib:/lib:/opt/freeware/lib/pthread'
],
}],
['"<(real_os_name)"=="OS400"', {
'ldflags': [
'-Wl,-blibpath:/QOpenSys/pkgs/lib:/QOpenSys/usr/lib',
'-Wl,-bbigtoc',
'-Wl,-brtl',
],
}],
],
'includes': [
'node.gypi'
],
'dependencies': [ '<(node_lib_target_name)' ],
'include_dirs': [
'src',
'deps/v8/include',
],
'sources': [
'<@(library_files)',
'common.gypi',
],
'direct_dependent_settings': {
'ldflags': [ '-Wl,-brtl' ],
},
},
]
}], # end aix section

I would like to fold it into the new node_base target...

@refack
Copy link
Contributor Author

refack commented Oct 29, 2018

@refack refack force-pushed the create-node_base branch 2 times, most recently from 0e09b56 to 1cdc26f Compare October 29, 2018 02:19
@refack
Copy link
Contributor Author

refack commented Oct 29, 2018

@refack
Copy link
Contributor Author

refack commented Oct 30, 2018

@richardlau richardlau added the wip Issues and PRs that are still a work in progress. label Oct 30, 2018
@richardlau
Copy link
Member

@refack I'm assuming the presence of the dontland! commit means this is still in progress and added the label.

@refack
Copy link
Contributor Author

refack commented Oct 31, 2018

Yeah. I thought I had this figured out, but it keeps surprising me with new edge cases...
(the dontland! is to overcome compiler OOM issues of Windows 🤦‍♂️)

If you have the time I'd love some review on the first few commits, they are pretty complete:

  • Create node_base - 3a7fdce
  • Add more pre-compiled headers on Windows -27d769b
  • Only do LTCG for the node exe - 3521f8a
  • Refactor ETW and DTRACE gyp - 2ce719d

@refack
Copy link
Contributor Author

refack commented Oct 31, 2018

Good news, I can now do a full (cold cache) windows build in 20% less time, and with clcache and warm cache, full windows build in < 4 minutes.

Smoke test CI: https://ci.nodejs.org/job/node-test-pull-request/18263/
Smoke test CI: https://ci.nodejs.org/job/node-test-pull-request/18264/

@refack refack force-pushed the create-node_base branch 2 times, most recently from a874629 to f084e58 Compare October 31, 2018 16:36
@refack
Copy link
Contributor Author

refack commented Nov 1, 2018

* remove unused includes
* make writing content-stable as to not trigger rebuilding
* move `arraysize` to util.h to reduce the need for `node_intrnals.h`
* make sure options_parser singletons init in order
* deoptimize NodeTraceWriter
@refack refack mentioned this pull request Nov 23, 2018
3 tasks
@refack refack closed this Dec 21, 2018
@refack refack deleted the create-node_base branch December 21, 2018 01:32
@refack refack removed their assignment Mar 11, 2019
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. c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. windows Issues and PRs related to the Windows platform. wip Issues and PRs that are still a work in progress.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants