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: refactor pkg-config for shared libraries #1603

Closed
Closed
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
116 changes: 53 additions & 63 deletions configure
Original file line number Diff line number Diff line change
Expand Up @@ -343,18 +343,31 @@ 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
pkg_config = os.environ.get('PKG_CONFIG', 'pkg-config')
pkg_config_args = '--silence-errors'
try:
subprocess.check_output([pkg_config, '--version'])
Copy link
Member

Choose a reason for hiding this comment

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

Is this explicit 'is pkg-config installed?' check necessary? Perhaps you can fold it into the subprocess call below? The net result - a (None, None) tuple - should be the same.

Copy link
Member Author

Choose a reason for hiding this comment

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

My thought was to avoid one call if pkg-config wasn't installed. I'll have a go at improving this.

except OSError:
# no pkg-config/pkgconf installed
return (None, None)

retval = ()
for flag in ['--libs', '--cflags']:
try:
val = subprocess.check_output([pkg_config, pkg_config_args, flag, pkg])
except subprocess.CalledProcessError:
# most likely missing a .pc-file
val = None

cmd = os.popen('pkg-config --cflags %s' % pkg, 'r')
cflags = cmd.readline().strip()
ret = cmd.close()
if (ret): return None
retval += (val,)

return (libs, cflags)
return retval
Copy link
Member

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.


def format_libraries(list):
Copy link
Member

Choose a reason for hiding this comment

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

Style: two newlines between functions.

"""Returns string of space separated libraries"""
list = list.replace(' ', '')
Copy link
Member

Choose a reason for hiding this comment

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

Why are you stripping whitespace here?

Copy link
Member Author

Choose a reason for hiding this comment

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

For consistency. Guess its cosmetics.

set = list.split(',')
Copy link

Choose a reason for hiding this comment

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

set is a type name, I'd avoid that.

Copy link
Member Author

Choose a reason for hiding this comment

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

good point.

return ' '.join('-l{0}'.format(i) for i in set)
Copy link

Choose a reason for hiding this comment

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

You're using 'x' + y and 'x{0}'.format(y) inconsistently in the added code. I'd say decide on one :).

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, was mixed for readability (based on feedback by bnoordhuis). I previously had format everywhere.


def try_check_compiler(cc, lang):
try:
Expand Down Expand Up @@ -672,41 +685,25 @@ 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_%s' % lib
output['variables']['node_%s' % shared_lib] = b(getattr(options, shared_lib))
Copy link
Member

Choose a reason for hiding this comment

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

Just a note but you can write this as 'shared_' + lib and 'node_' + shared_lib as well. I'll leave it up to you to decide what's more readable.


if getattr(options, shared_lib):
default_lib = format_libraries(getattr(options, '%s_libname' % shared_lib))
default_libpath = getattr(options, '%s_libpath' % shared_lib)
default_cflags = getattr(options, '%s_includes' % shared_lib)

(pkg_libs, pkg_cflags) = pkg_config(lib)
libs = pkg_libs if pkg_libs else default_lib
cflags = pkg_cflags if pkg_cflags else default_cflags

if libs:
output['libraries'] += libs.split()
Copy link
Member

Choose a reason for hiding this comment

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

If libs.split() is always safe here (because it's always in the form of -lfoo -lbar) can you add a comment explaining that?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok.

if cflags:
output['include_dirs'] += cflags.split()
Copy link
Member

Choose a reason for hiding this comment

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

Apropos the libs.split() and cflags.split() calls, what happens when they contain paths with spaces?

Copy link
Member

Choose a reason for hiding this comment

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

Also, I'm not 100% confident it's correct to assign the result of cflags.split() to include_dirs.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point. Split is unsafe.

Copy link
Member

Choose a reason for hiding this comment

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

Looking at it again, I don't think include_dirs is the right field, it should be cflags or cflags_cc. Probably the former.

if default_libpath:
output['libraries'] += ['-L%s' % default_libpath]

def configure_v8(o):
o['variables']['v8_enable_gdbjit'] = 1 if options.gdb else 0
Expand All @@ -718,26 +715,12 @@ 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
Copy link

Choose a reason for hiding this comment

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

Any reason not to use b() thingie here? Just guessing it goes for boolean, though I can't grep for it right now :P.

Copy link
Member Author

Choose a reason for hiding this comment

The 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.

Copy link

Choose a reason for hiding this comment

The 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)
Copy link
Member

Choose a reason for hiding this comment

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

Style: two newlines.

Copy link
Member Author

Choose a reason for hiding this comment

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

How about a separate pr with a full pep8 on configure?

Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Member Author

Choose a reason for hiding this comment

The 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):
if options.fully_static:
Expand Down Expand Up @@ -1020,15 +1003,22 @@ 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)
configure_intl(output)
configure_fullystatic(output)

# remove duplicates from libraries
unique_libraries = []
for library in output['libraries']:
if library not in unique_libraries:
unique_libraries.append(library)
output['libraries'] = unique_libraries
Copy link
Member

Choose a reason for hiding this comment

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

Maybe just output['libraries'] = list(set(output['libraries']))?


# variables should be a root level element,
# move everything else to target_defaults
variables = output['variables']
Expand Down