Skip to content

Conversation

@bnoordhuis
Copy link
Member

Fix parsing of pkg-config --cflags-only-I. The configure_library()
step sometimes appended a list in a list instead of list of strings to
include_dirs.

This commit removes the default handling for includes and libpath
options. They don't have defaults at the moment and I don't see that
changing anytime soon. Fixing the code is more work and because it's
dead code anyway, I opted to remove it instead.

Fixes: #1985

R=@jbergstroem

CI: https://jenkins-iojs.nodesource.com/view/iojs/job/iojs+any-pr+multi/7/

@jbergstroem jbergstroem added the build Issues and PRs related to build files or the CI. label Jun 15, 2015
@Haifen
Copy link

Haifen commented Jun 15, 2015

Can confirm this fixes #1985 on the same build machine and environment where I first encountered the bug.

Referenced in Gentoo bugtracker at:
Bug 552232
Attachment 405206

@jbergstroem
Copy link
Member

I've tried adding all kinds of weird stuff to my .pc configs - seems to be robust. LGTM. I'm keen on exploring how we can avoid parsing output at all..

Fix parsing of `pkg-config --cflags-only-I`.  The configure_library()
step sometimes appended a list in a list instead of list of strings to
include_dirs.

This commit removes the default handling for includes and libpath
options.  They don't have defaults at the moment and I don't see that
changing anytime soon.  Fixing the code is more work and because it's
dead code anyway, I opted to remove it instead.

Fixes: nodejs#1985
PR-URL: nodejs#1986
Reviewed-By: Johan Bergström <[email protected]>
@bnoordhuis bnoordhuis closed this Jun 15, 2015
@bnoordhuis bnoordhuis deleted the fix-issue-1985 branch June 15, 2015 23:16
@bnoordhuis bnoordhuis merged commit c207e8d into nodejs:master Jun 15, 2015
@rvagg rvagg mentioned this pull request Jun 16, 2015
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.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants