Skip to content

Conversation

vmoroz
Copy link
Member

@vmoroz vmoroz commented Apr 11, 2025

Convert local v8impl::NewEnv function to the node_napi_env__::New method.
The new method helps creating new Node-API environment by any code that includes the node_api_internals.h header file.
There are no changes to the function bodies except for moving them to the top of the file.
The ThrowNodeApiVersionError had to be moved because the node_napi_env__::New uses it.

This change is required for the new C-based Node.js embedding API - PR #54660.
Since the PR #54660 is too big, it was decided in a Node-API meeting to split it up into smaller PRs.
This is the first PR in the series.

@vmoroz vmoroz added the node-api Issues and PRs related to the Node-API. label Apr 11, 2025
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/node-api

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. labels Apr 11, 2025
@legendecas legendecas moved this from Need Triage to In Progress in Node-API Team Project Apr 11, 2025
Copy link

codecov bot commented Apr 11, 2025

Codecov Report

Attention: Patch coverage is 37.03704% with 17 lines in your changes missing coverage. Please review.

Project coverage is 90.15%. Comparing base (795dd8e) to head (66c68ed).
Report is 51 commits behind head on main.

Files with missing lines Patch % Lines
src/node_api.cc 37.03% 15 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #57834      +/-   ##
==========================================
- Coverage   90.24%   90.15%   -0.09%     
==========================================
  Files         630      628       -2     
  Lines      185470   185036     -434     
  Branches    36375    36234     -141     
==========================================
- Hits       167371   166823     -548     
- Misses      10993    11164     +171     
+ Partials     7106     7049      -57     
Files with missing lines Coverage Δ
src/node_api_internals.h 100.00% <ø> (ø)
src/node_api.cc 76.16% <37.03%> (+0.02%) ⬆️

... and 50 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.

Copy link
Member

@mertcanaltin mertcanaltin left a comment

Choose a reason for hiding this comment

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

lgtm

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

@nodejs-github-bot
Copy link
Collaborator

@vmoroz
Copy link
Member Author

vmoroz commented Apr 14, 2025

The build fails because VS2022 build for ARM64 hits out of memory issue:

C:\Program Files\Microsoft Visual Studio\2022\Community\VC\Tools\MSVC\14.43.34808\include\tuple(132,1): fatal  error C1060: compiler is out of heap space [C:\workspace\node-compile-windows\node\tools\v8_gypfiles\v8_initializers.vcxproj]
C:\Program Files\Microsoft Visual Studio\2022\Community\VC\Tools\MSVC\14.43.34808\include\type_traits(1527,92): fatal  error C1060: compiler is out of heap space [C:\workspace\node-compile-windows\node\tools\v8_gypfiles\v8_initializers.vcxproj]

I wonder if it is a known issue and someone is looking at it, or I should see where to add the /Zm compiler option to limit the memory use.

Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

@nodejs-github-bot
Copy link
Collaborator

@StefanStojanovic
Copy link
Contributor

The build fails because VS2022 build for ARM64 hits out of memory issue:

C:\Program Files\Microsoft Visual Studio\2022\Community\VC\Tools\MSVC\14.43.34808\include\tuple(132,1): fatal  error C1060: compiler is out of heap space [C:\workspace\node-compile-windows\node\tools\v8_gypfiles\v8_initializers.vcxproj]
C:\Program Files\Microsoft Visual Studio\2022\Community\VC\Tools\MSVC\14.43.34808\include\type_traits(1527,92): fatal  error C1060: compiler is out of heap space [C:\workspace\node-compile-windows\node\tools\v8_gypfiles\v8_initializers.vcxproj]

I wonder if it is a known issue and someone is looking at it, or I should see where to add the /Zm compiler option to limit the memory use.

Hello. Yes, this is a known problem, although it doesn't occur often. We are already setting /Zm2000. It could be increased e.g., /Zm3000, but that will probably not be necessary. The reason for this is that in Node.js v24 and later, we'll be moving from MSVC to ClangCL. This is a PR making it official which should land sometime this week. With ClangCL, we are not using PCH, as we have enabled ccache, so this will not be an issue.

I have restarted CI for this, so let's see if it succeeds this time.

@lpinca lpinca added the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 19, 2025
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 19, 2025
@nodejs-github-bot nodejs-github-bot merged commit c326200 into nodejs:main Apr 19, 2025
72 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in c326200

@github-project-automation github-project-automation bot moved this from In Progress to Done in Node-API Team Project Apr 19, 2025
@vmoroz vmoroz deleted the pr/convert_newenv_to_a_method branch April 20, 2025 03:15
@vmoroz
Copy link
Member Author

vmoroz commented Apr 20, 2025

@StefanStojanovic and @lpinca , thank you for helping with the PR completion!

RafaelGSS pushed a commit that referenced this pull request May 1, 2025
PR-URL: #57834
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
RafaelGSS pushed a commit that referenced this pull request May 2, 2025
PR-URL: #57834
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
aduh95 pushed a commit that referenced this pull request May 6, 2025
PR-URL: #57834
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
aduh95 pushed a commit that referenced this pull request May 6, 2025
PR-URL: #57834
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
RafaelGSS pushed a commit that referenced this pull request May 14, 2025
PR-URL: #57834
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
aduh95 pushed a commit that referenced this pull request May 16, 2025
PR-URL: #57834
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
aduh95 pushed a commit that referenced this pull request May 17, 2025
PR-URL: #57834
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
aduh95 pushed a commit that referenced this pull request May 17, 2025
PR-URL: #57834
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
aduh95 pushed a commit that referenced this pull request May 17, 2025
PR-URL: #57834
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
aduh95 pushed a commit that referenced this pull request May 18, 2025
PR-URL: #57834
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
aduh95 pushed a commit that referenced this pull request May 19, 2025
PR-URL: #57834
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
@ghost ghost mentioned this pull request Jun 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. node-api Issues and PRs related to the Node-API.
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

8 participants