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

Enable ccache on Windows v2 #56847

Merged
merged 11 commits into from
Feb 9, 2025
Merged

Conversation

StefanStojanovic
Copy link
Contributor

@StefanStojanovic StefanStojanovic commented Jan 31, 2025

This is a continuation of work that started here and considers review comments. Since there were 2 potential approaches, this PR implements the one not represented in the previous PR. For more details, you can just read that one's description, but in short, the idea here is to disable PCH when using ccache and Clang (MSVC cannot compile with PCH). This makes initial compilation twice as long as when using PCH, but the following ones are much faster.

The fIrst 3 commits in this PR are the same as the ones in the previous ones, the only difference is the 4th one.

Refs: #56705

@StefanStojanovic StefanStojanovic added windows Issues and PRs related to the Windows platform. build Issues and PRs related to build files or the CI. labels Jan 31, 2025
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/gyp
  • @nodejs/v8-update

@nodejs-github-bot nodejs-github-bot added doc Issues and PRs related to the documentations. install Issues and PRs related to the installers. needs-ci PRs that need a full CI run. tools Issues and PRs related to the tools directory. v8 engine Issues and PRs related to the V8 dependency. labels Jan 31, 2025
configure.py Outdated Show resolved Hide resolved
configure.py Outdated Show resolved Hide resolved
configure.py Outdated Show resolved Hide resolved
configure.py Outdated Show resolved Hide resolved
tools/v8_gypfiles/v8.gyp Outdated Show resolved Hide resolved
common.gypi Outdated Show resolved Hide resolved
StefanStojanovic and others added 11 commits February 4, 2025 16:33
Prior to these changes compilation with ccache was broken because of
the .h file regeneration in each compilation.
Add cp command in building docs
Remove CCACHE_USED macro
Disable PCH when using Clang and ccache
Co-authored-by: Joyee Cheung <[email protected]>
Co-authored-by: Joyee Cheung <[email protected]>
Co-authored-by: Joyee Cheung <[email protected]>
Co-authored-by: Joyee Cheung <[email protected]>
Co-authored-by: Joyee Cheung <[email protected]>
@StefanStojanovic
Copy link
Contributor Author

Thanks for the review and change suggestions @joyeecheung I added one more change missing in vcbuild.bat and everything seems to be OK now.

@StefanStojanovic StefanStojanovic added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 4, 2025
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 4, 2025
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

Copy link
Member

@joyeecheung joyeecheung left a comment

Choose a reason for hiding this comment

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

LGTM, thanks! Looking forward to trying it on Windows :)

@nodejs-github-bot
Copy link
Collaborator

@StefanStojanovic StefanStojanovic added the commit-queue Add this label to land a pull request using GitHub Actions. label Feb 6, 2025
@nodejs-github-bot nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Feb 6, 2025
@nodejs-github-bot
Copy link
Collaborator

Commit Queue failed
- Loading data for nodejs/node/pull/56847
✔  Done loading data for nodejs/node/pull/56847
----------------------------------- PR info ------------------------------------
Title      Enable ccache on Windows v2 (#56847)
   ⚠  Could not retrieve the email or name of the PR author's from user's GitHub profile!
Branch     StefanStojanovic:mefi-ccache-2 -> nodejs:main
Labels     doc, windows, install, build, v8 engine, tools, needs-ci
Commits    11
 - build,win: enable ccache
 - doc,win: add ccache support
 - deps: change V8 generated .h files for ccache
 - fix: resolve PR comments
 - Update configure.py
 - Update configure.py
 - Update configure.py
 - Update configure.py
 - Update tools/v8_gypfiles/v8.gyp
 - Update common.gypi
 - Update vcbuild.bat
Committers 1
 - StefanStojanovic <[email protected]>
PR-URL: https://github.com/nodejs/node/pull/56847
Refs: https://github.com/nodejs/node/pull/56705
Reviewed-By: Joyee Cheung <[email protected]>
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/56847
Refs: https://github.com/nodejs/node/pull/56705
Reviewed-By: Joyee Cheung <[email protected]>
--------------------------------------------------------------------------------
   ℹ  This PR was created on Fri, 31 Jan 2025 15:39:33 GMT
   ✔  Approvals: 1
   ✔  - Joyee Cheung (@joyeecheung) (TSC): https://github.com/nodejs/node/pull/56847#pullrequestreview-2593989152
   ✘  This PR needs to wait 26 more hours to land (or 0 hours if there is one more approval)
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2025-02-04T20:38:28Z: https://ci.nodejs.org/job/node-test-pull-request/64994/
- Querying data for job/node-test-pull-request/64994/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu
https://github.com/nodejs/node/actions/runs/13179308049

@StefanStojanovic StefanStojanovic removed the commit-queue-failed An error occurred while landing this pull request using GitHub Actions. label Feb 6, 2025
@targos targos added commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Feb 9, 2025
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Feb 9, 2025
@nodejs-github-bot nodejs-github-bot merged commit a017307 into nodejs:main Feb 9, 2025
68 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in a017307

targos pushed a commit that referenced this pull request Feb 10, 2025
PR-URL: #56847
Refs: #56705
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
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. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. doc Issues and PRs related to the documentations. install Issues and PRs related to the installers. needs-ci PRs that need a full CI run. tools Issues and PRs related to the tools directory. v8 engine Issues and PRs related to the V8 dependency. windows Issues and PRs related to the Windows platform.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants