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.

Sign up for GitHub

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
wants to merge 16 commits into from
Closed

build: refactor pkg-config for shared libraries #1603

Changes from 8 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
e7ce51e
build: refactor pkg-config for shared libraries
jbergstroem May 4, 2015
38c6195
fixup! simplify library uniqueness
jbergstroem May 4, 2015
4365f34
fixup! avoid one pkg-config call
jbergstroem May 4, 2015
d5990d1
fixup! style
jbergstroem May 4, 2015
43feb45
fixup! remove unneeded string replace
jbergstroem May 4, 2015
82e5af7
fixup! check PKG_CONFIG_PATH as well
jbergstroem May 4, 2015
524de59
fixup: properly parse include headers
jbergstroem May 5, 2015
ac3df1b
fixup: styling and libpath support
jbergstroem May 12, 2015
bb6697f
fixup: another styling nit
jbergstroem May 12, 2015
1f99345
fixup: check for libs when using system-icu
jbergstroem May 12, 2015
dea139a
fixup: check_output returns bytes, not a string
jbergstroem May 12, 2015
0350f14
fixup: style and document info about splitting lib/libpath
jbergstroem May 14, 2015
d8f4f6d
fixup: provide libpath ahead of libraries
jbergstroem May 18, 2015
c5fed67
fixup: avoid using set for variable name
jbergstroem May 18, 2015
1e1b008
fix: Don't use PKG_CONFIG_PATH
jbergstroem May 18, 2015
389a667
fix: don't sort/unique libs
jbergstroem May 18, 2015
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: 54 additions & 68 deletions configure
View file
Open in desktop
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 = '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]).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,41 +683,27 @@ 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
Copy link
Member

Choose a reason for hiding this comment

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

Femto-style nit: can you use single quotes for consistency here and below?

(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
libs = pkg_libs if pkg_libs else default_lib
libpath = pkg_libpath if pkg_libpath else default_libpath

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]
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):
o['variables']['v8_enable_gdbjit'] = 1 if options.gdb else 0
Expand All @@ -718,26 +715,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):
if options.fully_static:
Expand Down Expand Up @@ -857,12 +839,13 @@ 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[1] == None:
Copy link
Member Author

Choose a reason for hiding this comment

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

Actually, we probably want to check for libs here. I'll update.

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
o['libraries'] += libs.split()
o['libraries'] += libpath.split()
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a similar comment about .split() as requested above?

Copy link
Member Author

Choose a reason for hiding this comment

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

Will do, this is wrong though -- libpath needs to be treated as a string similar to pkg-config. I might look at refactoring this to using configure_library instead.

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