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 all 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: 56 additions & 66 deletions configure
Original file line number Diff line number Diff line change
Expand Up @@ -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
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"""
libraries = list.split(',')
return ' '.join('-l{0}'.format(i) for i in libraries)


def try_check_compiler(cc, lang):
Expand Down Expand Up @@ -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)
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

# 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()
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]


def configure_v8(o):
Expand All @@ -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
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,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]
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.

# safe to split, cannot contain spaces
o['libraries'] += libs.split()
o['cflags'] += cflags.split()
# use the "system" .gyp
Expand Down Expand Up @@ -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)
Expand Down