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

build: clean up napi build in test-addons-clean #13034

Closed
wants to merge 1 commit into from

Conversation

joyeecheung
Copy link
Member

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

Refs: #13031

Affected core subsystem(s)

build

@nodejs-github-bot nodejs-github-bot added the build Issues and PRs related to build files or the CI. label May 15, 2017
@mscdex mscdex added the node-api Issues and PRs related to the Node-API. label May 15, 2017
@gibfahn
Copy link
Member

gibfahn commented May 15, 2017

cc/ @nodejs/n-api @nodejs/build

@refack
Copy link
Contributor

refack commented May 15, 2017

So you're exempt from dealing with vcbuild.bat because apparently we (Windows people) don't get any clean logic at all 😢
That brings up the question? way do you have cleaning targets in Makefile? git clean does a very good job?

Copy link
Contributor

@refack refack left a 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

@joyeecheung
Copy link
Member Author

@refack git clean doesn't clean up files that are gitignored though (like /test/addons/??_*/)..

I agree there can be similar rules in vcbuild.bat, but that can be solved in another PR, no?

@refack
Copy link
Contributor

refack commented May 15, 2017

@refack git clean doesn't clean up files that are gitignored though (like /test/addons/??_*/)..

-x cleans ignored files (which is good enough for me).

I agree there can be similar rules in vcbuild.bat, but that can be solved in another PR, no?

IMHO there's no need... (If there was any logic IMHO is should have been changed here...)

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

@gibfahn
Copy link
Member

gibfahn commented May 15, 2017

That brings up the question? way do you have cleaning targets in Makefile? git clean does a very good job?

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, make clean won't delete them but git clean -fdx will (I think).

The subtargets like clean-addons are useful though, you can wipe out the addons build files without having to rebuild node entirely.

@mhdawson
Copy link
Member

@refack
Copy link
Contributor

refack commented May 16, 2017

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, make clean won't delete them but git clean -fdx will (I think).

The subtargets like clean-addons are useful though, you can wipe out the addons build files without having to rebuild node entirely.

Well then you enjoy your well-thought-of Makefile targets 😞

@joyeecheung
Copy link
Member Author

joyeecheung commented May 16, 2017

@refack Hmm..I didn't realize that git clean -x does the job as well, sorry. Although I still think this is useful when working on addon tests.

@mhdawson
Copy link
Member

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 ?

@refack
Copy link
Contributor

refack commented May 16, 2017

I was always +1, and as I said no need to add to vcbuild.bat

@mhdawson
Copy link
Member

Landed as 6342988

@mhdawson mhdawson closed this May 17, 2017
mhdawson pushed a commit that referenced this pull request May 17, 2017
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]>
anchnk pushed a commit to anchnk/node that referenced this pull request May 19, 2017
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]>
@jasnell jasnell mentioned this pull request May 28, 2017
@gibfahn gibfahn mentioned this pull request Jun 15, 2017
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI. node-api Issues and PRs related to the Node-API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants