Use pkg-config to find libxml2 and libcurl on Linux [SR-5603] #1152

kevints · 2017-08-01T22:34:41Z

This allows linking swift-corelibs-foundation to packages with flags
defined by pkg-config, rather than hardcoded ones. There are a couple
of structural changes as well, namely that pkg-config will now be
executed during configure rather than later by ninja. This seems
acceptable as it's how cmake and autoconf work, but means a reconfigure
will be required if any transitive dependency of a package we depend on changes.

The other structural change is that -lcurl will be passed
unconditionally on platforms without pkg-config support (technically the
pkg_config_required variable set). This makes curl a harder dependency
on those platforms, but that seems unavoidable given that it's the only
implementation of URLSession.

This allows linking swift-corelibs-foundation to packages with flags defined by `pkg-config`, rather than hardcoded ones. There are a couple of structural changes as well, namely that `pkg-config` will now be executed during `configure` rather than later by `ninja`. This seems acceptable as it's how cmake and autoconf work, but means a reconfigure will be required if any transitive dependency of a package we depend on files changes. The other structural change is that `-lcurl` will be passed unconditionally on platforms without pkg-config support (technically the `pkg_config_required` variable set). This makes curl a harder dependency on those platforms, but that seems unavoidable given that it's the only implementation of `URLSession`.

kevints · 2017-08-01T22:42:54Z

@parkera can you kick off CI for this change?

kevints · 2017-08-01T22:45:40Z

build.py

-	foundation.LDFLAGS += '-lcurl '
+if Configuration.current.requires_pkg_config:
+    pkg_config_dependencies = [
+        'icu-i18n',


These should probably specify a minimum package version (probably from Ubuntu 14.04 as it's the oldest supported target), WDYT?

There's no harm in doing that, I guess. Currently we link against the system libicu which has plusses and minuses.

kevints · 2017-08-01T22:54:36Z

build.py

+        swift_cflags += package.swiftc_flags
+else:
+	foundation.CFLAGS += ''.join([
+		'-I${SYSROOT}/usr/include/curl ',


I mechanically moved this from the previous unconditional block; however I'm not sure it ever did anything given that CF headers include <curl/curl.h> and this directory appears to contain curl.h and friends, without subdirectories. It also seems to have been repeated at least 4 times - I've condensed 3 of those into this line but I'm unsure about the 4th in the FreeBSD config (the pkg-plist seems to suggest that it wouldn't be needed there either but I don't have a system to test on).

ianpartridge · 2017-08-01T22:57:44Z

build.py

+        foundation.LDFLAGS += ' {} '.format(' '.join(package.ldflags))
+        swift_cflags += package.swiftc_flags
+else:
+	foundation.CFLAGS += ''.join([


Why don't these get added via package.cflags on line 89 above?

This is the else block of Configuration.current.requires_pkg_config, so these are hardcoded guess paths (moved from being unconditionally included on line 64-65 and other places). When we have pkg-config there's no reason to include them as they'll be included automatically by pkg-config <package_name> --libs and pkg-config <package_name> --cflags respectively. More importantly, if the PKG_CONFIG_PATH environment variable points to somewhere other than the system libraries and headers for these packages we won't consider them and will instead use the intended paths.

Got it, thanks.

alblue · 2017-08-01T22:57:59Z

@swift-ci please test

ianpartridge · 2017-08-01T23:03:39Z

https://bugs.swift.org/browse/SR-5608 doesn't exist?

kevints · 2017-08-01T23:06:30Z

@ianpartridge looks like the issue key is actually SR-5603, updated the title

ianpartridge · 2017-08-01T23:08:41Z

Thanks. I really like the look of this PR - thanks for working on it 👍

ianpartridge · 2017-08-02T07:46:44Z

@spevans how does this look to you?

spevans · 2017-08-02T08:01:57Z

build.py

 if triple == "armv7-none-linux-androideabi":
 	foundation.LDFLAGS += '-llog '
 else:
 	foundation.LDFLAGS += '-lpthread '

-foundation.LDFLAGS += '-ldl -lm -lswiftCore -lxml2 '
+foundation.LDFLAGS += '-ldl -lm -lswiftCore '


Is it possible to remove all of these flags? -lswiftCore and -ldl should be added by the swift compiler and I am also pretty sure that the libswiftCore already uses some math functions so -lm is probably getting added somewhere lower down if needed. I dont think -ldl is needed on FreeBSD anyway so a cleanup of these flags would be good.

These are the flags that get passed to the build for the CoreFoundation C library, which includes <math.h> and <dlfcn.h> so I believe -lm and -ldl should remain here in the spirit of include-what-you-use. I'm not sure about -lswiftCore as CoreFoundation doesn't appear to be importing any swift headers and swiftc should indeed be passing that flag, but maybe there is some magic I don't understand. I'll file a bug to follow up as I'd like to keep this change focused on the PR title.

-lswiftCore is actually added by the compiler for any swift program that is compiled. -ldl is a dependancy of libswiftCore.so so it gets picked up there. Math functions are also used by the stdlib and I think -lm is auto added by the C compiler (the same a -lc) since it is dependant on how a given C library is structured, ie if certain functions are broken out into other libraries

spevans · 2017-08-02T08:09:07Z

Overall this looks good. Being able to specify pkg-config for the other libraries will be useful if we need to build new versions of them.

The only thing to check is that an external ICU still builds correctly.

@kevints Could you run a local test build of swift and swift-corelibs-foundation with the --libicu --install-libicu options?

You will need to download ICU57.1 http://site.icu-project.org/download/57#TOC-ICU4C-Download (I think 59.1 has some issues)

Then untar it so that you have an icu directory at the same level as the other swift-corelibs-* directories. Then build swift with the icu options and check the tests work.

kevints · 2017-08-02T16:58:38Z

@spevans hmm, tests don't seem to pass against my local build with --libicu and --install-libicu due to

...swiftpm-linux-x86_64/x86_64-unknown-linux/release/swift-build-stage1: error while loading shared libraries: libicui18n.so.57: cannot open shared object file: No such file or directory

having a look at ldd it looks like both my system libicu (version 52) and the built icu (version 57) are both getting linked in but the 57 one is not found (since it isn't copied to the library runpath which is $ORIGIN/../lib/swift/linux, this might be a race between install and test invocation).

	libicuuc.so.52 => /usr/lib/x86_64-linux-gnu/libicuuc.so.52 (0x00007f3c9ca35000)
	libicui18n.so.52 => /usr/lib/x86_64-linux-gnu/libicui18n.so.52 (0x00007f3c9c62d000)
	/lib64/ld-linux-x86-64.so.2 (0x0000562e03598000)
	libicui18n.so.57 => not found
	libicuuc.so.57 => not found
	libicudata.so.57 => not found

This seems in line with what I'd expect having read through the foundation build - it unconditionally invokes pkg-config for icu, which will find the system one. This change doesn't change anything about that so I suspect this may have been broken for a while, if it ever worked at all (looking through the buildbot presets there doesn't appear to be CI for building on Linux with --libicu --install-libicu). I'll file a bug but I don't think the breakage is related to this change.

kevints · 2017-08-02T17:36:22Z

@spevans filed:

https://bugs.swift.org/browse/SR-5620
https://bugs.swift.org/browse/SR-5619
https://bugs.swift.org/browse/SR-5618

spevans · 2017-08-02T17:41:56Z

@kevints No worries, I'll take a look at the ICU stuff as I did it originally. What version of linux are you using?

kevints · 2017-08-02T17:49:01Z

@spevans saw those issues on Ubuntu 14.04

kevints · 2017-08-02T17:52:39Z

@spevans modulo those ICU issues does this change look good to you otherwise?

spevans · 2017-08-02T18:18:22Z

@kevints Yes this change looks ok to me. Im taking a look at the ICU issues but I'll deal with that separately - I think its because libxml2 is also dependant on ICU hence you seeing 2 versions.

I'll also look at cleaning up the linker flags, again that can be done as a separate PR.

spevans · 2017-08-04T12:00:00Z

@kevints I tested this PR whilst also building an external ICU and it was ok. I think the problem you saw may have been due to old config in the build directory - I did a clean build/test and it was ok.

The 2 versions is because ICU is built with versioned symbols and libxml2 uses the system ICU, that will need to be solved in the future.

phausler

overall looks reasonable to me

ianpartridge · 2017-08-12T13:37:11Z

@swift-ci please test and merge

kevints force-pushed the SR-5603 branch 2 times, most recently from 845b0d4 to b1e00d2 Compare August 1, 2017 22:36

kevints force-pushed the SR-5603 branch from b1e00d2 to b97db38 Compare August 1, 2017 22:39

kevints commented Aug 1, 2017

View reviewed changes

ianpartridge reviewed Aug 1, 2017

View reviewed changes

kevints changed the title ~~Use pkg-config to find libxml2 and libcurl on Linux [SR-5608]~~ Use pkg-config to find libxml2 and libcurl on Linux [SR-5603] Aug 1, 2017

ianpartridge requested a review from phausler August 2, 2017 07:46

spevans reviewed Aug 2, 2017

View reviewed changes

phausler approved these changes Aug 11, 2017

View reviewed changes

swift-ci merged commit a585dfc into swiftlang:master Aug 12, 2017

kevints mannequin mentioned this pull request Aug 1, 2017

[SR-5603] swift-corelibs-foundation should support finding dependencies via pkg-config on Linux #3822

Closed

kevints mannequin mentioned this pull request May 5, 2022

[SR-5619] --libicu and --install-libicu flags appear broken on Linux #3819

Closed

Use pkg-config to find libxml2 and libcurl on Linux [SR-5603] #1152

Use pkg-config to find libxml2 and libcurl on Linux [SR-5603] #1152

Uh oh!

Conversation

kevints commented Aug 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kevints commented Aug 1, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kevints Aug 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alblue commented Aug 1, 2017

Uh oh!

ianpartridge commented Aug 1, 2017

Uh oh!

kevints commented Aug 1, 2017

Uh oh!

ianpartridge commented Aug 1, 2017

Uh oh!

ianpartridge commented Aug 2, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

spevans commented Aug 2, 2017

Uh oh!

kevints commented Aug 2, 2017

Uh oh!

kevints commented Aug 2, 2017

Uh oh!

spevans commented Aug 2, 2017

Uh oh!

kevints commented Aug 2, 2017

Uh oh!

kevints commented Aug 2, 2017

Uh oh!

spevans commented Aug 2, 2017

Uh oh!

spevans commented Aug 4, 2017

Uh oh!

phausler left a comment

Choose a reason for hiding this comment

Uh oh!

ianpartridge commented Aug 12, 2017

Uh oh!

Uh oh!

kevints commented Aug 1, 2017 •

edited

Loading

kevints Aug 1, 2017 •

edited

Loading