build: ignore empty folders in test-addons-napi - #16380
Conversation
Sorry, something went wrong.
The same as nodejs#16031 except for N-API addons. Fixes: nodejs#13521
gibfahn
left a comment
There was a problem hiding this comment.
LGTM with suggestion
Sorry, something went wrong.
| # (https://github.com/nodejs/node/issues/14843) | ||
| @for dirname in test/addons-napi/*/; do \ | ||
| if [ ! -f "$$PWD/$${dirname}binding.gyp" ]; then \ | ||
| continue; fi ; \ |
There was a problem hiding this comment.
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.
Sorry, something went wrong.
There was a problem hiding this comment.
I think moving control flow into logical expressions is … hm. 😄
Sorry, something went wrong.
There was a problem hiding this comment.
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.
Sorry, something went wrong.
There was a problem hiding this comment.
@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?
Sorry, something went wrong.
Sorry, something went wrong.
mhdawson
left a comment
There was a problem hiding this comment.
LGTM
Sorry, something went wrong.
The same as #16031 except for N-API addons. PR-URL: #16380 Fixes: #13521 Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
The same as #16031 except for N-API addons. PR-URL: #16380 Fixes: #13521 Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
The same as #16031 except for N-API addons. PR-URL: #16380 Fixes: #13521 Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
The same as #16031 except for N-API addons. PR-URL: #16380 Fixes: #13521 Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
The same as nodejs/node#16031 except for N-API addons. PR-URL: nodejs/node#16380 Fixes: nodejs/node#13521 Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
The same as nodejs/node#16031 except for N-API addons. PR-URL: nodejs/node#16380 Fixes: nodejs/node#13521 Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
The same as nodejs/node#16031 except for N-API addons. PR-URL: nodejs/node#16380 Fixes: nodejs/node#13521 Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
The same as #16031 except for N-API addons.
Fixes: #13521
Checklist
Affected core subsystem(s)
build