-
Notifications
You must be signed in to change notification settings - Fork 30.3k
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: refactor pkg-config for shared libraries #1603
Changes from all commits
e7ce51e
38c6195
4365f34
d5990d1
43feb45
82e5af7
524de59
ac3df1b
bb6697f
1f99345
dea139a
0350f14
d8f4f6d
c5fed67
1e1b008
389a667
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -343,17 +343,28 @@ def b(value): | |
|
||
|
||
def pkg_config(pkg): | ||
cmd = os.popen('pkg-config --libs %s' % pkg, 'r') | ||
libs = cmd.readline().strip() | ||
ret = cmd.close() | ||
if (ret): return None | ||
|
||
cmd = os.popen('pkg-config --cflags %s' % pkg, 'r') | ||
cflags = cmd.readline().strip() | ||
ret = cmd.close() | ||
if (ret): return None | ||
|
||
return (libs, cflags) | ||
pkg_config = os.environ.get('PKG_CONFIG', 'pkg-config') | ||
args = '--silence-errors' | ||
retval = () | ||
for flag in ['--libs-only-l', '--cflags-only-I', '--libs-only-L']: | ||
try: | ||
val = subprocess.check_output([pkg_config, args, flag, pkg]) | ||
# check_output returns bytes | ||
val = val.encode().strip().rstrip('\n') | ||
except subprocess.CalledProcessError: | ||
# most likely missing a .pc-file | ||
val = None | ||
except OSError: | ||
# no pkg-config/pkgconf installed | ||
return (None, None, None) | ||
retval += (val,) | ||
return retval | ||
|
||
|
||
def format_libraries(list): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Style: two newlines between functions. |
||
"""Returns string of space separated libraries""" | ||
libraries = list.split(',') | ||
return ' '.join('-l{0}'.format(i) for i in libraries) | ||
|
||
|
||
def try_check_compiler(cc, lang): | ||
|
@@ -672,40 +683,30 @@ def configure_node(o): | |
o['variables']['node_target_type'] = 'static_library' | ||
|
||
|
||
def configure_libz(o): | ||
o['variables']['node_shared_zlib'] = b(options.shared_zlib) | ||
|
||
if options.shared_zlib: | ||
o['libraries'] += ['-l%s' % options.shared_zlib_libname] | ||
if options.shared_zlib_libpath: | ||
o['libraries'] += ['-L%s' % options.shared_zlib_libpath] | ||
if options.shared_zlib_includes: | ||
o['include_dirs'] += [options.shared_zlib_includes] | ||
|
||
|
||
def configure_http_parser(o): | ||
o['variables']['node_shared_http_parser'] = b(options.shared_http_parser) | ||
|
||
if options.shared_http_parser: | ||
o['libraries'] += ['-l%s' % options.shared_http_parser_libname] | ||
if options.shared_http_parser_libpath: | ||
o['libraries'] += ['-L%s' % options.shared_http_parser_libpath] | ||
if options.shared_http_parser_includes: | ||
o['include_dirs'] += [options.shared_http_parser_includes] | ||
|
||
|
||
def configure_libuv(o): | ||
o['variables']['node_shared_libuv'] = b(options.shared_libuv) | ||
|
||
if options.shared_libuv: | ||
o['libraries'] += ['-l%s' % options.shared_libuv_libname] | ||
if options.shared_libuv_libpath: | ||
o['libraries'] += ['-L%s' % options.shared_libuv_libpath] | ||
else: | ||
o['variables']['uv_library'] = 'static_library' | ||
|
||
if options.shared_libuv_includes: | ||
o['include_dirs'] += [options.shared_libuv_includes] | ||
def configure_library(lib, output): | ||
shared_lib = 'shared_' + lib | ||
output['variables']['node_' + shared_lib] = b(getattr(options, shared_lib)) | ||
|
||
if getattr(options, shared_lib): | ||
default_cflags = getattr(options, shared_lib + '_includes') | ||
default_lib = format_libraries(getattr(options, shared_lib + '_libname')) | ||
default_libpath = getattr(options, shared_lib + '_libpath') | ||
if default_libpath: | ||
default_libpath = '-L' + default_libpath | ||
(pkg_libs, pkg_cflags, pkg_libpath) = pkg_config(lib) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This will throw a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I always return a tuple -- "worst case" being There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My bad, confused |
||
cflags = pkg_cflags.split('-I') if pkg_cflags else default_cflags | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's not the way you should split stuff :). You're just asking for trouble. Say, I use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How do you suggest I split it? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. On whitespace, I guess. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We discussed that previously. What if paths have a space in them? Its a tricky situation. Most userland libraries that parses pkg-config output (there's a few in python, ruby, JavaScript and perl) chooses between There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Experienced users usually understand that naming a folders with a space in them is just asking for trouble, so it is commonly avoided. But nobody is explicitly avoiding There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd say There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The problem with splitting with space is that the first item won't strip its There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I just told you to prepend a single space, and it will work. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It won't work for the first result, since it wouldn't split Edit: (but would Edit2: Argh. Obvious whitespace issues here but I assume you know what I mean, leading whitespace being a problem. |
||
libs = pkg_libs if pkg_libs else default_lib | ||
libpath = pkg_libpath if pkg_libpath else default_libpath | ||
|
||
# libpath needs to be provided ahead libraries | ||
if libpath: | ||
output['libraries'] += [libpath] | ||
if libs: | ||
# libs passed to the linker cannot contain spaces. | ||
# (libpath on the other hand can) | ||
output['libraries'] += libs.split() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok. |
||
if cflags: | ||
output['include_dirs'] += [cflags] | ||
|
||
|
||
def configure_v8(o): | ||
|
@@ -718,25 +719,11 @@ def configure_v8(o): | |
def configure_openssl(o): | ||
o['variables']['node_use_openssl'] = b(not options.without_ssl) | ||
o['variables']['node_shared_openssl'] = b(options.shared_openssl) | ||
o['variables']['openssl_no_asm'] = ( | ||
1 if options.openssl_no_asm else 0) | ||
o['variables']['openssl_no_asm'] = 1 if options.openssl_no_asm else 0 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Any reason not to use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's not a boolean, its string true or false but the openssl build system requires 1 or 0. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok, my bad. |
||
|
||
if options.without_ssl: | ||
return | ||
|
||
if options.shared_openssl: | ||
(libs, cflags) = pkg_config('openssl') or ('-lssl -lcrypto', '') | ||
|
||
libnames = options.shared_openssl_libname.split(',') | ||
o['libraries'] += ['-l%s' % s for s in libnames] | ||
|
||
if options.shared_openssl_libpath: | ||
o['libraries'] += ['-L%s' % options.shared_openssl_libpath] | ||
|
||
if options.shared_openssl_includes: | ||
o['include_dirs'] += [options.shared_openssl_includes] | ||
else: | ||
o['cflags'] += cflags.split() | ||
configure_library('openssl', o) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Style: two newlines. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How about a separate pr with a full pep8 on configure? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I personally don't care that much about pep8. As long as changes are not too incongruent with existing code, they're fine by me. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok; was just looking through the eslint PR and thought we could improve configure as well. |
||
|
||
|
||
def configure_fullystatic(o): | ||
|
@@ -857,11 +844,14 @@ def configure_intl(o): | |
# ICU from pkg-config. | ||
o['variables']['v8_enable_i18n_support'] = 1 | ||
pkgicu = pkg_config('icu-i18n') | ||
if not pkgicu: | ||
if pkgicu[0] is None: | ||
print 'Error: could not load pkg-config data for "icu-i18n".' | ||
print 'See above errors or the README.md.' | ||
sys.exit(1) | ||
(libs, cflags) = pkgicu | ||
(libs, cflags, libpath) = pkgicu | ||
# libpath provides linker path which may contain spaces | ||
o['libraries'] += [libpath] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd say logically libpath belongs before libs :). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yep, you're right. |
||
# safe to split, cannot contain spaces | ||
o['libraries'] += libs.split() | ||
o['cflags'] += cflags.split() | ||
# use the "system" .gyp | ||
|
@@ -1020,9 +1010,9 @@ if (options.dest_os): | |
flavor = GetFlavor(flavor_params) | ||
|
||
configure_node(output) | ||
configure_libz(output) | ||
configure_http_parser(output) | ||
configure_libuv(output) | ||
configure_library('zlib', output) | ||
configure_library('http_parser', output) | ||
configure_library('libuv', output) | ||
configure_v8(output) | ||
configure_openssl(output) | ||
configure_winsdk(output) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Style: two newlines after functions.