build: refactor pkg-config for shared libraries #1603

jbergstroem · 2015-05-04T04:04:32Z

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 · 2015-05-04T04:08:01Z

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

bnoordhuis · 2015-05-04T11:50:36Z

configure


-  return (libs, cflags)
+  return retval


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 · 2015-05-05T00:02:14Z

@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 · 2015-05-11T10:40:46Z

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

bnoordhuis · 2015-05-11T15:49:59Z

configure

-  return (libs, cflags)
-
+  pkg_config = os.environ.get('PKG_CONFIG_PATH',
+    os.environ.get('PKG_CONFIG', 'pkg-config'))


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 · 2015-05-11T16:08:13Z

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 · 2015-05-11T22:26:33Z

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 · 2015-05-11T23:53:34Z

@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 · 2015-05-12T00:00:54Z

--cflags-only-I is fine for include_dirs. I was worried about general compiler flags ending up there.

@bnoordhuis

- fix outstanding styling issues brought up by @bnoordhuis - add libpath support through pkg-config - minor fixes to icu since we've modified pkg_config

jbergstroem · 2015-05-12T01:00:06Z

configure

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


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

jbergstroem · 2015-05-12T01:26:25Z

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

bnoordhuis · 2015-05-12T10:30:50Z

configure

+    default_lib = format_libraries(getattr(options, shared_lib + '_libname'))
+    default_libpath = getattr(options, shared_lib + '_libpath')
+    if default_libpath:
+      default_libpath = "-L" + default_libpath


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

bnoordhuis · 2015-05-12T10:35:06Z

LGTM with comments.

# Please enter the commit message for your changes. Lines starting

mgorny · 2015-05-18T04:28:38Z

configure

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


You're clobbering the executable with .pc file search paths :P.

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

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

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 <info@bnoordhuis.nl>

jbergstroem · 2015-05-20T22:06:54Z

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 <info@bnoordhuis.nl>

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>

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

bnoordhuis reviewed May 4, 2015
View reviewed changes

configure

return (libs, cflags)

return retval

Copy link

Member

bnoordhuis May 4, 2015

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.

jbergstroem added 5 commits May 5, 2015 09:23

fixup! simplify library uniqueness

38c6195

fixup! avoid one pkg-config call

4365f34

fixup! style

d5990d1

fixup! remove unneeded string replace

43feb45

fixup! check PKG_CONFIG_PATH as well

82e5af7

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>

bnoordhuis reviewed May 11, 2015
View reviewed changes

jbergstroem added 3 commits May 12, 2015 10:15

fixup: properly parse include headers

524de59

fixup: styling and libpath support

ac3df1b

- fix outstanding styling issues brought up by @bnoordhuis - add libpath support through pkg-config - minor fixes to icu since we've modified pkg_config

fixup: another styling nit

bb6697f

jbergstroem reviewed May 12, 2015
View reviewed changes

jbergstroem added 2 commits May 12, 2015 11:00

fixup: check for libs when using system-icu

1f99345

fixup: check_output returns bytes, not a string

dea139a

bnoordhuis reviewed May 12, 2015
View reviewed changes

fixup: style and document info about splitting lib/libpath

0350f14

# Please enter the commit message for your changes. Lines starting

mgorny reviewed May 18, 2015
View reviewed changes

jbergstroem added 4 commits May 18, 2015 15:17

fixup: provide libpath ahead of libraries

d8f4f6d

fixup: avoid using set for variable name

c5fed67

fix: Don't use PKG_CONFIG_PATH

1e1b008

fix: don't sort/unique libs

389a667

jbergstroem closed this May 20, 2015

rvagg mentioned this pull request May 23, 2015

Release proposal v2.1.0 #1777

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

build: refactor pkg-config for shared libraries #1603

build: refactor pkg-config for shared libraries #1603

jbergstroem commented May 4, 2015

jbergstroem commented May 4, 2015

bnoordhuis May 4, 2015

jbergstroem commented May 5, 2015

jbergstroem commented May 11, 2015

bnoordhuis May 11, 2015

bnoordhuis commented May 11, 2015

jbergstroem commented May 11, 2015

jbergstroem commented May 11, 2015

bnoordhuis commented May 12, 2015

jbergstroem May 12, 2015

jbergstroem commented May 12, 2015

bnoordhuis May 12, 2015

bnoordhuis commented May 12, 2015

mgorny May 18, 2015

jbergstroem May 18, 2015

mgorny May 18, 2015

jbergstroem May 18, 2015

jbergstroem commented May 20, 2015

build: refactor pkg-config for shared libraries #1603

build: refactor pkg-config for shared libraries #1603

Conversation

jbergstroem commented May 4, 2015

jbergstroem commented May 4, 2015

Choose a reason for hiding this comment

jbergstroem commented May 5, 2015

jbergstroem commented May 11, 2015

Choose a reason for hiding this comment

bnoordhuis commented May 11, 2015

jbergstroem commented May 11, 2015

jbergstroem commented May 11, 2015

bnoordhuis commented May 12, 2015

Choose a reason for hiding this comment

jbergstroem commented May 12, 2015

Choose a reason for hiding this comment

bnoordhuis commented May 12, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbergstroem commented May 20, 2015