Skip to content

src: add an option to make compile cache path relative #58797

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
Merged
merged 1 commit into from
Sep 12, 2025
Merged

src: add an option to make compile cache path relative #58797

merged 1 commit into from
Sep 12, 2025
+448 −15

Conversation

Aditi-1400
Copy link
Contributor

@Aditi-1400 Aditi-1400 commented Jun 23, 2025 •
edited by joyeecheung
Loading

Adds an option (NODE_COMPILE_CACHE_RELATIVE_PATH) for the built-in compile cache to encode the hashes with relative file paths. On enabling the option,
the source directory along with cache directory can be bundled and moved, and the cache continues to work.

When enabled, paths encoded in hash are relative to compile cache directory.

Fixes: #58755
Refs: #52696

Thanks @joyeecheung for all the help :)

@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. needs-ci PRs that need a full CI run. labels Jun 23, 2025
@codecov Codecov
Copy link

codecov bot commented Jun 23, 2025 •
edited
Loading

Codecov Report

❌ Patch coverage is 86.07595% with 11 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.89%. Comparing base (9bcc5a8) to head (b879f6b).
⚠️ Report is 319 commits behind head on main.

Files with missing lines Patch % Lines
src/compile_cache.cc 81.48% 1 Missing and 4 partials ⚠️
src/path.cc 73.68% 2 Missing and 3 partials ⚠️
src/env.cc 87.50% 0 Missing and 1 partial ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main   #58797      +/-   ##
==========================================
+ Coverage   89.37%   89.89%   +0.52%     
==========================================
  Files         654      667      +13     
  Lines      192554   196670    +4116     
  Branches    37434    38617    +1183     
==========================================
+ Hits       172086   176794    +4708     
+ Misses      12995    12331     -664     
- Partials     7473     7545      +72
Files with missing lines Coverage Δ
lib/internal/modules/helpers.js 98.29% <100.00%> (+0.92%) ⬆️
src/compile_cache.h 100.00% <ø> (ø)
src/env.h 98.14% <ø> (ø)
src/node_modules.cc 77.06% <100.00%> (-0.25%) ⬇️
src/env.cc 80.83% <87.50%> (-0.05%) ⬇️
src/compile_cache.cc 80.78% <81.48%> (-0.01%) ⬇️
src/path.cc 72.81% <73.68%> (+3.76%) ⬆️

... and 195 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

joyeecheung
joyeecheung reviewed Jun 23, 2025
View reviewed changes
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.

Can you add some integration to module.enableCompileCache() as well? I think having it in options bag when the first argument is an object might work well enough. The path can be options.path in that case.

@joyeecheung joyeecheung mentioned this pull request Jul 3, 2025
Open
13 tasks
@mcollina mcollina added the semver-minor PRs that contain new features and should be released in the next minor version. label Jul 4, 2025
jasnell
jasnell reviewed Jul 20, 2025
View reviewed changes
joyeecheung
joyeecheung reviewed Jul 28, 2025
View reviewed changes
@Aditi-1400 Aditi-1400 force-pushed the compile-cache branch 2 times, most recently from 5abcfc9 to de78d1a Compare August 1, 2025 13:15
jasnell
jasnell reviewed Aug 1, 2025
View reviewed changes
jasnell
jasnell reviewed Aug 1, 2025
View reviewed changes
jasnell
jasnell reviewed Aug 1, 2025
View reviewed changes
jasnell
jasnell reviewed Aug 1, 2025
View reviewed changes
jasnell
jasnell reviewed Aug 1, 2025
View reviewed changes
joyeecheung
joyeecheung reviewed Aug 1, 2025
View reviewed changes
jasnell
jasnell reviewed Aug 1, 2025
View reviewed changes
jasnell
jasnell reviewed Aug 1, 2025
View reviewed changes
jasnell
jasnell reviewed Aug 1, 2025
View reviewed changes
jasnell
jasnell reviewed Aug 1, 2025
View reviewed changes
jasnell
jasnell reviewed Aug 1, 2025
View reviewed changes
jasnell
jasnell requested changes Aug 1, 2025
View reviewed changes
Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

The file:// URL handling in this is incorrect. Either support for file:// URLs here at all should be dropped or they should be correctly handled.

jasnell

This comment was marked as duplicate.

Sign in to view

@Aditi-1400 Aditi-1400 force-pushed the compile-cache branch 4 times, most recently from ddd3bcd to d03bf27 Compare August 11, 2025 16:30
@Aditi-1400
src: add an option to make compile cache portable
b879f6b 
Adds an option (NODE_COMPILE_CACHE_PORTABLE) for
the built-in compile cache to encode the hashes with
relative file paths. On enabling the option,
the source directory along with cache directory can be
bundled and moved, and the cache continues to work.

When enabled, paths encoded in hash are relative to
compile cache directory.
@Aditi-1400 Aditi-1400 added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 28, 2025
@github-actions github-actions bot added request-ci-failed An error occurred while starting CI via request-ci label, and manual interventon is needed. and removed request-ci Add this label to start a Jenkins CI on a PR. labels Aug 28, 2025
@github-actions GitHub Actions
Copy link
Contributor

Failed to start CI
   ⚠  Commits were pushed since the last approving review:
   ⚠  - src: add an option to make compile cache portable
   ✘  Refusing to run CI on potentially unsafe PR
https://github.com/nodejs/node/actions/runs/17295846847

@Aditi-1400 Aditi-1400 added request-ci Add this label to start a Jenkins CI on a PR. and removed request-ci-failed An error occurred while starting CI via request-ci label, and manual interventon is needed. request-ci Add this label to start a Jenkins CI on a PR. labels Aug 28, 2025
@Aditi-1400 Aditi-1400 requested a review from joyeecheung August 28, 2025 15:37
@joyeecheung joyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 8, 2025
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 8, 2025
@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

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

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

@nodejs-github-bot
Copy link
Collaborator

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

@Aditi-1400 Aditi-1400 added the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 12, 2025
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 12, 2025
@nodejs-github-bot nodejs-github-bot merged commit 94422e8 into nodejs:main Sep 12, 2025
73 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 94422e8

targos pushed a commit that referenced this pull request Sep 12, 2025
@Aditi-1400 @targos
src: add an option to make compile cache portable
c4ef8b5 
Adds an option (NODE_COMPILE_CACHE_PORTABLE) for
the built-in compile cache to encode the hashes with
relative file paths. On enabling the option,
the source directory along with cache directory can be
bundled and moved, and the cache continues to work.

When enabled, paths encoded in hash are relative to
compile cache directory.

PR-URL: #58797
Fixes: #58755
Refs: #52696
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@joyeecheung joyeecheung added dont-land-on-v24.x PRs that should not land on the v24.x-staging branch and should not be released in v24.x. dont-land-on-v25.x PRs that should not land on the v25.x-staging branch and should not be released in v25.x. labels Sep 18, 2025
joyeecheung
joyeecheung reviewed Sep 18, 2025
View reviewed changes
lib/internal/modules/helpers.js
* - `portable`: If `portable` is true, the cache directory will be considered relative. Defaults to false.
* If cache path is undefined, it defaults to the NODE_MODULE_CACHE environment variable.
* If `NODE_MODULE_CACHE` isn't set, it defaults to `path.join(os.tmpdir(), 'node-compile-cache')`.
* @param {string | { path?: string, portable?: boolean } | undefined} options
Copy link
Member

Choose a reason for hiding this comment

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

I just noticed an issue with the new options - it should be directory instead of path to align with the return value. Marking the PR with dont-land labels to fix it before it gets released.

@github-actions github-actions bot mentioned this pull request Sep 24, 2025
Merged
nodejs-github-bot added a commit that referenced this pull request Sep 24, 2025
@nodejs-github-bot
2025-09-25, Version 24.9.0 (Current)
41613c5 
Notable changes:

http:
  * (SEMVER-MINOR) add shouldUpgradeCallback to let servers control HTTP upgrades (Tim Perry) #59824
sqlite:
  * (SEMVER-MINOR) cleanup ERM support and export Session class (James M Snell) #58378
  * (SEMVER-MINOR) add tagged template (0hm☘️) #58748
src:
  * (SEMVER-MINOR) add an option to make compile cache portable (Aditi) #58797
worker:
  * (SEMVER-MINOR) add heap profile API (theanarkh) #59846

PR-URL: #59997
@targos targos mentioned this pull request Sep 24, 2025
Open
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mcollina mcollina mcollina left review comments

@joyeecheung joyeecheung joyeecheung approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. dont-land-on-v24.x PRs that should not land on the v24.x-staging branch and should not be released in v24.x. dont-land-on-v25.x PRs that should not land on the v25.x-staging branch and should not be released in v25.x. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support relative compile cache
5 participants
@Aditi-1400 @nodejs-github-bot @jasnell @joyeecheung @mcollina