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

build: ignore empty folders in test-addons-napi #16380

Closed
wants to merge 1 commit into from
Closed

build: ignore empty folders in test-addons-napi #16380

wants to merge 1 commit into from

Conversation

addaleax
Copy link
Member

The same as #16031 except for N-API addons.

Fixes: #13521

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

build

@addaleax addaleax added addons Issues and PRs related to native addons. build Issues and PRs related to build files or the CI. node-api Issues and PRs related to the Node-API. test Issues and PRs related to the tests. labels Oct 22, 2017
@nodejs-github-bot nodejs-github-bot added the build Issues and PRs related to build files or the CI. label Oct 22, 2017
hiroppy
hiroppy approved these changes Oct 22, 2017
View reviewed changes
Copy link
Member

@hiroppy hiroppy left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I had the same issue. I confirmed the PR locally, the test passed! Thank you @addaleax

gibfahn
gibfahn approved these changes Oct 22, 2017
View reviewed changes
Copy link
Member

@gibfahn gibfahn left a comment

Choose a reason for hiding this comment

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

LGTM with suggestion

Makefile
@for dirname in test/addons-napi/*/; do \
if [ ! -f "$$PWD/$${dirname}binding.gyp" ]; then \
continue; fi ; \
Copy link
Member

Choose a reason for hiding this comment

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

Suggestion: it might be cleaner if you change

	if [ ! -f "$$PWD/$${dirname}binding.gyp" ]; then \
			continue; fi ; \

to

[ ! -f "$$PWD/$${dirname}binding.gyp" ] && continue ; \

Maybe it's just me that find that cleaner though.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think moving control flow into logical expressions is … hm. 😄

Copy link
Member

Choose a reason for hiding this comment

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

It just wasn't immediately obvious where the end of the if block was, because the fi isn't on its own line.

As this already squashes things down, you could go further and do:

		if [ ! -f "$$PWD/$${dirname}binding.gyp" ]; then continue; fi ; \

I think moving control flow into logical expressions is … hm. 😄

Unrelated to this PR, but I'd be interested to know what the objection is, I've heard a lot of people say this, but for me [ x ] && y is just a cleaner way of saying if [ x ]; then y; fi (for simple ys). Obviously in a language with nicer ifs, then there's not much gain (e.g. for rust if x { y }), but in bash it seems more worthwhile.

Copy link
Member Author

Choose a reason for hiding this comment

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

@gibfahn I really like [ x ] && y too, but for other things – shell scripts are exceptional in that you can actually make control flow part of the expression, and that just feels pretty odd to me I guess?

@gireeshpunathil
Copy link
Member

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

mhdawson
mhdawson approved these changes Oct 24, 2017
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

@tniessen
Copy link
Member

Landed in 65d2067, thanks Anna!

@tniessen tniessen closed this Oct 29, 2017
tniessen pushed a commit that referenced this pull request Oct 29, 2017
@addaleax @tniessen
build: ignore empty folders in test-addons-napi
65d2067 
The same as #16031 except
for N-API addons.

PR-URL: #16380
Fixes: #13521
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
@jasnell jasnell mentioned this pull request Oct 30, 2017
Merged
gibfahn pushed a commit that referenced this pull request Oct 30, 2017
@addaleax @gibfahn
build: ignore empty folders in test-addons-napi
293fe0e 
The same as #16031 except
for N-API addons.

PR-URL: #16380
Fixes: #13521
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
gibfahn pushed a commit that referenced this pull request Oct 30, 2017
@addaleax @gibfahn
build: ignore empty folders in test-addons-napi
d9cc8ee 
The same as #16031 except
for N-API addons.

PR-URL: #16380
Fixes: #13521
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
gibfahn pushed a commit that referenced this pull request Oct 31, 2017
@addaleax @gibfahn
build: ignore empty folders in test-addons-napi
528edb2 
The same as #16031 except
for N-API addons.

PR-URL: #16380
Fixes: #13521
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
@gibfahn gibfahn mentioned this pull request Oct 31, 2017
Merged
Qard pushed a commit to ayojs/ayo that referenced this pull request Nov 2, 2017
@addaleax @Qard
build: ignore empty folders in test-addons-napi
1dd85ce 
The same as nodejs/node#16031 except
for N-API addons.

PR-URL: nodejs/node#16380
Fixes: nodejs/node#13521
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Qard pushed a commit to ayojs/ayo that referenced this pull request Nov 2, 2017
@addaleax @Qard
build: ignore empty folders in test-addons-napi
55b5bd3 
The same as nodejs/node#16031 except
for N-API addons.

PR-URL: nodejs/node#16380
Fixes: nodejs/node#13521
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
addaleax added a commit to ayojs/ayo that referenced this pull request Dec 7, 2017
@addaleax
build: ignore empty folders in test-addons-napi
23e3591 
The same as nodejs/node#16031 except
for N-API addons.

PR-URL: nodejs/node#16380
Fixes: nodejs/node#13521
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gibfahn gibfahn gibfahn approved these changes

@mhdawson mhdawson mhdawson approved these changes

@gireeshpunathil gireeshpunathil gireeshpunathil approved these changes

@hiroppy hiroppy hiroppy approved these changes

@cjihrig cjihrig cjihrig approved these changes

@jasnell jasnell jasnell approved these changes

@danbev danbev danbev approved these changes

Assignees
No one assigned
Labels
addons Issues and PRs related to native addons. build Issues and PRs related to build files or the CI. node-api Issues and PRs related to the Node-API. test Issues and PRs related to the tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

test: n-api/addons test change issues