Fixes configure_library() in configure #3135
Conversation
|
Commit log should explain what changed and why, please see CONTRIBUTING.md. |
|
Shall I close this and resend another PR? |
|
@zyxar just force push a new commit |
|
There's some minor fixes needed in the commit message but I can do that later. I just need to sit down and review this properly; will be in a few days though (travelling). We have another similar PR that tries to massage this: #2758. |
| @@ -731,11 +731,14 @@ def configure_library(lib, output): | |||
| if pkg_cflags: | |||
| output['include_dirs'] += ( | |||
| filter(None, map(str.strip, pkg_cflags.split('-I')))) | |||
| elif options.__dict__[shared_lib + '_includes']: | |||
There was a problem hiding this comment.
Did you mean to do shared_lib + '_includes' in options?
There was a problem hiding this comment.
I'm inclined to think the __dict__ approach is correct here because it skips over empty flags.
There was a problem hiding this comment.
@bnoordhuis Oh, you are right. In that case, we can write this as if getattr(options, shared_lib + '_includes'):. But this also will work. So, I think its okay.
| filter(None, map(str.strip, pkg_cflags.split('-L')))) | ||
| output['libraries'] += [pkg_libpath] | ||
| elif options.__dict__[shared_lib + '_libpath']: | ||
| output['libraries'] += ['-L%s' % options.__dict__[shared_lib + '_libpath']] |
There was a problem hiding this comment.
I think this line is just over 80 columns? If so, please wrap it.
|
Basically LGTM. @thefourtheye You want to weigh in? |
|
I'm way overdue on this. Apologies. I'm ok with as-is since the current state is just incorrect. |
|
LGTM if the CI is green. |
|
CI: https://ci.nodejs.org/job/node-test-pull-request/702/ @zyxar Can you fix the style issue (if it is a style issue) and reword the commit log a little? The first line should start with |
|
@bnoordhuis Sure. I'd do it soon. |
Fixes configure_library() to produce correct LDFLAGS when configuring with
prebuilt 3rd-party libraries (libuv, openssl, etc), either using `pkg-config'
or `--shared-{LIBRARY}-includes=xxx --shared-{LIBRARY}-libpath=xxx'.
Fix configure_library() to produce correct LDFLAGS when configuring with
prebuilt 3rd-party libraries (libuv, openssl, etc) using `pkg-config' or
`--shared-{LIBRARY}-includes=xxx --shared-{LIBRARY}-libpath=xxx'.
PR-URL: #3135
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
|
Thanks Markus, landed in e2eb334. Just so you know, github doesn't send notifications when you add or amend commits. It's best to notify reviewers by posting a comment afterwards. |
Fix configure_library() to produce correct LDFLAGS when configuring with
prebuilt 3rd-party libraries (libuv, openssl, etc) using `pkg-config' or
`--shared-{LIBRARY}-includes=xxx --shared-{LIBRARY}-libpath=xxx'.
PR-URL: #3135
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
|
landed in lts-v4.x-staging as aef3d54 |
Fix configure_library() to produce correct LDFLAGS when configuring with
prebuilt 3rd-party libraries (libuv, openssl, etc) using `pkg-config' or
`--shared-{LIBRARY}-includes=xxx --shared-{LIBRARY}-libpath=xxx'.
PR-URL: #3135
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Fix configure_library() to produce correct LDFLAGS when configuring with
prebuilt 3rd-party libraries (libuv, openssl, etc) using `pkg-config' or
`--shared-{LIBRARY}-includes=xxx --shared-{LIBRARY}-libpath=xxx'.
PR-URL: #3135
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
No description provided.