Uh oh!
There was an error while loading. Please reload this page.
Fixes configure_library() in configure - #3135
Conversation
bnoordhuis
commented
Sep 30, 2015
Commit log should explain what changed and why, please see CONTRIBUTING.md. |
zyxar
commented
Sep 30, 2015
Shall I close this and resend another PR? |
jbergstroem
commented
Sep 30, 2015
@zyxar just force push a new commit |
jbergstroem
commented
Oct 1, 2015
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. |
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.
There was a problem hiding this comment.
I think this line is just over 80 columns? If so, please wrap it.
bnoordhuis
commented
Nov 10, 2015
Basically LGTM. @thefourtheye You want to weigh in? |
jbergstroem
commented
Nov 10, 2015
I'm way overdue on this. Apologies. I'm ok with as-is since the current state is just incorrect. |
thefourtheye
commented
Nov 11, 2015
LGTM if the CI is green. |
bnoordhuis
commented
Nov 11, 2015
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 |
zyxar
commented
Nov 11, 2015
@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>bnoordhuis
commented
Nov 16, 2015
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>MylesBorins
commented
Nov 16, 2015
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.