-
Notifications
You must be signed in to change notification settings - Fork 30.2k
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
build: clean up napi build in test-addons-clean #13034
Conversation
cc/ @nodejs/n-api @nodejs/build |
So you're exempt from dealing with |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving no need for changes in vcbuild.bat
@refack I agree there can be similar rules in |
IMHO there's no need... (If there was any logic IMHO is should have been changed here...) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Interesting question, not sure if there's any difference between the two, except that if you've added some new files and not committed yet, The subtargets like |
Well then you enjoy your well-thought-of Makefile targets 😞 |
@refack Hmm..I didn't realize that |
I tend to agree that since there is a clean target for regular addons, bringing napi in line with that makes sense. If we decide that there is a better approach then a PR could address that for both the original addons and napi-addons. Any objections to landing based on that approach ? |
I was always +1, and as I said no need to add to |
Landed as 6342988 |
PR-URL: #13034 Ref: #13031 Reviewed-By: Daniel Bevenius <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Rajaram Gaunker <[email protected]>
PR-URL: nodejs#13034 Ref: nodejs#13031 Reviewed-By: Daniel Bevenius <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Rajaram Gaunker <[email protected]>
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passesRefs: #13031
Affected core subsystem(s)
build