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

build: fixes and optimizations in gyp files #27108

Closed
wants to merge 2 commits into from
Closed

build: fixes and optimizations in gyp files #27108

wants to merge 2 commits into from

Conversation

refack
Copy link
Contributor

@refack refack commented Apr 5, 2019 •
edited
Loading

  • Move V8 PCH to tools/v8_gypfiles
  • Create new v8_initializers_pch.h
  • Define GTEST_LANG_CXX11 for gtest compatibility with C++17 (TR namespace removed)
  • Include PCH header directory when using PCH
  • De-dup directives in toolchain.gypi and add comments
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot
Copy link
Collaborator

Lite-CI: https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/3182

@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. install Issues and PRs related to the installers. tools Issues and PRs related to the tools directory. windows Issues and PRs related to the Windows platform. labels Apr 5, 2019
@refack refack self-assigned this Apr 5, 2019
@refack refack added the gyp Issues and PRs related to the GYP tool and .gyp build files label Apr 5, 2019
@nodejs-github-bot
Copy link
Collaborator

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

@refack
Copy link
Contributor Author

refack commented Apr 5, 2019

/CC @nodejs/build-files @nodejs/v8-update

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@refack refack force-pushed the gyp-optimization-73 branch 4 times, most recently from bb39ecf to ea42d79 Compare April 7, 2019 23:54
richardlau
richardlau reviewed Apr 8, 2019
View reviewed changes
deps/gtest/gtest.gyp
},
'include_dirs': ['.', 'include'],
'defines': [
'GTEST_LANG_CXX11=1',
Copy link
Member

Choose a reason for hiding this comment

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

https://codereview.chromium.org/2174663002/. Maybe we should be bumping our vendored in version of gtest?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have that in the pipe line.

richardlau
richardlau previously requested changes Apr 8, 2019
View reviewed changes
vcbuild.bat Outdated Show resolved Hide resolved
@refack refack force-pushed the gyp-optimization-73 branch from 24ac28b to 0e089b3 Compare April 8, 2019 14:57
@richardlau richardlau dismissed their stale review April 8, 2019 15:02

Addressed

@refack refack mentioned this pull request Apr 8, 2019
Closed
2 tasks
@refack refack force-pushed the gyp-optimization-73 branch 4 times, most recently from d161800 to 9b0ac8b Compare April 8, 2019 21:30
@nodejs-github-bot
Copy link
Collaborator

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

@refack
Copy link
Contributor Author

refack commented Apr 8, 2019 •
edited
Loading

Ping @nodejs/build-files PTAL.
I would really like to have this in for v12
(Only failing test is code-cache/test-code-cache.js)

@refack
Copy link
Contributor Author

refack commented Apr 8, 2019

Ping @nodejs/v8-update

@joyeecheung
Copy link
Member

joyeecheung commented Apr 9, 2019 •
edited
Loading

Can you split this into smaller PRs? I can review some of the GYP changes, but can't really sign off on Windows/MSVC spcific changes.

@refack refack force-pushed the gyp-optimization-73 branch from 9b0ac8b to 18e465d Compare April 9, 2019 13:02
@refack refack force-pushed the gyp-optimization-73 branch 2 times, most recently from 13a110d to 9b770b3 Compare April 9, 2019 13:30
@refack
Copy link
Contributor Author

refack commented Apr 9, 2019

Can you split this into smaller PRs?

I moved out the V8 (#27148) and Windows (#27149, #27150) changes.

@nodejs-github-bot
Copy link
Collaborator

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

@refack refack requested a review from joyeecheung April 9, 2019 13:30
@refack refack removed install Issues and PRs related to the installers. windows Issues and PRs related to the Windows platform. labels Apr 9, 2019
@nodejs-github-bot
Copy link
Collaborator

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

@refack refack added the review wanted PRs that need reviews. label Apr 9, 2019
joyeecheung
joyeecheung reviewed Apr 9, 2019
View reviewed changes
node.gyp Outdated
['OS=="win"', {
'sources': ['src/res/node.rc'],
'conditions': [
['node_use_etw=="true"', {
Copy link
Member

Choose a reason for hiding this comment

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

Um, doesn't this break etw builds? (I am not sure who actually uses this, though, and it's been broken quite often, but it's listed in https://github.com/nodejs/node/blob/master/doc/guides/diagnostic-tooling-support-tiers.md)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Code just moved to

node/node.gyp

Line 627 in 380d1a6

[ 'node_use_etw=="true"', {

@refack refack mentioned this pull request Apr 9, 2019
Merged
3 tasks
@refack refack force-pushed the gyp-optimization-73 branch from 63f4c78 to 380d1a6 Compare April 11, 2019 14:55
@nodejs-github-bot
Copy link
Collaborator

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

@refack
Copy link
Contributor Author

refack commented Apr 11, 2019

@nodejs/build-files PTAL

@BridgeAR BridgeAR force-pushed the master branch 2 times, most recently from 8ae28ff to 2935f72 Compare May 31, 2020 12:19
@jasnell jasnell added the stalled Issues and PRs that are stalled. label Jun 19, 2020
@jasnell
Copy link
Member

jasnell commented Jun 19, 2020

No activity on this is over a year, does not appear to be moving forward. Closing, can reopen if someone wishes to pick it back up

@jasnell jasnell closed this Jun 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joyeecheung joyeecheung joyeecheung left review comments

@richardlau richardlau richardlau left review comments

Assignees

@refack refack

Labels
build Issues and PRs related to build files or the CI. gyp Issues and PRs related to the GYP tool and .gyp build files review wanted PRs that need reviews. stalled Issues and PRs that are stalled. tools Issues and PRs related to the tools directory.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@refack @nodejs-github-bot @joyeecheung @jasnell @richardlau