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

[node-api] rework port #29743

Merged
merged 44 commits into from
Apr 13, 2023
Merged

[node-api] rework port #29743

merged 44 commits into from
Apr 13, 2023

Conversation

Pospelove
Copy link
Contributor

@Pospelove Pospelove commented Feb 19, 2023

rework port based on feedback from the upstream team: nodejs/node#46641 (comment)

works on my arm64-osx host, needs windows verification so WIP for now

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

This is a new experimental fast check for PR issues. Please let us know if this bot is helpful!

After committing all other changes, the version database must be updated
git add -u && git commit
git checkout a325228200d7f229f3337e612e0077f2a5307090 -- versions
./vcpkg x-add-version --all
Diff
diff --git a/versions/baseline.json b/versions/baseline.json
index 1b8f59b..ad81ba5 100644
--- a/versions/baseline.json
+++ b/versions/baseline.json
@@ -5433,8 +5433,8 @@
       "port-version": 0
     },
     "node-api": {
-      "baseline": "18.12.1",
-      "port-version": 2
+      "baseline": "8",
+      "port-version": 0
     },
     "nonius": {
       "baseline": "2019-04-20",
diff --git a/versions/n-/node-api.json b/versions/n-/node-api.json
index 0dd5dae..0504213 100644
--- a/versions/n-/node-api.json
+++ b/versions/n-/node-api.json
@@ -1,5 +1,10 @@
 {
   "versions": [
+    {
+      "git-tree": "fa75e515539e8e85bfa38689e5eb33852684faa5",
+      "version": "8",
+      "port-version": 0
+    },
     {
       "git-tree": "d127d3b83f2702e56542e7c90807893464f3de79",
       "version-semver": "18.12.1",

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

This is a new experimental fast check for PR issues. Please let us know if this bot is helpful!

After committing all other changes, the version database must be updated
git add -u && git commit
git checkout a325228200d7f229f3337e612e0077f2a5307090 -- versions
./vcpkg x-add-version --all
Diff
diff --git a/versions/baseline.json b/versions/baseline.json
index 1b8f59b..ad81ba5 100644
--- a/versions/baseline.json
+++ b/versions/baseline.json
@@ -5433,8 +5433,8 @@
       "port-version": 0
     },
     "node-api": {
-      "baseline": "18.12.1",
-      "port-version": 2
+      "baseline": "8",
+      "port-version": 0
     },
     "nonius": {
       "baseline": "2019-04-20",
diff --git a/versions/n-/node-api.json b/versions/n-/node-api.json
index 0dd5dae..d2b7751 100644
--- a/versions/n-/node-api.json
+++ b/versions/n-/node-api.json
@@ -1,5 +1,10 @@
 {
   "versions": [
+    {
+      "git-tree": "6ce52d04028503b849e76861c8e591d054cb9aa9",
+      "version": "8",
+      "port-version": 0
+    },
     {
       "git-tree": "d127d3b83f2702e56542e7c90807893464f3de79",
       "version-semver": "18.12.1",

@Pospelove Pospelove changed the title Node api mod [node-api] rework port Feb 19, 2023
@JonLiu1993 JonLiu1993 added the category:port-update The issue is with a library, which is requesting update new revision label Feb 20, 2023
Copy link
Contributor

@dg0yt dg0yt left a comment

Choose a reason for hiding this comment

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

Once we start using CMake properly, we could even export the CMake config. (But I don't want to ask for it now.)

Comment on lines 3 to 7
add_custom_target(nodelib_def ALL
COMMAND ${NODEJS_EXECUTABLE} ${CMAKE_SOURCE_DIR}/generateNodeLibDef.js
COMMENT "Generating .def file"
WORKING_DIRECTORY ${CMAKE_SOURCE_DIR}
)
Copy link
Contributor

Choose a reason for hiding this comment

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

A rule to generate a particular file. This calls for add_custom_command, with an output file in CMAKE_BINARY_DIR.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would still prefer to see this changed.

COMMENT "Building node.lib"
)

add_dependencies(nodelib nodelib_def)
Copy link
Contributor

Choose a reason for hiding this comment

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

... and then this becomes obsolete.


add_dependencies(nodelib nodelib_def)

install(FILES ${CMAKE_BINARY_DIR}/node.lib DESTINATION lib)

This comment was marked as resolved.

@Pospelove Pospelove marked this pull request as ready for review February 20, 2023 20:45
@Pospelove
Copy link
Contributor Author

Pospelove commented Feb 20, 2023

@JonLiu1993 ready for review (now works on windows)

Comment on lines 3 to 7
add_custom_target(nodelib_def ALL
COMMAND ${NODEJS_EXECUTABLE} ${CMAKE_SOURCE_DIR}/generateNodeLibDef.js
COMMENT "Generating .def file"
WORKING_DIRECTORY ${CMAKE_SOURCE_DIR}
)
Copy link
Contributor

Choose a reason for hiding this comment

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

I would still prefer to see this changed.

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

This is a new experimental fast check for PR issues. Please let us know if this bot is helpful!

PRs must add only one version and must not modify any published versions

When making any changes to a library, the version or port-version in vcpkg.json or CONTROL must be modified.

error: checked-in files for node-api have changed but the version was not updated
version: 8-19.6.0
old SHA: 7d2b9b2fe1b39626659d04bd190212c949a104db
new SHA: 2e6b838f0b5e60037187a0fe40e257a2b7b7d629
Did you remember to update the version or port version?
Use --overwrite-version to bypass this check
***No files were updated***

@Pospelove
Copy link
Contributor Author

@dg0yt @JonLiu1993 ready for review

@JonLiu1993
Copy link
Member

@Pospelove, The upstream has given their suggestions for the version form of node-api-headers, you can refer to it.

@Pospelove
Copy link
Contributor Author

@JonLiu1993 done

@BillyONeal BillyONeal added info:reviewed Pull Request changes follow basic guidelines and removed depends:upstream-changes Waiting on a change to the upstream project labels Apr 13, 2023
@BillyONeal
Copy link
Member

Thanks for applying the versioning changes from upstream :)

@BillyONeal BillyONeal merged commit 78c1f5b into microsoft:master Apr 13, 2023
@BillyONeal
Copy link
Member

Thank you!

@Pospelove Pospelove deleted the node-api-mod branch April 14, 2023 05:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:new-port The issue is requesting a new library to be added; consider making a PR! category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants