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: avoid passing empty strings to build flags #1789

Closed
wants to merge 2 commits into from

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
@Fishrock123 Fishrock123 added the build Issues and PRs related to build files or the CI. label May 25, 2015
@@ -690,7 +690,8 @@ def configure_library(lib, output):
if default_libpath:
default_libpath = '-L' + default_libpath
(pkg_libs, pkg_cflags, pkg_libpath) = pkg_config(lib)
cflags = pkg_cflags.split('-I') if pkg_cflags else default_cflags
# Remove empty strings from the list of include_dirs
cflags = filter(None, pkg_cflags.split('-I')) if pkg_cflags else default_cflags
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
Member Author

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
Member Author

Choose a reason for hiding this comment

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

@bnoordhuis filter(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_cflags
if pkg_cflags:
  cflags = filter(False, map(str.strip, pkg_cflags.split('-I')))
  # Maybe easier to read:
  cflags = map(str.strip, pkg_cflags.split('-I'))
  cflags = [s for s in cflags if s]

Copy link
Member Author

Choose a reason for hiding this comment

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

Adapted a similar strategy.

@rvagg
Copy link
Member

rvagg commented May 30, 2015

lgtm

@jbergstroem
Copy link
Member Author

@bnoordhuis looks good to you?

@bnoordhuis
Copy link
Member

LGTM

@@ -690,7 +690,11 @@ def configure_library(lib, output):
if default_libpath:
default_libpath = '-L' + default_libpath
(pkg_libs, pkg_cflags, pkg_libpath) = pkg_config(lib)
cflags = pkg_cflags.split('-I') if pkg_cflags else default_cflags
# Remove empty strings from the list of include_dirs
if pkg_cflags:
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
Member Author

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
Member Author

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

@rvagg rvagg 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 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.

iojs 2.1.0 & pkg-config output issues
5 participants