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 12 commits
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
122 changes: 58 additions & 64 deletions configure
Original file line number Diff line number Diff line change
Expand Up @@ -343,17 +343,30 @@ 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 = 'pkg-config'
pkg_config = os.environ.get('PKG_CONFIG', pkg_config)
pkg_config = os.environ.get('PKG_CONFIG_PATH', pkg_config)
Copy link

Choose a reason for hiding this comment

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

You're clobbering the executable with .pc file search paths :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.

PKG_CONFIG is in there for backwards compatibility from the floating pkg-config patch and PKG_CONFIG_PATH is suggested by pkg-config upstream.

Copy link

Choose a reason for hiding this comment

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

Whaaat? PKG_CONFIG is used by some build systems to point to pkg-config executable. PKG_CONFIG_PATH contains search path for .pc files.

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'll re-read the docs. A python library also added it to path which is why I considered it in the first place.

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
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"""
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):
Expand Down Expand Up @@ -672,40 +685,29 @@ 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_library(lib, output):
shared_lib = 'shared_' + lib
output['variables']['node_' + shared_lib] = b(getattr(options, shared_lib))

def configure_http_parser(o):
o['variables']['node_shared_http_parser'] = b(options.shared_http_parser)
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)
Copy link

Choose a reason for hiding this comment

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

This will throw a ValueError (or sth like that) when you return None instead of the tuple.

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 always return a tuple -- "worst case" being (None, None, None)

Copy link

Choose a reason for hiding this comment

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

My bad, confused val = with retval =.

cflags = pkg_cflags.split('-I') if pkg_cflags else default_cflags
Copy link

Choose a reason for hiding this comment

The 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 /home/Foo-Ibn prefix.

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 do you suggest I split it?

Copy link

Choose a reason for hiding this comment

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

On whitespace, I guess.

Copy link
Member Author

Choose a reason for hiding this comment

The 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 -I and space. My call (and bnoordhuis suggestion) was that -I probably would be less common.

Copy link
Contributor

Choose a reason for hiding this comment

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

-I would also be less expected.

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 -I in their paths.

Copy link

Choose a reason for hiding this comment

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

I'd say -I (space + -I) would be a safer compromise. Just prepend a space to get clean split :).

Copy link
Member Author

Choose a reason for hiding this comment

The 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 -I. I'd have to do some extra logic for that to work as intended. I think -I is rare enough. If we get a bug filed, I'll revisit.

Copy link

Choose a reason for hiding this comment

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

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 won't work for the first result, since it wouldn't split -Ifoo

Edit: (but would -Ifoo) -- meaning the first result would have to be additionally formatted as I mentioned above.

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

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]
if libs:
# libs passed to the linker cannot contain spaces.
# (libpath on the other hand can)
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]
if libpath:
output['libraries'] += [libpath]
Copy link
Member

Choose a reason for hiding this comment

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

Style nit: two newlines.



def configure_v8(o):
Expand All @@ -718,25 +720,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
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):
Expand Down Expand Up @@ -857,12 +845,15 @@ 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
# safe to split, cannot contain spaces
o['libraries'] += libs.split()
# libpath provides linker path which may contain spaces
o['libraries'] += [libpath]
Copy link

Choose a reason for hiding this comment

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

I'd say logically libpath belongs before libs :).

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, you're right.

o['cflags'] += cflags.split()
# use the "system" .gyp
o['variables']['icu_gyp_path'] = 'tools/icu/icu-system.gyp'
Expand Down Expand Up @@ -1020,15 +1011,18 @@ 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
output['libraries'] = list(set(output['libraries']))
Copy link

Choose a reason for hiding this comment

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

Dangerous. You reorder libraries, you ask for symbol resolution failures.

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, you're right. Ignore duplicates?

Copy link

Choose a reason for hiding this comment

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

Yes. Just pass whatever you get. Don't do magic, duplicates don't hurt.


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