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

Add helper scripts for updating headers and symbols.js #7

Merged
merged 10 commits into from
Feb 15, 2023

Conversation

KevinEady
Copy link
Contributor

@KevinEady KevinEady commented Dec 30, 2022

Added two new scripts runnable via npm:

  • npm run-script update-headers: Fetch latest headers from nodejs/node and removes NAPI_EXPERIMENTAL preprocessor checks of #ifdef and #ifndef, assuming it is not defined.
  • npm run-script write-symbols: Use clang to process headers to create symbols.js

This PR also adds a GitHub Actions workflow for performing the sync on a daily schedule (also can be manually triggered).

Relates: #2

- update-headers.js: Fetch latest headers from nodejs/node
- write-symbols.js: Use clang to process headers to create symbols.js
scripts/update-headers.js Outdated Show resolved Hide resolved
@KevinEady KevinEady marked this pull request as draft December 30, 2022 23:00
@KevinEady KevinEady marked this pull request as ready for review December 31, 2022 03:30
Copy link
Member

@NickNaso NickNaso left a comment

Choose a reason for hiding this comment

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

Just some comments.

scripts/write-symbols.js Outdated Show resolved Hide resolved
scripts/write-symbols.js Outdated Show resolved Hide resolved
scripts/update-headers.js Outdated Show resolved Hide resolved
scripts/write-symbols.js Outdated Show resolved Hide resolved
- Add 'use strict'
- Make write-symbols main() async
@KevinEady
Copy link
Contributor Author

@KevinEady
Copy link
Contributor Author

scripts/write-symbols.js Outdated Show resolved Hide resolved
- Remove double string interpolation

Co-authored-by: Chengzhong Wu <[email protected]>
@mhdawson
Copy link
Member

@KevinEady I think when we discussed in a Node-API meeting you were going to update to remove the Experimental part of the headers. Was that added and I just can't find it?

@mhdawson
Copy link
Member

@KevinEady, I found the part that removes the experimental functions from the symbols but I think we also should do that in the header file as well.

@KevinEady
Copy link
Contributor Author

@KevinEady, I found the part that removes the experimental functions from the symbols but I think we also should do that in the header file as well.

Addressed in 41f0767...36a1e92

This action run https://github.com/KevinEady/node-api-headers/actions/runs/4174541956/jobs/7228252444 created PR https://github.com/KevinEady/node-api-headers/pull/10/files?diff=split&w=1

@NickNaso @mhdawson this is ready for review again

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

@legendecas legendecas merged commit ecefbdd into nodejs:main Feb 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants