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

Conversation

jbergstroem
Copy link
Member

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

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.
@jbergstroem
Copy link
Member Author

@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 pkg-config if available should always return accurate results since that's kind of its job.

@mscdex mscdex added the build Issues and PRs related to build files or the CI. label May 4, 2015

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.

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
<pmcclave@chromium.org>
@jbergstroem
Copy link
Member Author

@bnoordhuis addressed all issues besides how to handle cflags. Here's a patch I'm considering seeing that pkg-config returns -I/foo, whereas we [configure] assume user just provides path:

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.

@jbergstroem
Copy link
Member Author

@bnoordhuis any feedback on above? I guess I should additionally fetch -L from pkg-config for default_libpath?

return (libs, cflags)

pkg_config = os.environ.get('PKG_CONFIG_PATH',
os.environ.get('PKG_CONFIG', 'pkg-config'))
Copy link
Member

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)

@bnoordhuis
Copy link
Member

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.

@jbergstroem
Copy link
Member Author

Looks like this changelog entry:

2003-01-01  Zack Rusin  <zack@kde.org>

    * main.c (main): added --libs-only-other and --cflags-only-other
    arguments, thanks to which a more obscure dependencies can be
    retrieved, e.g. -pthread

Edit: actually, no. I went through the git repo and the initial git import dated 2001 seems to have it.

@jbergstroem
Copy link
Member Author

@bnoordhuis I don't mind reverting to the more generic --cflags (and --libs), but since I'm to split on -I it felt safer.

@bnoordhuis
Copy link
Member

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

@jbergstroem
Copy link
Member Author

@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
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?

@bnoordhuis
Copy link
Member

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

jbergstroem added a commit that referenced this pull request May 20, 2015
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 <info@bnoordhuis.nl>
@jbergstroem
Copy link
Member Author

Committed in 2b1c01c. Thanks for your review, @bnoordhuis – and thanks for the input @mgorny, @rlidwka.

Fishrock123 pushed a commit to Fishrock123/node that referenced this pull request May 21, 2015
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 <info@bnoordhuis.nl>
@rvagg rvagg mentioned this pull request May 23, 2015
andrewdeandrade pushed a commit to andrewdeandrade/node that referenced this pull request Jun 3, 2015
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 <info@bnoordhuis.nl>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants