-
Notifications
You must be signed in to change notification settings - Fork 29.8k
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
Conversation
Improve detection and usage of pkg-config. This simplifies the setup of all our shared libraries. If pkg-config is installed on the host and `--shared` flags are passed by the user, we try to get defaults from pkg-config instead of using the default provided by configure.
@bnoordhuis You previously suggested 'enabling' a shared library by providing any of the libpath, includedir or other flags. I still think that should be avoided. Using |
|
||
return (libs, cflags) | ||
return retval |
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.
PKG_CONFIG_PATH is the default path to pkg-config, while PKG_CONFIG is used as a result of a floating patch from Paul McClave <[email protected]>
@bnoordhuis addressed all issues besides how to handle cflags. Here's a patch I'm considering seeing that diff --git configure configure
index def2367..8ef0b38 100755
--- configure
+++ configure
@@ -347,9 +347,9 @@ def pkg_config(pkg):
os.environ.get('PKG_CONFIG', 'pkg-config'))
pkg_config_args = '--silence-errors'
retval = ()
- for flag in ['--libs', '--cflags']:
+ for flag in ['--libs-only-l', '--cflags-only-I']:
try:
- val = subprocess.check_output([pkg_config, pkg_config_args, flag, pkg])
+ val = subprocess.check_output([pkg_config, pkg_config_args, flag, pkg]).rstrip('\n')
except subprocess.CalledProcessError:
# most likely missing a .pc-file
val = None
@@ -691,12 +691,12 @@ def configure_library(lib, output):
(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
+ cflags = pkg_cflags.split("-I") if pkg_cflags else default_cflags
if libs:
output['libraries'] += libs.split()
if cflags:
- output['include_dirs'] += cflags.split()
+ output['include_dirs'] += [cflags]
if default_libpath:
output['libraries'] += ['-L%s' % default_libpath] I don't particularly like that we pass flags through libraries but not in include_dirs, but that's related to gyp I guess. |
@bnoordhuis any feedback on above? I guess I should additionally fetch |
return (libs, cflags) | ||
|
||
pkg_config = os.environ.get('PKG_CONFIG_PATH', | ||
os.environ.get('PKG_CONFIG', 'pkg-config')) |
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: arguments should line up. Maybe it reads slightly easier as:
pkg_config = 'pkg-config'
pkg_config = os.environ.get('PKG_CONFIG', pkg_config)
pkg_config = os.environ.get('PKG_CONFIG_PATH', pkg_config)
Do you happen to know when --cflags-only-I and --libs-only-L were added? They haven't always been around but I'm not sure when they first appeared. Compatibility may be an issue. |
Looks like this changelog entry:
Edit: actually, no. I went through the git repo and the initial git import dated 2001 seems to have it. |
@bnoordhuis I don't mind reverting to the more generic |
--cflags-only-I is fine for include_dirs. I was worried about general compiler flags ending up there. |
- fix outstanding styling issues brought up by @bnoordhuis - add libpath support through pkg-config - minor fixes to icu since we've modified pkg_config
@@ -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: |
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.
Actually, we probably want to check for libs here. I'll update.
@bnoordhuis ok, I've tried different configurations of shared libraries on different platforms now (including icu). Had to convert the output of pkg-config from bytes to string since I had issues with how gyp handled output printing. |
default_lib = format_libraries(getattr(options, shared_lib + '_libname')) | ||
default_libpath = getattr(options, shared_lib + '_libpath') | ||
if default_libpath: | ||
default_libpath = "-L" + default_libpath |
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.
Femto-style nit: can you use single quotes for consistency here and below?
LGTM with comments. |
# Please enter the commit message for your changes. Lines starting
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) |
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.
You're clobbering the executable with .pc file search paths :P.
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.
PKG_CONFIG is in there for backwards compatibility from the floating pkg-config patch and PKG_CONFIG_PATH is suggested by pkg-config upstream.
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.
Whaaat? PKG_CONFIG
is used by some build systems to point to pkg-config executable. PKG_CONFIG_PATH
contains search path for .pc files.
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.
I'll re-read the docs. A python library also added it to path which is why I considered it in the first place.
Improve detection and usage of pkg-config. This simplifies the setup of all our shared libraries. If pkg-config is installed on the host and `--shared` flags are passed by the user, we try to get defaults from pkg-config instead of using the default provided by configure. PR-URL: #1603 Reviewed-By: Ben Noordhuis <[email protected]>
Committed in 2b1c01c. Thanks for your review, @bnoordhuis – and thanks for the input @mgorny, @rlidwka. |
Improve detection and usage of pkg-config. This simplifies the setup of all our shared libraries. If pkg-config is installed on the host and `--shared` flags are passed by the user, we try to get defaults from pkg-config instead of using the default provided by configure. PR-URL: nodejs#1603 Reviewed-By: Ben Noordhuis <[email protected]>
Improve detection and usage of pkg-config. This simplifies the setup of all our shared libraries. If pkg-config is installed on the host and `--shared` flags are passed by the user, we try to get defaults from pkg-config instead of using the default provided by configure. PR-URL: nodejs/node#1603 Reviewed-By: Ben Noordhuis <[email protected]>
Improve detection and usage of pkg-config. This simplifies the setup of all our shared libraries.
If pkg-config is installed on the host and
--shared
flags are passed by the user, we try to get defaults from pkg-config instead of using the default provided by configure./R = @iojs/build, @bnoordhuis