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.

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

build: fix pkg-config output parsing in configure #1986

Merged
merged 1 commit into from
Jun 15, 2015

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