Skip to content

Conversation

@jbergstroem
Copy link
Member

While checking the return values from icu-i18n, we didn't validate the content (enough) before passing it to the build system. Also make cflags parsing more robust by avoiding empty strings.

The reason we're introducing filtering is to avoid empty strings which might occur for other reasons (I found deviations between the systems I've been testing on and the stuff from arch linux). When you pass input to .split() it stops doing the "convenience stuff";

>>>'-I/tmp/foo -I/bar/baz -I'.split('-I') ['', '/tmp/foo ', '/bar/baz ', ''] >>>filter(None, _) ['/tmp/foo ', '/bar/baz ']

Fixes: #1787

/R=@bnoordhuis, @alferpal

While checking the return values from icu-i18n, we didn't validate the content before passing it to the build system. Also make cflags parsing more robust by avoiding empty strings. Fixes: nodejs#1787
@Fishrock123Fishrock123 added the build Issues and PRs related to build files or the CI. label May 25, 2015
configure Outdated
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Long line.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aside, the filter step removes empty lines but not blank lines. Would that matter?

Copy link
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't come across that just yet. I'll improve the filter; guess it couldn't hurt. Just want to avoid making parsing too complex.

Copy link
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bnoordhuisfilter(False, foo) would do, right (captures '' or None)?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think filter() accepts False. What about this?

cflags=default_cflagsifpkg_cflags: cflags=filter(False, map(str.strip, pkg_cflags.split('-I'))) # Maybe easier to read:cflags=map(str.strip, pkg_cflags.split('-I')) cflags= [sforsincflagsifs]

Copy link
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adapted a similar strategy.

@jbergstroemjbergstroem mentioned this pull request May 27, 2015
@rvagg
Copy link
Member

lgtm

@jbergstroem
Copy link
MemberAuthor

@bnoordhuis looks good to you?

@bnoordhuis
Copy link
Member

LGTM

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor nitpick. What if pkg_cflags is a blank string with more than one whitespace characters? cflags would become an empty list. Is that okay?

Copy link
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@thefourtheye I've never been able to get pkg-config give me a similar output. Even if it did, calling strip on " " (two spaces) would return empty:

>>>" ".strip() ''

jbergstroem added a commit that referenced this pull request May 31, 2015
While checking the return values from icu-i18n, we didn't validate the content before passing it to the build system. Also make cflags parsing more robust by avoiding empty strings. Fixes: #1787 PR-URL: #1789 Reviewed-By: Rod Vagg <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>
@jbergstroem
Copy link
MemberAuthor

Committed in c5a1009. I'll drive including this in all minor releases that needs a fix. Thanks for the input and reviews everyone.

@rvaggrvagg mentioned this pull request Jun 1, 2015
andrewdeandrade pushed a commit to andrewdeandrade/node that referenced this pull request Jun 3, 2015
While checking the return values from icu-i18n, we didn't validate the content before passing it to the build system. Also make cflags parsing more robust by avoiding empty strings. Fixes: nodejs/node#1787 PR-URL: nodejs/node#1789 Reviewed-By: Rod Vagg <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

buildIssues and PRs related to build files or the CI.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

iojs 2.1.0 & pkg-config output issues

5 participants

@jbergstroem@rvagg@bnoordhuis@thefourtheye@Fishrock123