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

[v16.x backport] node-api,src: fix module registration in MSVC C++ #43293

Closed
wants to merge 1 commit into from
Closed

[v16.x backport] node-api,src: fix module registration in MSVC C++ #43293

wants to merge 1 commit into from
+98 −3

Conversation

vmoroz
Copy link
Member

@vmoroz vmoroz commented Jun 1, 2022

Backport PR #42459 to v16.x

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/gyp
  • @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. node-api Issues and PRs related to the Node-API. v16.x labels Jun 1, 2022
@vmoroz
node-api,src: fix module registration in MSVC C++
9669f0f 
PR-URL: nodejs#42459
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>

Backport-PR-URL: nodejs#43293
@Flarna
Copy link
Member

Flarna commented Jun 2, 2022 •
edited
Loading

The only diff to the original PR is in test/node-api/test_init_order/test_init_order.cc to replace std:size() right?

@Flarna Flarna added the request-ci Add this label to start a Jenkins CI on a PR. label Jun 2, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 2, 2022
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@Flarna Flarna added the request-ci Add this label to start a Jenkins CI on a PR. label Jun 3, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 3, 2022
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@vmoroz
Copy link
Member Author

vmoroz commented Jun 3, 2022

The only diff to the original PR is in test/node-api/test_init_order/test_init_order.cc to replace std:size() right?

Yes, it was done to fix the previously reported issue.

mhdawson
mhdawson approved these changes Jun 3, 2022
View reviewed changes
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

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

@github-actions github-actions bot mentioned this pull request Jun 4, 2022
Open
21 tasks
danielleadams pushed a commit that referenced this pull request Jun 23, 2022
@vmoroz @danielleadams
node-api,src: fix module registration in MSVC C++
5ebed62 
PR-URL: #42459
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>

Backport-PR-URL: #43293
@danielleadams
Copy link
Member

Landed in 5ebed62

danielleadams pushed a commit that referenced this pull request Jul 7, 2022
@vmoroz @danielleadams
node-api,src: fix module registration in MSVC C++
31ee218 
PR-URL: #42459
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>

Backport-PR-URL: #43293
@vmoroz vmoroz deleted the backport-42459-to-v16.x branch August 26, 2022 19:07
guangwong pushed a commit to noslate-project/node that referenced this pull request Oct 10, 2022
@vmoroz @guangwong
node-api,src: fix module registration in MSVC C++
fa15b58 
PR-URL: nodejs/node#42459
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>

Backport-PR-URL: nodejs/node#43293
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JungMinu JungMinu JungMinu approved these changes

@mhdawson mhdawson mhdawson approved these changes

@Flarna Flarna Flarna approved these changes

@addaleax addaleax Awaiting requested review from addaleax

Assignees
No one assigned
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
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@vmoroz @nodejs-github-bot @Flarna @danielleadams @JungMinu @mhdawson