scylladb / seastar

High performance server-side application framework
http://seastar.io
Apache License 2.0
8.35k stars 1.54k forks source link

seastar.pc: Neither "--static" nor without "--static" does the right thing #579

Closed nyh closed 5 years ago

nyh commented 5 years ago

When I try to build a Seastar application with

$ c++ getting-started.cc `pkg-config --cflags --libs --static ./build/release/seastar.pc`

The linking fails when trying to find /usr/lib64/libunistring.so, -ltspi and -lidn2. I do have all three libraries installed on my machine, but not the development package (libunistring-devel et al) but just the runtime package. For example, this runtime package includes /usr/lib64/libunistring.so.2, but NOT a symbolic link /usr/lib64/libunistring.so.

That should be enough - Seastar doesn't use any of these packages directly, it just uses GNU TLS, and cmake decied that that needs these libunistring.so, -ltspi and -lidn2. But since GNU TLS is a shared library, it will bring its own dependencies with it and it knows their full path (e.g. /usr/lib64/libunistring.so.2) - see ldd /usr/lib64/libgnutls.so - and does NOT need the symlink from the development packages to be installed.

Note that build without "--static" doesn't work either, it misses, for example, "-lrt".

nyh commented 5 years ago

The more I think about it, I think requiring the user to use "--static" is wrong. The user doesn't even know that Seastar is a static library, and certainly doesn't know whether any of its dependencies are in turn static, or not static.

I think that Seastar knows that it itself is a static library, so it should add as normal (not "--static") dependencies whatever someone would need to add to the link line to build with Seastar. For example, "-ltr" or "-lgnutls", but not further dependencies of those whenever those are shared libraries.

I know we're trying to be very clever and support also a case where gnutls itself may be a static library, and require the link line to also include its own dependencies. But that is not the usual case. The usual case is that gnutls is installed on the system, as a shared library, and all our cleverness breaks in this normal case.

By the way, gnutls itself has a pkg-config. E.g.,

$ pkg-config --libs gnutls
-lgnutls 

We could have used that if we just assume that the user needs to have gnutls installed on the system as "normal" users would, but unfortunately we can't do this if we want to support all the super-complicated cases supported by our cmake and cooking stuff.

hakuch commented 5 years ago

As I have demonstrated with this CMake example, --static is necessary because it changes if dependencies are included transitively or not. It is necessary to know whether Seastar is static for this reason.

nyh commented 5 years ago

But as I explained above, "--static" does the wrong thing, and in practice - does not work.... We **don't want*** to include transitive dependencies of shared, non-static, dependencies (such as libgnutls), because then the compilation doesn't work. Think about this: The fact someone can compile with "-lgnutls" and libgnutls.so.2 depends on libidn2.so.2, does NOT mean that someone can compile explicitly with "-lidn2" (because libidn2.so.2 is available, but the libidn2.so symlink is not). Moreover it is not needed, because libgnutls is a shared library, even if Seastar is not.

Please try "rpm -e libunistring-devel" (a Fedora example) - don't worry - you'll never miss it - and then try the build line I gave above, and you'll see the compilation error for yourself. Maybe the reason why you never noticed this problem is because as soon as you had these errors, you just installed libunistring-devel, libidn2-devel, etc., not thinking twice why those are actually needed (and they're not).

hakuch commented 5 years ago

But as I explained above, "--static" does the wrong thing, and in practice - does not work....

I don't know what you mean here. Are you implying that pkg-config is inherently broken, that Seastar's use of it is broken, or that Seastar's dependencies' use of it is broken?

We **don't want*** to include transitive dependencies of shared, non-static, dependencies (such as libgnutls), because then the compilation doesn't work. Think about this: The fact someone can compile with "-lgnutls" and libgnutls.so.2 depends on libidn2.so.2, does NOT mean that someone can compile explicitly with "-lidn2" (because libidn2.so.2 is available, but the libidn2.so symlink is not). Moreover it is not needed, because libgnutls is a shared library, even if Seastar is not.

What you're describing sounds like an issue with the pkg-config specification of GnuTLS (an upstream issue). If libdn2 is a private dependency which can be found arbitrarily, then GnuTLS needs to structure its pkg-config as such.

If that's not what you're describing, then I would ask that you elaborate, because I'm having trouble understanding.

Please try "rpm -e libunistring-devel" (a Fedora example) - don't worry - you'll never miss it - and then try the build line I gave above, and you'll see the compilation error for yourself. Maybe the reason why you never noticed this problem is because as soon as you had these errors, you just installed libunistring-devel, libidn2-devel, etc., not thinking twice why those are actually needed (and they're not).

I try not to make assumptions about what people are thinking, as a rule.

nyh commented 5 years ago

Another thing: one might say that the problem exists already in the cmake and not just in the seastar.pc file.:

Cmake knows that Seastar needs libgnutls to compile (and use), and that is true. But for some reason it decided to figure out that this libgnutls "transitively" further needs other stuff to link: "/usr/lib64/libgnutls.so", "-lidn2", "-ltspi". This wrong - to link with libgnutls.so (if it is a shared library), you DON'T need those three things in the link line ever. Regardless of whether Seastar is shared or static, Because libgnutls is static.

Something you do need to exist (but don't need on the link line!) is libgnutls.so's DT_NEEDED (i.e., ldd) dependencies, which are /usr/lib64/libgnutls.so.2 (not .so), /lib64/libidn2.so.0 (again, not "-lidnd2", which may not work at all because of a missing symlink), and it doesn't need "-ltspi" at all (I can't find any evidence that this library is needed at link time at all.

nyh commented 5 years ago

@haaawk replying to your second question (I hope my previous comment answer the other ones, and please do try this yourself what I suggested and fails - instead of saying that I'm making assumptions about what you did.

No, I think gnutls.pc is not broken:

$ pkg-config --libs gnutls
-lgnutls 
$ pkg-config --libs --static gnutls
-lgnutls -ltspi -lgmp /usr/lib64/libunistring.so -lidn2 -lnettle -lhogweed -lgmp -lnettle -ltasn1 -lp11-kit 

As you can see, gnutls is correctly telling us that if we are compiling with gnutls is a shared library, only "-lgnutls" is needed, and that's it. But if, and only if, you want to compile with the static version of gnutls, then you need all this other stuff (libunistring.so et al.) on the link line.

Although Seastar is a static library, the application is not compiled statically (with "-static" link option), and "-lgnutls" will pick up the shared version of this library installed on my system - not the static library. So it is wrong for the application to use the "--static" version of gnutls's dependencies. It needs to use the non-static version, with just "-lgnutls".

This is why I suggested that our use of "--static" is wrong, and was not intended for what we used it for. In fact, in the "pc" manual page, there is:

     Libs.private  Required linking flags for this package that are only required when
                   linking statically.  Libraries this package depends on for linking
                   against it statically, which are not described as dependencies
                   should be specified here.  (optional; fragment list)

Note the phrase "linking statically". I believe it doesn't say that this library is static, but rather that the linking is static (gcc -static).

I'll open an upstream bug report on why they used "libunistring.so" instead of "-lunistring" in libgnutls.pc. But that's unrelated to our issue, which we don't need all these libraries at all.

Finally, if you forget for a moment our opinions and beliefs, the current seastar.pc simply doesn't work (without installing a bunch of completely unnecessary packages, which our install-dependencies.sh doesn't). It needs to be fixed, not defended.

hakuch commented 5 years ago

@haaawk replying to your second question (I hope my previous comment answer the other ones, and please do try this yourself what I suggested and fails - instead of saying that I'm making assumptions about what you did.

Quote:

Maybe the reason why you never noticed this problem is because as soon as you had these errors, > you just installed libunistring-devel, libidn2-devel, etc., not thinking twice why those are actually needed > (and they're not).

No, I think gnutls.pc is not broken:

$ pkg-config --libs gnutls
-lgnutls 
$ pkg-config --libs --static gnutls
-lgnutls -ltspi -lgmp /usr/lib64/libunistring.so -lidn2 -lnettle -lhogweed -lgmp -lnettle -ltasn1 -lp11-kit 

As you can see, gnutls is correctly telling us that if we are compiling with gnutls is a shared library, only "-lgnutls" is needed, and that's it. But if, and only if, you want to compile with the static version of gnutls, then you need all this other stuff (libunistring.so et al.) on the link line.

Although Seastar is a static library, the application is not compiled statically (with "-static" link option), and "-lgnutls" will pick up the shared version of this library installed on my system - not the static library. So it is wrong for the application to use the "--static" version of gnutls's dependencies. It needs to use the non-static version, with just "-lgnutls".

This is why I suggested that our use of "--static" is wrong, and was not intended for what we used it for. In fact, in the "pc" manual page, there is:

     Libs.private  Required linking flags for this package that are only required when
                   linking statically.  Libraries this package depends on for linking
                   against it statically, which are not described as dependencies
                   should be specified here.  (optional; fragment list)

Note the phrase "linking statically". I believe it doesn't say that this library is static, but rather that the linking is static (gcc -static).

I'll open an upstream bug report on why they used "libunistring.so" instead of "-lunistring" in libgnutls.pc. But that's unrelated to our issue, which we don't need all these libraries at all.

Finally, if you forget for a moment our opinions and beliefs, the current seastar.pc simply doesn't work (without installing a bunch of completely unnecessary packages, which our install-dependencies.sh doesn't). It needs to be fixed, not defended.

Please indicate anything I have said which is defensive.

In any case, I will respond to your actual technical points after I've thought about them a bit.

haaawk commented 5 years ago

@nyh did I get mixed into a fight without event knowing?

espindola commented 5 years ago

I think this is a missing feature in pkg-config. We can report a bug, but for now we have to live with it.

pkg-config has two modes

The missing feature is to check the assumption against which libraries are actually available. If only dynamic or only static are available, it would be nice for it to figure that out and use that mode when processing that dependency subtree.

With that extra feature, the scylla build would still use --static since it wants to static link seastar into scylla, but it would not be required of every user.

Currently we only support building seastar as a static library, so we unfortunately have to ask pkg-config to assume that everything is static.

I think it would be wrong to lie to pkg-config by listing static dependencies as shared. We will one day support dynamic linking and seastar should not behave as special case relative to all other libraries.

espindola commented 5 years ago

Looking at the pkg-config there is a good workaround: --maximum-traverse-depth=2

It is a valid workaround under the assumption that seastar is the one static library that will be used and everything else is shared. With current master I get:

% pkg-config --libs seastar.pc /usr/lib64/libboost_program_options.so /usr/lib64/libboost_system.so /usr/lib64/libboost_thread.so /usr/lib64/libcares.so /usr/lib64/libcryptopp.so /usr/lib64/libfmt.so.5.2.1 /home/espindola/scylla/scylla/seastar/build/libseastar.a -llz4 % pkg-config --libs --static seastar.pc /usr/lib64/libboost_program_options.so /usr/lib64/libboost_system.so /usr/lib64/libboost_thread.so /usr/lib64/libcares.so /usr/lib64/libcryptopp.so /usr/lib64/libfmt.so.5.2.1 /home/espindola/scylla/scylla/seastar/build/libseastar.a -ldl -lrt /usr/lib64/libboost_filesystem.so /usr/lib64/libboost_system.so /usr/lib64/libboost_thread.so /usr/lib64/libsctp.so /usr/lib64/libnuma.so -latomic -lstdc++fs -llz4 -lgnutls -ltspi -lgmp /usr/lib64/libunistring.so -lidn2 -lnettle -lhogweed -lgmp -lnettle -ltasn1 -lp11-kit -lprotobuf -lpthread -lz -lhwloc -lm -lnuma -lltdl -lpthread -ldl -lyaml-cpp % pkg-config --libs --static --maximum-traverse-depth=2 seastar.pc /usr/lib64/libboost_program_options.so /usr/lib64/libboost_system.so /usr/lib64/libboost_thread.so /usr/lib64/libcares.so /usr/lib64/libcryptopp.so /usr/lib64/libfmt.so.5.2.1 /home/espindola/scylla/scylla/seastar/build/libseastar.a -ldl -lrt /usr/lib64/libboost_filesystem.so /usr/lib64/libboost_system.so /usr/lib64/libboost_thread.so /usr/lib64/libsctp.so /usr/lib64/libnuma.so -latomic -lstdc++fs

hakuch commented 5 years ago

I'm preparing a reproducible example to clarify the issue for myself (and hopefully others).

Looking at the pkg-config there is a good workaround: --maximum-traverse-depth=2

It is a valid workaround under the assumption that seastar is the one static library that will be used and everything else is shared. With current master I get:

That's a fairly restrictive assumption on how Seastar is configured and deployed, no? If so, that may be fine for integrating Seastar into a particular program in a particular environment, but not something we should make any kind of default.

% pkg-config --libs seastar.pc /usr/lib64/libboost_program_options.so /usr/lib64/libboost_system.so /usr/lib64/libboost_thread.so /usr/lib64/libcares.so /usr/lib64/libcryptopp.so /usr/lib64/libfmt.so.5.2.1 /home/espindola/scylla/scylla/seastar/build/libseastar.a -llz4 % pkg-config --libs --static seastar.pc /usr/lib64/libboost_program_options.so /usr/lib64/libboost_system.so /usr/lib64/libboost_thread.so /usr/lib64/libcares.so /usr/lib64/libcryptopp.so /usr/lib64/libfmt.so.5.2.1 /home/espindola/scylla/scylla/seastar/build/libseastar.a -ldl -lrt /usr/lib64/libboost_filesystem.so /usr/lib64/libboost_system.so /usr/lib64/libboost_thread.so /usr/lib64/libsctp.so /usr/lib64/libnuma.so -latomic -lstdc++fs -llz4 -lgnutls -ltspi -lgmp /usr/lib64/libunistring.so -lidn2 -lnettle -lhogweed -lgmp -lnettle -ltasn1 -lp11-kit -lprotobuf -lpthread -lz -lhwloc -lm -lnuma -lltdl -lpthread -ldl -lyaml-cpp % pkg-config --libs --static --maximum-traverse-depth=2 seastar.pc /usr/lib64/libboost_program_options.so /usr/lib64/libboost_system.so /usr/lib64/libboost_thread.so /usr/lib64/libcares.so /usr/lib64/libcryptopp.so /usr/lib64/libfmt.so.5.2.1 /home/espindola/scylla/scylla/seastar/build/libseastar.a -ldl -lrt /usr/lib64/libboost_filesystem.so /usr/lib64/libboost_system.so /usr/lib64/libboost_thread.so /usr/lib64/libsctp.so /usr/lib64/libnuma.so -latomic -lstdc++fs

nyh commented 5 years ago

On Mon, Jan 14, 2019 at 6:16 PM Rafael Ávila de Espíndola < notifications@github.com> wrote:

I think this is a missing feature in pkg-config. We can report a bug, but for now we have to live with it.

pkg-config has two modes

  • with no extra options, it assumes dynamic linking of everything
  • with --static it assumes static linking of everything

I'm really not an expert pkg-config philosopher, but the way I see it now (please point me to better explanations, if you can find them), I think the two options are slightly different from what you said:

  1. With no extra options, it assumes normal linking of everything. Just normal gcc behavior of preferring shared libraries where they exist, but using a static library when that is the only one that exists (like for Seastar).

  2. With "--static" it assumes static linking of everything (as you said), i.e., gcc -static will be used.

I think option 1 is exactly what we need. Because we build libseastar.a ourselves, we know that it is only a static library, so normal linking with it will require - always - also linking its direct dependencies, like "-lgnutls". So, Seastar normal libs (not private libs) need to include the output of pkg-config --libs gnutls. But not the output ofpkg-config --libs --static gnutls! In other words, on a common system where gnutls was installed as a shared library, pkg-config --libs gnutls will include just "-lgnutls" (and nothing else), and that will be added to Seastar's normal (not private) libs line.

For the "libs.private" line, we can add the output of pkg-config --libs --static gnutls. This will allow linking with a static everything (if you're lucky enough to have static versions of all the libraries... usually you don't).

The missing feature is to check the assumption against which libraries are actually available. If only dynamic or only static are available, it would be nice for it to figure that out and use that mode when processing that dependency subtree.

My thinking is that if a system has just a static version of gnutls.a (and in this millennium, this is definitely not the norm), its pkg-config --libs gnutls would have to list its own dependencies! Otherwise, no application which uses pkg-config --libs gnutls would work as expected. So if we just use pkg-config --libs gnutls we'll be fine.

Or, if there's a different cmake mechanism instead of the good-old-pkg-config we can use that. As long as we can figure out that just "-lgnutls" is what we need to actually compile with libgnutls.

With that extra feature, the scylla build would still use --static since it wants to static link seastar into scylla, but it would not be required of every user.

We only have a static version of Seastar... If we had both, I wonder, would we have any good reason to use the static library and not the shared one? Even TLS performance can be improved with initial-exec TLS.

Currently we only support building seastar as a static library, so we unfortunately have to ask pkg-config to assume that everything is static.

I'm starting to doubt this is the case. And clearly it doesn't work without installing a bunch on unnecessary packages just so we can specify a bunch of unnecessary library names on the command line.

I think it would be wrong to lie to pkg-config by listing static dependencies as shared. We will one day support dynamic linking and seastar should not behave as special case relative to all other libraries.

I still don't understand why we think that the question whether Seastar itself is static or shared, has any implication on whether application's should need to specify the transitive dependencies of Seastar explicitly (e.g., -lidn2 because it's required by libgnutls required by Seastar). The question of whether we need, or don't need, to include -lidn2, doesn't have to do with whether Seastar is shared, but rather whether gnutls is shared.

espindola commented 5 years ago

That's a fairly restrictive assumption on how Seastar is configured and deployed, no? If so, that may be fine for integrating Seastar into a particular program in a particular environment, but not something we should make any kind of default.

Yes, it is really just a workaround if someone really don't want to have extra symbolic links.

hakuch commented 5 years ago

@nyh:

I believe the example here is equivalent to the one we are discussing here, and I have amended it with generated pkg-config files.

Therefore, if you can identify the issue and/or explain the error in the following snippets, it would clarify for me what you think the problem is.

To review:

Here's the generated vector.pc on my machine:

includedir=/home/jhaberku/src/transitivity/vector/include
libdir=/home/jhaberku/src/transitivity/build

Name: vector
Description: Example shared library that is used privately.
Version: 0.1.0
Cflags: -I${includedir}
Libs: ${libdir}/libvector.so

and the generated seastar.pc (here, I configured Seastar to be static, but I will show both cases in the example):

includedir=/home/jhaberku/src/transitivity/seastar/include
libdir=/home/jhaberku/src/transitivity/build

Name: seastar
Description: Example shared or static library that is used publicly.
Version: 0.1.0
Requires.private: vector >= 0.1.0
Cflags: -I${includedir}
Libs: ${libdir}/libseastar.a

Examples

I'm going to drop the path prefix of /home/jhaberku/src/transitivity from the output for clarity.

When Seastar is a shared library

foo needs to link against seastar.

$ pkg-config --libs --cflags ./seastar.pc
-Iseastar/include -Ivector/include build/libseastar.so

It compiles.

$ g++ ../foo/main.cc $(pkg-config --libs --cflags ./seastar.pc) -o foo_pc

bar also depends on vector.

$ g++ ../bar/main.cc $(pkg-config --libs --cflags ./seastar.pc) -o bar_pc
/tmp/ccyYLZQh.o:main.cc:function main: error: undefined reference to 'vector::magic(int)'
collect2: error: ld returned 1 exit status

So let's indicate this to pkg-config:

$ pkg-config --libs --cflags ./seastar.pc ./vector.pc
-Iseastar/include -Ivector/include build/libseastar.so build/libvector.so

It now compiles.

$ g++ ../bar/main.cc $(pkg-config --libs --cflags ./seastar.pc ./vector.pc) -o bar_pc

Seastar as a static library

Without --static:

$ pkg-config --libs --cflags ./seastar.pc
-Iseastar/include -Ivector/include build/libseastar.a 

Does it work?

$ g++ ../foo/main.cc $(pkg-config --libs --cflags ./seastar.pc) -o foo_pc
/home/jhaberku/src/transitivity/build/libseastar.a(greet.cc.o):greet.cc:function seastar::greet(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&): error: undefined reference to 'vector::magic(int)'
collect2: error: ld returned 1 exit status

With --static:

$ pkg-config --libs --cflags --static ./seastar.pc
-Iseastar/include -Ivector/include build/libseastar.a build/libvector.so
$ g++ ../foo/main.cc $(pkg-config --libs --cflags --static ./seastar.pc) -o foo_pc

Let's try with bar now (also has a dependency on vector):

$ pkg-config --libs --cflags ./seastar.pc ./vector.pc
-Iseastar/include -Ivector/include build/libseastar.a build/libvector.so

Aha! We have specified ./vector.pc because it's a direct dependency of bar, but it also happened to satisfy seastar's private dependency on vector so the compilation will succeed. This is just a coincidence in this case, and it reflects "leaking" information.

But we can still call --static and get the same thing:

$ pkg-config --libs --cflags --static ./seastar.pc ./vector.pc
-Iseastar/include -Ivector/include build/libseastar.a build/libvector.so
espindola commented 5 years ago
  1. With no extra options, it assumes normal linking of everything. Just normal gcc behavior of preferring shared libraries where they exist, but using a static library when that is the only one that exists (like for Seastar).

That is incorrect. pkg-config (unlike ld, gcc is not involved) has to always follow one dependency path or another. It can read Requires: or Requires.private. Once it does, it has decided on static versus shared linking.

Because we build libseastar.a ourselves, we know that it is only a static library

That will hopefully not always be true. In particular building it shared will be a nice improvement for Dev build mode.

Also, even if we never wanted to build it shared, we should not be different from every other library just to work around this pkg-config limitation. Users should not have to know that to use static libraries they should use --static, except for seastar, which is special.

There is no such thing as "normal linking". Each library is dynamic or static. As I explained on my first message, it would be nice for pkg-config to be able to check which one is available, but unfortunately that feature is missing and it always assumes static or dynamic.

I still don't understand why we think that the question whether Seastar itself is static or shared, has any implication on whether application's should need to specify the transitive dependencies of Seastar explicitly (e.g., -lidn2 because it's required by libgnutls required by Seastar).

I don't think that. That is why I am saying there is a limitation (bug?) in pkg-config.

hakuch commented 5 years ago

@espindola, is it possible that you could describe the limitation of pkg-config that you have in mind in the context of the examples I just shared?

nyh commented 5 years ago

@hakuch, your example is missing one level of libraries, in the original example we had seastar requiring gnutls requiring idn2 (for example), and only the upper level (seastar) was a static library. Because you only have two levels (seastar and vector) you don't see this problem.

To "fix" your example, consider that "vector" is itself a shared library, which needs the libstuff shared library. Now, look at the static "seastar" example. You'll have:

$ pkg-config --libs --cflags --static ./seastar.pc
-Iseastar/include -Ivector/include build/libseastar.a -lvector -lstuff

See, here we got "-lstuff" added because it's a dependency of vector and we used "--static". But it's NOT needed, because libvector.so already refers to it, because libvector.so is a shared library, even though seastar.a isn't.

One may suggest that it doesn't matter - although "-lstuff" isn't needed, it doesn't hurt either. But doesn't it? The problem is, that it does hurt (a little) - on Fedora (and I assume other distros as well?) different RPMs bring libstuff.so.7 (the actual library that other system libraries refer to) and libstuff.so (only needed to compile things against libstuff.so).

I still don't understand why the solution I suggested isn't a good one. My solution is that when a system installs only a static version of a library, it should install a different version of the ".pc" file which includes the LIbs.private in its Libs.

By the way, I'm starting to wonder if --maximum-traverse-depth isn't a good solution and not just a hack. Basically it will allow someone to use Seastar's direct dependencies (because Seastar is static and miss these dependencies) but not its deeper ones. This almost works:

 c++ getting-started.cc `pkg-config --cflags --libs --static --maximum-traverse-depth=2 ./build/release/seastar.pc`
/usr/bin/ld: /home/nyh/seastar/build/release/libseastar.a(reactor.cc.o): undefined reference to symbol 'pthread_setname_np@@GLIBC_2.12'
//usr/lib64/libpthread.so.0: error adding symbols: DSO missing from command line
collect2: error: ld returned 1 exit status

I think we're missing an explicit mention of "-pthread" (or -lpthread) in the command line, and it's unrelated to the other problems.

nyh commented 5 years ago

Hmm, it's not just pthread. maximum-traverse-depth=2 also misses other things. Like yaml-cpp. I don't know why.

hakuch commented 5 years ago

@hakuch, your example is missing one level of libraries, in the original example we had seastar requiring gnutls requiring idn2 (for example), and only the upper level (seastar) was a static library. Because you only have two levels (seastar and vector) you don't see this problem.

To "fix" your example, consider that "vector" is itself a shared library, which needs the libstuff shared library. Now, look at the static "seastar" example. You'll have:

In my examples, vector is a shared library.

nyh commented 5 years ago

@hakuch I know that in your example vector is a shared library, but another level is missing (yet another library that vector will need, like libstuff in my comment).

hakuch commented 5 years ago

@nyh: I'll add another layer and post a new example.

nyh commented 5 years ago

(and please note that in my example with the third layer, the problem is just the "small" problem of the extra unneeded "-lstuff". It's not an "obvious" problem like things not working at all).

espindola commented 5 years ago

@espindola, is it possible that you could describe the limitation of pkg-config that you have in mind in the context of the examples I just shared?

It is better to expand the example a bit. Lets say that vector can be shared or static and depends internally on gmp.

When running "pkg-config --libs seastar.pc", we would only get -lseastar. When running "pkg-config --libs --static seastar.pc" we would get -lseastar -lvector -lgmp.

Note that --static (or its absence) is all or nothing. It would be nice for seastar to check what variants are actually present.

For example, if we only have a shared vector "pkg-config --static seastar.pc" should print -lseastar -lvector.

If we only have a static vector and seastar "pkg-config seastar.pc" should print -lseastar -lvector -lgmp.

It is a small gain and a complication on pkg-config, so no surprise it is not implemented. It could also have a --shared-deps option, but that would be, in practice, equivalent to --maximum-traverse-depth=2.

espindola commented 5 years ago

I still don't understand why the solution I suggested isn't a good one. My solution is that when a system installs only a static version of a library, it should install a different version of the ".pc" file which includes the LIbs.private in its Libs.

Because we can provide both shared and static. Which one is available depends only on which packages are installed.

Consider a time when seastar is part of fedora. What we would have is

seastar -> /usr/lib64/libseastar.so.42 seastar-devel -> /usr/lib64/libseastar.so seastar-static -> /usr/lib64/libseastar.a

nyh commented 5 years ago

On Mon, Jan 14, 2019 at 7:23 PM Rafael Ávila de Espíndola < notifications@github.com> wrote:

I still don't understand why the solution I suggested isn't a good one. My solution is that when a system installs only a static version of a library, it should install a different version of the ".pc" file which includes the LIbs.private in its Libs.

Because we can provide both shared and static. Which one is available depends only on which packages are installed.

Consider a time when seastar is part of fedora. What we would have is

seastar -> /usr/lib64/libseastar.so.42 seastar-devel -> /usr/lib64/libseastar.so seastar-static -> /usr/lib64/libseastar.a

You're right. But then the assumption can be (and usually is!) that a normal user will only ever use libseastar.a if everything is static (i.e., gcc -static). In that case, "pkg-config --static" is the correct thing to do. In all other cases, you'll always use libseastar.so. That's how everyone does things when there are both .so and .a. I think it would be a very rare thing to do dynamic linking, and have libseastar.so, but still insist to build with libseastar.a with everything else remaining shared libraries.

But Seastar is different. It's only a static library, and everybody is forced to use it, even in dynamic compilation. This is why I think the dynamic version of pkg-config needs to provide its direct dependencies as well.

espindola commented 5 years ago

On Mon, Jan 14, 2019 at 7:23 PM Rafael Ávila de Espíndola < @.**> wrote: I still don't understand why the solution I suggested isn't a good one. My solution is that when a system installs only a static version of a library, it should install a different version of the ".pc" file which includes the LIbs.private in its Libs. Because we can provide both shared and static. Which one is available depends only on which packages are installed. Consider a time when seastar is part of fedora. What we would have is seastar -> /usr/lib64/libseastar.so.42 seastar-devel -> /usr/lib64/libseastar.so seastar-static -> /usr/lib64/libseastar.a You're right. But then the assumption can be (and usually is!) that a normal user will only ever use libseastar.a if everything* is static (i.e., gcc -static). In that case, "pkg-config --static" is the correct thing to do. In all other cases, you'll always use libseastar.so. That's how everyone does things when there are both .so and .a. I think it would be a very rare thing to do dynamic linking, and have libseastar.so, but still insist to build with libseastar.a with everything else remaining shared libraries. But Seastar is different. It's only a static library, and everybody is forced to use it, even in dynamic compilation. This is why I think the dynamic version of pkg-config needs to provide its direct dependencies as well.

The "usual assumption" is not how we normally use seastar today!

The entire point of having different private and public variables is so that pkg-config can pick the correct one. Yes, it would be nice if pkg-config had command line options to saying that we want a static seastar, a dynamic liz4 and a static boost. It would also be nice for it to check if both static and shared are actually available.

Unfortunately neither of these features exists. Hopefully they will in the future. Until then all we can do is keep informing pkg-config of the real dependencies and live with the limitation.

If you find it too much to install a package with a symbolic link, just don't use pkg-config or just wait a bit and use the shared version of seastar.

Forcing everyone to have a seastar.pc that lies about what dependencies it has is not a solution IMHO. Specially to avoid just a few symbolic links.

hakuch commented 5 years ago

Thanks to @nyh and @espindola for elaborating. I amended the example (but didn't yet update the remote repository).

We now have an abstraction library (hardware abstraction library) that vector depends on privately.

Interestingly, CMake does the right thing and does not link against libabstraction.so:

/usr/lib64/ccache/c++     CMakeFiles/foo.dir/foo/main.cc.o  -o foo -Wl, rpath,/home/jhaberku/src/transitivity/build libseastar.a libvector.so 

Let's look at pkg-config stuff:

includedir=/home/jhaberku/src/transitivity/abstraction/include
libdir=/home/jhaberku/src/transitivity/build

Name: abstraction
Description: Example shared library that is used privately by another private shared library
Version: 0.1.0
Cflags: -I${includedir}
Libs: ${libdir}/libabstraction.so
includedir=/home/jhaberku/src/transitivity/vector/include
libdir=/home/jhaberku/src/transitivity/build

Name: vector
Description: Example shared library that is used privately.
Version: 0.1.0
Requires.private: abstraction >= 0.1.0
Cflags: -I${includedir}
Libs: ${libdir}/libvector.so

With --static and a static seastar:

$ pkg-config --libs --cflags --static ./seastar.pc
-Iseastar/include -Ivector/include -Iabstraction/include build/libseastar.a build/libvector.so build/libabstraction.so 

We're now linking against libabstraction.so even though we don't need to. We can omit it, like below:

$ g++ ../foo/main.cc -I/home/jhaberku/src/transitivity/seastar/include -Ivector/include -Iabstraction/include build/libseastar.a build/libvector.so -o foo
hakuch commented 5 years ago

@nyh, you said:

The problem is, that it does hurt (a little) - on Fedora (and I assume other distros as well?) different RPMs bring libstuff.so.7 (the actual library that other system libraries refer to) and libstuff.so (only needed to compile things against libstuff.so).

I see now that when seastar is static, we unnecessarily link against libabstraction.so (note that this is an issue with pkg-config and not CMake).

However, I don't quite follow your explanation of why this extra link is a problem. Can you explain differently?

espindola commented 5 years ago

Thanks to @nyh and @espindola for elaborating. I amended the example (but didn't yet update the remote repository).

We now have an abstraction library (hardware abstraction library) that vector depends on privately.

Interestingly, CMake does the right thing and does not link against libabstraction.so:

So does meson. Which is why it parses .pc files itself and checks if both .so and .a variants are present.

This is really a missing feature is pkg-config. That is not that unreasonable, since pkg-config is not a replacement for a full build system like cmake or meson.

nyh commented 5 years ago

The "usual assumption" is not how we normally use seastar today!

No - what I called the "usual assumption" (TM) is that when somebody has both shared library and shared library installed in his system, the only case he wants to use the static library is when he wants to compile everything static, i.e., -static.

Think back - when was the last time you had both static libz and dynamic libz, and decided you want to compile with that static libz? How do you even say this in gcc? When you do "gcc -lz" you will get the dynamic libz by default (if both exist). If you do "gcc pkg-config --libs --static zlib", this will also come out "-lz", and also give you the dynamic, not the static library!

Basically "pkg-config --static" is only good for one thing: compiling everything statically (gcc -static). We can't wish for it to work for other things (like when one specific library is static-only), even if everyone else wishes the same :-)

The entire point of having different private and public variables is so

that pkg-config can pick the correct one. Yes, it would be nice if pkg-config had command line options to saying that we want a static seastar, a dynamic liz4 and a static boost. It would also be nice for it to check if both static and shared are actually available.

Unfortunately neither of these features exists. Hopefully they will in the future. Until then all we can do is keep informing pkg-config of the real dependencies and live with the limitation.

I'll prepare patch tomorrow, which lives with the limitations, in a way that actually produces correct results - even if it doesn't fit with the perfect model of what "--static" may or should do. The current way we do it, simply doesn't do the right thing.

If you find it too much to install a package with a symbolic link, just don't use pkg-config or just wait a bit and use the shared version of seastar.

It's not "too much", but it's my last resort. We would need to also add these packages to install-dependencies.sh.

The nice thing about having a two year old build machine on which I compiled a hundred different things is that it puts into perspective what normal users will, or will not, have installed. Clearly, no normal user will have ever installed the libunistring-devel or libidn2-devel packages - I didn't. Until today, I never even heard of these libraries (Seastar never uses them directly), and I never wanted to compile any source code which relied of them. I still don't.

Forcing everyone to have a seastar.pc that lies about what dependencies it has is not a solution IMHO. Specially to avoid just a few symbolic links.

I agree that the "avoid symbolic links" sounds silly. I think it's just a symptom that we're doing something bigger wrong.

espindola commented 5 years ago

The "usual assumption" is not how we normally use seastar today! No - what I called the "usual assumption" (TM) is that when somebody has both shared library and shared library installed in his system, the only case he wants to use the static library is when he wants to compile everything static, i.e.

Also false. We will probably want to keep using a static seastar. Liking with a static libstdc++ is so common that gcc has a command line option to make it easier. I have also seen -Bstatic -lfoo -Bdynamic in other projects before.

, -static. Think back - when was the last time you had both static libz and dynamic libz, and decided you want to compile with that static libz?

When I was working on lld and wanted to link with llvm.

How do you even say this in gcc?

-Bstatic ... -Bdynamic

I'll prepare patch tomorrow, which lives with the limitations, in a way that actually produces correct results - even if it doesn't fit with the perfect model of what "--static" may or should do. The current way we do it, simply doesn't do the right thing.

It does! What will your patch do? Assume that we don't have a shared library like the old build system? That is simply not OK.

If you don't want to put up with pkg-config limitation just use cmake on your project. As Jesse example shows cmake has no problem stopping dependency propagation when it gets to a shared library.

Unlike ld cmake or meson, there is no way in pkg-config to say that we want to link with a static seastar and a dynamic liblz4. We just have to live with it.

nyh commented 5 years ago

On Mon, Jan 14, 2019 at 8:13 PM Rafael Ávila de Espíndola < notifications@github.com> wrote:

It does! What will your patch do? Assume that we don't have a shared library like the old build system? That is simply not OK.

Let's say we do what I suggested, which is to add to Seastar's Libs (not Libs.private) its direct dependencies, like -lgnutls and -lyaml. Then, we also add a dynamic library Seastar, but never change seastar.pc and it still has all those libraries in "Libs". What's the harm, then? You'll get a dynamic link with Seastar with unnecessary "-lgnutls", "-lyaml" on the link line. Exactly what you are trying to convince me I should tolerate :-) (when I get unnecessary -lidn2, -lunistring, etc. on the link line).

If you don't want to put up with pkg-config limitation just use cmake on your project.

No, I want a one-line command that Just Works (TM) for users who just install Seastar and want to run a hello-world using the instructions we give them.

Currently, it simply doesn't work. If our best solution to this problem is to add libidn2-devel and friends to install-dependencies.sh, then that's what we should do (despite these packages being completely unnecessary and install irrelevant crap like header files). But we have to do something. The current example simply doesn't work :-(

As Jesse example shows cmake has no problem stopping dependency propagation when it gets to a shared library.

Unlike ld cmake or meson, there is no way in pkg-config to say that we want to link with a static seastar and a dynamic liblz4. We just have to live with it.

As long as what we tell the users to do actually works :-)

espindola commented 5 years ago

Let's say we do what I suggested, which is to add to Seastar's Libs (not Libs.private) its direct dependencies, like -lgnutls and -lyaml. Then, we also add a dynamic library Seastar, but never change seastar.pc and it still has all those libraries in "Libs".

That is simply wrong. Those are not dependencies someone linking with seastar has to link with if seastar is dynamic.

You seem to think that you not having to install a package justify people using a dynamic seastar having to have extra dependencies.

Again, there is a limitation is pkg-config. The best we can do is tell it the truth and hope the limitation gets fixed one day. We lose hope if we lie to pkg-config about what our dependencies are.

Unlike ld cmake or meson, there is no way in pkg-config to say that we want to link with a static seastar and a dynamic liblz4. We just have to live with it. As long as what we tell the users to do actually works :-)

It does! Just install the missing package or don't use pkg-config.

espindola commented 5 years ago

Hmm, it's not just pthread. maximum-traverse-depth=2 also misses other things. Like yaml-cpp. I don't know why.

Sorry, just noticed this part. This looks like a plain bug. --maximum-traverse-depth=2 is missing -lgnutls but --maximum-traverse-depth=3 include lgnutls and its dependencies.

espindola commented 5 years ago

Looking at the source code I see that --maximum-traverse-depth is just about following Requires. It would work if everything used .pc files:

Level1 is the empty root, so always empty Level2 is "just" seastar, it includes seastar and the Libs and Libs.private fields Level3 is seastar and its direct Requires. In particular it gnutls and its Libs.private.

If gnutls had

Requires.private: tspi gmp ustring idn2 nettle, hogweed, libtasn1, p11-kit-1

instead of

Libs.private: -ltspi -lgmp /usr/lib64/libunistring.so -lidn2 Requires.private: nettle, hogweed, libtasn1, p11-kit-1

Then it would work.

espindola commented 5 years ago

I reported https://git.dereferenced.org/pkgconf/pkgconf/issues/21

denesb commented 5 years ago

@hakuch and @espindola I admire your commitment to make things correct, from the pure theoretical point of view, but I think what our build system needs to do, first and foremost, is to work and only secondarily be correct if possible. Yes, it is annoying to "lie" about what our dependencies are, or to apply dirty hacks to keeps things working, but this is the reality. Tools have limitations but I think that requiring the users of seastar to work around these limitations, just to keep our pkg-config file correct, is unacceptable. You say, why is it a big deal that one has to install a few more libraries. It is a very big deal. Every bit of inconvenience a user has to suffer is an opportunity for them to get pissed enough to just leave the project and go find some other project that just works.

To give you an example, I wanted to try out @hakuch's new patch of integrating the CMakeified seastar into Scylla and what I was greeted with, when attempting a build, is that I have to install 1 new library. I installed it, then ran the build just to find out that I have to install 3 new libraries. Where will this end? I'm already very tempted to just leave testing this patch and just do something else, because I do happen to know that seastar did not acquire any new dependencies in the meanwhile and this is all just a bug in seastar's build system. Consider this: adding these false dependencies to install-dependencies.sh is even more wrong then "lieing" about our dependencies in the requires list.

Also please consider that pkg-config is the primary point of contact for new users just getting a taste of seastar, because most people (including me) don't throw together a CMakelists.txt for a simple experimental project that is composed of a single file, so the argument of "pkg-config is broken, go use CMake if you don't want to install these extra, useless packages" is just not correct. pkg-config has to just work, regardless of how incorrect it's content is, because users just don't care about that stuff. We can fix the correctness issue, when pkg-config becomes more flexible, but we can't wait for that with making our build system work.

hakuch commented 5 years ago

@denesb, I'm not sure where to start in replying to your post because from my point of view it's largely misguided, including in how you define terms like "works", "correct", and the actual problems that would prevent newcomers from joining the project (which is, incidentally, exactly why our old use of pkg-config was so harmful: it was very unconventional). I don't mean this to be insulting, but rather to explain why it may take some time for me to address it effectively.

However,

To give you an example, I wanted to try out @hakuch's new patch of integrating the CMakeified seastar into Scylla and what I was greeted with, when attempting a build, is that I have to install 1 new library. I installed it, then ran the build just to find out that I have to install 3 new libraries. Where will this end? I'm already very tempted to just leave testing this patch and just do something else, because I do happen to know that seastar did not acquire any new dependencies in the meanwhile and this is all just a bug in seastar's build system. Consider this: adding these false dependencies to install-dependencies.sh is even more wrong then "lieing" about our dependencies in the requires list.

I have no clear idea of what specifically you're talking about here. If it's directly about this issue, then please elaborate. If it's not, then please open a dedicated issue with details so that they can be addressed.

espindola commented 5 years ago

@hakuch and @espindola I admire your commitment to make things correct, from the pure theoretical point of view, but I think what our build system needs to do, first and foremost, is to work

cmake and meson are not theoretical.

There is a limitation in pkg-config implementation. It cannot represent linking one library statically and its dependencies dynamically.

That is not a limitation in the file format. In fact, there is no such limitation in cmake or meson, and everything should just work for consumers using either.

So the options we have are

I am sorry I missed the fact that install-dependencies.sh had to be updated. I will fix that, but having correct dependencies is not a theoretical concern.

denesb commented 5 years ago

On Tue, 2019-01-15 at 07:19 -0800, Jesse Haber-Kucharsky wrote:

@denesb, I'm not sure where to start in replying to your post because from my point of view it's largely misguided, including in how you define terms like "works", "correct", and the actual problems that would prevent newcomers from joining the project (which is, incidentally, exactly why our old use of pkg-config was so harmful: it was very unconventional). I don't mean this to be insulting, but rather to explain why it may take some time for me to address it effectively.

However,

To give you an example, I wanted to try out @hakuch's new patch of integrating the CMakeified seastar into Scylla and what I was greeted with, when attempting a build, is that I have to install 1 new library. I installed it, then ran the build just to find out that I have to install 3 new libraries. Where will this end? I'm already very tempted to just leave testing this patch and just do something else, because I do happen to know that seastar did not acquire any new dependencies in the meanwhile and this is all just a bug in seastar's build system. Consider this: adding these false dependencies to install-dependencies.sh is even more wrong then "lieing" about our dependencies in the requires list.

I have no clear idea of what specifically you're talking about here. If it's directly about this issue, then please elaborate. If it's not, then please open a dedicated issue with details so that they can be addressed.

It is related to this issue, in that our pkg-config doesn't work with --static (that is how your patch to Scylla uses it). What I'm trying to argue for is to have a pkg-config that just works, with or without --static. Currently we have two choices:

We should make one of these work somehow, without burdening the user with a workaround.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

{"api_version":"1.0","publisher":{"api_key":"05dde50f1d1a384dd78767c5 5493e4bb","name":"GitHub"},"entity":{"external_key":"github/scylladb/ seastar","title":"scylladb/seastar","subtitle":"GitHub repository","main_image_url":" https://github.githubassets.com/images/email/message_cards/header.png","avatar_image_url":"https://github.githubassets.com/images/email/message_cards/avatar.png","action":{"name":"Open in GitHub","url":" https://github.com/scylladb/seastar"}},"updates":{"snippets":[{"icon":"PERSON","message":"@hakuch in #579: @denesb, I'm not sure where to start in replying to your post because from my point of view it's largely misguided, including in how you define terms like \"works\", \"correct\", and the actual problems that would prevent newcomers from joining the project (which is, incidentally, exactly why our old use of pkg-config was so harmful: it was very unconventional). I don't mean this to be insulting, but rather to explain why it may take some time for me to address it effectively.\r\n\r\nHowever,\r\n\r\n\u003e To give you an example, I wanted to try out @hakuch's new patch of integrating the CMakeified seastar into Scylla and what I was greeted with, when attempting a build, is that I have to install 1 new library. I installed it, then ran the build just to find out that I have to install 3 new libraries. Where will this end? I'm already very tempted to just leave testing this patch and just do something else, because I do happen to know that seastar did not acquire any new dependencies in the meanwhile and this is all just a bug in seastar's build system. Consider this: adding these false dependencies to install-dependencies.sh is even more wrong then \"lieing\" about our dependencies in the requires list.\r\n\r\nI have no clear idea of what specifically you're talking about here. If it's directly about this issue, then please elaborate. If it's not, then please open a dedicated issue with details so that they can be addressed.\r\n"}],"action":{"name":"View Issue","url":" https://github.com/scylladb/seastar/issues/579#issuecomment-454429291 "}}} [ { "@context": "http://schema.org", "@type": "EmailMessage", "potentialAction": { "@type": "ViewAction", "target": " https://github.com/scylladb/seastar/issues/579#issuecomment-454429291 ", "url": " https://github.com/scylladb/seastar/issues/579#issuecomment-454429291 ", "name": "View Issue" }, "description": "View this Issue on GitHub", "publisher": { "@type": "Organization", "name": "GitHub", "url": "https://github.com" } } ]

denesb commented 5 years ago

On Tue, 2019-01-15 at 07:28 -0800, Rafael Ávila de Espíndola wrote:

@hakuch and @espindola I admire your commitment to make things correct, from the pure theoretical point of view, but I think what our build system needs to do, first and foremost, is to work

cmake and meson are not theoretical.

I'm not saying they are.

There is a limitation in pkg-config implementation. It cannot represent linking one library statically and its dependencies dynamically.

It seems the authors of pkg-config think otherwise, they closed the bug you opened as "wontfix" and they seem to think its the upstream (GNUTLS) pkg-config file that is at blame here.

That is not a limitation in the file format. In fact, there is no such limitation in cmake or meson, and everything should just work for consumers using either.

So the options we have are

Require a superset of packges for people using a static seastar with pkg-config. Write incorrect .pc files, which means that nothing can possibly produce correct results. Including a future pkg-config that has the missing feature. The one thing this would enable is that people using a static seastar could pass the wrong flag to pkg-config to get a smaller set of package dependencies.

I am sorry I missed the fact that install-dependencies.sh had to be updated. I will fix that, but having correct dependencies is not a theoretical concern.

If I understood correctly, pkg-config has two modes, --static, meaning force static linking of the library and its dependencies, and the "non-static", let's call this default. One could interpret the default mode as whatever the library prefers. Seastar prefers to be linked statically, with its dependencies being linked dynamically. Why not make the default mode do this exactly, leaving the door open for a completely static linking setup as well (with --static)?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

{"api_version":"1.0","publisher":{"api_key":"05dde50f1d1a384dd78767c5 5493e4bb","name":"GitHub"},"entity":{"external_key":"github/scylladb/ seastar","title":"scylladb/seastar","subtitle":"GitHub repository","main_image_url":" https://github.githubassets.com/images/email/message_cards/header.png","avatar_image_url":"https://github.githubassets.com/images/email/message_cards/avatar.png","action":{"name":"Open in GitHub","url":" https://github.com/scylladb/seastar"}},"updates":{"snippets":[{"icon":"PERSON","message":"@espindola in #579: \u003e @hakuch and @espindola I admire your commitment to make things correct, from the pure theoretical point of view, but I think what our build system needs to do, first and foremost, is to work \r\n\r\ncmake and meson are not theoretical.\r\n\r\nThere is a limitation in pkg-config implementation. It cannot represent linking one library statically and its dependencies dynamically.\r\n\r\nThat is not a limitation in the file format. In fact, there is no such limitation in cmake or meson, and everything should just work for consumers using either.\r\n\r\nSo the options we have are\r\n Require a superset of packges for people using a static seastar with pkg-config.\r\n Write incorrect .pc files, which means that nothing can possibly produce correct results. Including a future pkg-config that has the missing feature. The one thing this would enable is that people using a static seastar could pass the wrong flag to pkg-config to get a smaller set of package dependencies.\r\n\r\nI am sorry I missed the fact that install- dependencies.sh had to be updated. I will fix that, but having correct dependencies is not a theoretical concern.\r\n"}],"action":{"name":"View Issue","url":" https://github.com/scylladb/seastar/issues/579#issuecomment-454433007 "}}} [ { "@context": "http://schema.org", "@type": "EmailMessage", "potentialAction": { "@type": "ViewAction", "target": " https://github.com/scylladb/seastar/issues/579#issuecomment-454433007 ", "url": " https://github.com/scylladb/seastar/issues/579#issuecomment-454433007 ", "name": "View Issue" }, "description": "View this Issue on GitHub", "publisher": { "@type": "Organization", "name": "GitHub", "url": "https://github.com" } } ]

hakuch commented 5 years ago

It seems the authors of pkg-config think otherwise, they closed the bug you opened as "wontfix" and they seem to think its the upstream (GNUTLS) pkg-config file that is at blame here.

If I'm not mistaken, that repository is not for pkg-config, but for a different program with similar (identical?) goals called pkgconf. I'm not sure if @espindola was aware of this or not when he opened the issue.

In any case, we have already established that pkg-config includes private dependencies with ---static by design thanks to the examples above. We are not, if I understand correctly, discussing limitations in GnuTLS's pkg-config specifications but limitations in the design of pkg-config itself.

However, pkg-config is a well-established tool that has wide-spread use on most (all?) Linux platforms. If we want to integrate Seastar into that ecosystem, it is our responsibility to use the tools of that ecosystem correctly.

@denesb, you keep using the word "works" and asserting that the current system does "not work". Without defining exactly what this means, I'm concerned that we're talking past each other.

There are many work-flows that we may wish to support. Three of them are:

The "solution" you are proposing addresses only the first (and under a very restrictive set of assumptions about the user's environment, at that!), but makes the second two very difficult (I would like to say "impossible", but I don't think I can safely).

The pkg-config and CMake files in the Seastar repository currently allow for all three work-flows.

Because of the aforementioned limitations of pkg-config itself, the use of Seastar from pkg-config requires an additional package to be installed. This is not a consequence of choices Seastar itself has made, but those of the ecosystem (Fedora, GnuTLS, pkg-config) in which Seastar wants to live.

denesb commented 5 years ago

On Tue, Jan 15, 2019, 19:54 Jesse Haber-Kucharsky <notifications@github.com wrote:

It seems the authors of pkg-config think otherwise, they closed the bug you opened as "wontfix" and they seem to think its the upstream (GNUTLS) pkg-config file that is at blame here.

If I'm not mistaken, that repository is not for pkg-conffig, but for a different program with similar (identical?) goals called pkgconf. I'm not sure if @espindola https://github.com/espindola was aware of this or not when he opened the issue.

In any case, we have already established that pkg-config includes private dependencies with ---static by design with the examples above. We are not, if I understand correctly, discussing limitations in GnuTLS's pkg-config specifications but limitations in the design of pkg-config itself.

However, pkg-config is a well-established tool that has wide-spread use on most (all?) Linux platforms. If we want to integrate Seastar into that ecosystem, it is our responsibility to use the tools of that ecosystem correctly.

@denesb https://github.com/denesb, you keep using the word "works" and asserting that the current system does "not work". Without defining exactly what this means, I'm concerned that we're talking past each other.

I may have used the term "doesn't work" imprecisely. Indeed the current state can be made to work, but only if one installs all these packages that are not strictly needed to usr seastar in a project. I'm trying to see if there is a way to avoid that.

There are many work-flows that we may wish to support. Three of them are:

  • Build Seastar in a local directory and point pkg-config to it to compile a Seastar program
  • Install Seastar to the file-system and use pkg-config to consume it
  • Install Seastar to the file-system and use CMake to consume it

The "solution" you are proposing addresses only the first, but makes the second two very difficult (I would like to say "impossible", but I don't think I can safely).

I don't see why my solution would make those too difficult, let alone impossible. I was proposing that we keep listing our transitive dependencies in the default (--static omitted) invocation of pkg-config, just like we did before the migration to CMake. I don't see why this would break option (2), let alone (3) as it only affects the generated pkg-config file. Since we do not support dynamic linking of seastar I do not see the problem. When we start supporting dynamic linking of seastar we can fix our pkg-config and state that we either support full dynamic or fullbstatic mode, through pkg-config that is. Until that day, I don't see the problem with supporting static kinking with dynamic dependencies and static all the way.

The pkg-config and CMake files in the Seastar repository currently allow

for all three work-flows.

Because of the aforementioned limitations of pkg-config itself, the use of Seastar from pkg-config requires an additional package to be installed.

It's not just one package, I stopped counting at 4.

This is not a consequence of Seastar itself, but the ecosystem (Fedora,

GnuTLS, pkg-config) in which it wants to live in.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/scylladb/seastar/issues/579#issuecomment-454487748, or mute the thread https://github.com/notifications/unsubscribe-auth/ABUy2aFsy2FPkHG0bwwzhFFpCEyafr3xks5vDhXwgaJpZM4Z-Bwg .

nyh commented 5 years ago

On Tue, Jan 15, 2019 at 5:28 PM Rafael Ávila de Espíndola < notifications@github.com> wrote:

@hakuch https://github.com/hakuch and @espindola https://github.com/espindola I admire your commitment to make things correct, from the pure theoretical point of view, but I think what our build system needs to do, first and foremost, is to work

cmake and meson are not theoretical.

We cannot solve every problem with another layer of indirection. We need a way for the user to know the command line to compile a Seastar application, without learning yet another tool to compile his application. Clearly, cmake is really popular these days, and you guys love it. But not everyone does. People should be allowed to compile Seastar applications without it. If we believe that pkg-config is a piece of crap that can't do what Seastar needs to print the command line, we can have a separate script to print the command line, so a user can do

c++ somefile.cc $SCYLLA/build/release/scyllas-own-pkconfig-script --libs --cflags

But before we do that, we need to understand why, even though pkg-config may not be perfect, Seastar is the only project which claims that pkg-config is not good enough to use. Yes, I know Seastar is the best project (we know code! we have all the best code!), but there's also room for the possibility that maybe we're doing something wrong here.

There is a limitation in pkg-config implementation. It cannot represent

linking one library statically and its dependencies dynamically.

Yes, pkg-config has a limitation, but I believe that you're over-dramatising its limitation.

The limitation is that pkg-config has just two modes:

  1. The "default" mode. This is for "default" linking. In most cases, both shared and static libraries exist, or just a shared library, so the "default" is that the shared library will be used.
  2. The "static" mode. This is for "-static" linking of everything, so all the transitive dependencies, all the way down, are needed.

Indeed, "pkg-config xyz" does not have any way to say that we want to use the static library of just xyz (by wrapping the output of pkgconfig by -static and [what's the opposite of -static?]) but everything else is shared. But Seastar does not need this feature! Seastar is static only. So it's default mode can just do what is needed for a static-only library, namely to include the direct (only!) dependencies in the pkg-config --libs output.

If, in the future, we support creating both shared and static versions of Seastar, we will drop the direct dependencies from Libs (as we have today). You're right that then, if a user decides to install only the static version, he will have a problem - basically, exactly the same problem we have today. This is where the pkg-config model will not be powerful enough. But if Seastar is used in a more traditional fashion - i.e., if a shared version exists, most users will install and use it primarily, not the static library - it will continue to work well.

So the options we have are

  • Require a superset of packges for people using a static seastar with pkg-config.
  • Write incorrect .pc files, which means that nothing can possibly produce correct results.

As I said above, everything is correct in my proposal if Seastar is only a static library. There's nothing "incorrect" in that case. As soon as we have an option for Seastar to also be a shared library, we'll drop the dependencies from Libs to Libs.private (as we have them now), and everything will go back to perfect correctness as you like.

  • Including a future pkg-config that has the missing feature.

If there's strong opposition to fix (as I consider it "fixing", you consider "break" :-) ) seastar.pc, I'll think about this option, considering both our own script and fixing pkg-config. I think this may be the only option that everyone can agree on....

  • The one thing this would enable is that people using a static seastar could pass the wrong flag to pkg-config to get a smaller set of package dependencies.

I am sorry I missed the fact that install-dependencies.sh had to be updated. I will fix that, but having correct dependencies is not a theoretical concern.

I don't think that this is a "correct" solution:

Please go back to what you considered an "incorrect" seastar.pc: You told me that an incorrect seastar.pc is one where "pkg-config seastar --libs" outputs too many library names (i.e., also dependencies for Seastar). E.g., if Seastar is a shared library, it's enough to do "-lseastar" and one doesn't need "-lseastar -lgnutls". The extra "-lgnutls" is incorrect, but harmless (in the sense that compilation will still work).

BUT, the current situation is "wrong" in exactly the same way: pkg-config --static --libs seastar outputs some unnecessary library names, e.g., "-lidn2"... Yes, it's harmless (but annoying), but if you consider it wrong, then it's just as wrong, isn't it?

I personally prefer that we don't add more garbage to install-dependencies.sh. As Botond said, Seastar did not recently grow new dependencies... All of the new dependencies are unnecessary.

nyh commented 5 years ago

On Tue, Jan 15, 2019 at 8:46 PM Nadav Har'El nyh@scylladb.com wrote:

Yes, pkg-config has a limitation, but I believe that you're over-dramatising its limitation.

The limitation is that pkg-config has just two modes:

  1. The "default" mode. This is for "default" linking. In most cases, both shared and static libraries exist, or just a shared library, so the "default" is that the shared library will be used.
  2. The "static" mode. This is for "-static" linking of everything, so all the transitive dependencies, all the way down, are needed.

Indeed, "pkg-config xyz" does not have any way to say that we want to use the static library of just xyz (by wrapping the output of pkgconfig by -static and [what's the opposite of -static?]) but everything else is shared.

I've given this more thought, and I realized that pkg-config doesn't even have this problem. Maybe it doesn't have any problem:

Consider that you have two versions of libseastar installed - one static and one shared. As I think we already agreed, we have two options what to use on the compilation command line:

  1. c++ ... pkg-config --libs seastar -lsomethingelse
  2. c++ ... -Bstatic pkg-config --libs --static seastar -Bdynamic -lsomethingelse

Note how the "Bstatic" option wraps the entire output of pkg-config! It is physically impossible for you to choose to compile with static seastar but dynamic gnutls (for example). This is because the entire pkg-config output is wrapped in -Bstatic, so both seastar and all its dependencies will be picked up as static libraries. So the situation where you said pkg-config is broken, can't even exist.

Yes, someone may want to manually compile with the static version of Seastar (assuming both it and dynamic version exist) but still with a dynamic version of gnutls. He can type in manually "-l:seastar.a -lgnutls" for example. BUT, this is not what pkg-config outputs. So pkg-config doesn't even claim to support such a feature. And it doesn't need to support it. pkg-config doesn't claim to do everything - just to do what it does well. And to do what it does well, we need "pkg-config --libs seastar", without "--static", to do the right thing (TM).

hakuch commented 5 years ago

I did some further investigation and I'm fairly sure the issue is with the version of gnutls.pc installed by Fedora (or possibly it's an upstream GnuTLS issue).

We have already established that pkg-config is limited such that it can be invoked at the "root" of the dependency tree in either "shared" or "static" mode, but not with a mix of both.

This means that the private dependencies of a package like GnuTLS are included unnecessarily at link time, but this shouldn't actually be a problem in practise if the distribution is set up correctly.

Let's look at the gnutls-devel package on Fedora.

$ sudo dnf install gnutls-devel                                                                                                                                                              
Fedora 29 - x86_64 - Updates                                                                                                                                                          11 kB/s |  14 kB     00:01     
Fedora 29 - x86_64 - Updates                                                                                                                                                         3.2 MB/s |  20 MB     00:06     
Last metadata expiration check: 0:00:04 ago on Wed 16 Jan 2019 11:09:38 AM EST.                                                                                                                                      
Dependencies resolved.                                                                                                                                                                                               
=====================================================================================================================================================================================================================
 Package                                               Arch                                           Version                                                  Repository                                       Size 
=====================================================================================================================================================================================================================
Installing:                                                                                                                                                                                                          
 gnutls-devel                                          x86_64                                         3.6.5-2.fc29                                             updates                                         2.1 M
Upgrading:
 gnutls                                                x86_64                                         3.6.5-2.fc29                                             updates                                         861 k
 gnutls-c++                                            x86_64                                         3.6.5-2.fc29                                             updates                                          25 k
 gnutls-dane                                           x86_64                                         3.6.5-2.fc29                                             updates                                          25 k
 gnutls-utils                                          x86_64                                         3.6.5-2.fc29                                             updates                                         293 k
Installing dependencies:
 nettle-devel                                          x86_64                                         3.4.1rc1-1.fc29                                          updates                                         631 k
 gmp-devel                                             x86_64                                         1:6.1.2-8.fc29                                           fedora                                          174 k
 p11-kit-devel 

Let us examine the files installed by the gnutls-devel package on Fedora.

rpm -ql gnutls-devel | grep lib
/usr/lib64/.libgnutls.so.30.23.0.hmac
/usr/lib64/.libgnutls.so.30.hmac
/usr/lib64/libgnutls-dane.so
/usr/lib64/libgnutls.so
/usr/lib64/libgnutlsxx.so
/usr/lib64/pkgconfig/gnutls-dane.pc
/usr/lib64/pkgconfig/gnutls.pc

And gnutls.pc:

prefix=/usr
exec_prefix=/usr
libdir=/usr/lib64
includedir=/usr/include

Name: GnuTLS
Description: Transport Security Layer implementation for the GNU system
URL: http://www.gnutls.org/
Version: 3.6.5
Libs: -L${libdir} -lgnutls
Libs.private:      -ltspi -lgmp /usr/lib64/libunistring.so -lidn2
Requires.private: nettle, hogweed, libtasn1, p11-kit-1
Cflags: -I${includedir}

Note that private fields reference libdn2.so and libunistring.so.

Let us examine the shared libraries encoded in libgnutls.so itself:

$ ldd /usr/lib64/libgnutls.so
        linux-vdso.so.1 (0x00007ffe5c0df000)
        libp11-kit.so.0 => /lib64/libp11-kit.so.0 (0x00007f5e9921b000)
        libidn2.so.0 => /lib64/libidn2.so.0 (0x00007f5e991fc000)
        libunistring.so.2 => /lib64/libunistring.so.2 (0x00007f5e99078000)
        libdl.so.2 => /lib64/libdl.so.2 (0x00007f5e99072000)
        libtasn1.so.6 => /lib64/libtasn1.so.6 (0x00007f5e9905d000)
        libnettle.so.6 => /lib64/libnettle.so.6 (0x00007f5e99022000)
        libhogweed.so.4 => /lib64/libhogweed.so.4 (0x00007f5e98fee000)
        libgmp.so.10 => /lib64/libgmp.so.10 (0x00007f5e98f70000)
        libc.so.6 => /lib64/libc.so.6 (0x00007f5e98daa000)
        /lib64/ld-linux-x86-64.so.2 (0x00007f5e99532000)
        libffi.so.6 => /lib64/libffi.so.6 (0x00007f5e98d9f000)
        libpthread.so.0 => /lib64/libpthread.so.0 (0x00007f5e98d7d000)

This means that when gnutls-devel was packaged, it was linked against libunistring.so.2. Notice the suffix of .2!

However, the pkg-config file references libunistring.so without the suffix!

Let's look at the shared libraries that are installed on my system.

$ sudo ls /lib64 | grep unistring
libunistring.so.2
libunistring.so.2.1.0

What are the dependencies of gnutls-devel?

$ rpm -qR gnutls-devel
/bin/sh
/bin/sh
/sbin/install-info
/sbin/install-info
/usr/bin/pkg-config
gnutls(x86-64) = 3.6.5-2.fc29
gnutls-c++(x86-64) = 3.6.5-2.fc29
gnutls-dane(x86-64) = 3.6.5-2.fc29
libgnutls-dane.so.0()(64bit)
libgnutls.so.30()(64bit)
libgnutlsxx.so.28()(64bit)
pkgconfig
pkgconfig(gnutls)
pkgconfig(hogweed)
pkgconfig(libtasn1)
pkgconfig(nettle)
pkgconfig(p11-kit-1)
rpmlib(CompressedFileNames) <= 3.0.4-1
rpmlib(FileDigests) <= 4.6.0-1
rpmlib(PayloadFilesHavePrefix) <= 4.0-1
rpmlib(PayloadIsXz) <= 5.2-1

Conclusion

The pkg-config file for gnutls-devel specifies requirements in its private fields which are not actually on the system when the package is installed.

When additional packages are installed for the private dependencies of GnuTLS (e.g., libunistring-devel) then the files referenced in gnutls.pc are available, and invoking pkg-config at the root with --static will work.

Therefore, this is an issue either in Fedora packaging, or in GnuTLS itself and the gnutls.pc file it copies or generates.

/cc @espindola, @eliransin

hakuch commented 5 years ago

Some possible solutions based on the above conclusion:

nyh commented 5 years ago

On Wed, Jan 16, 2019 at 6:23 PM Jesse Haber-Kucharsky < notifications@github.com> wrote:

This means that when gnutls-devel was packaged, it was linked against libunistring.so.2. Notice the suffix of .2!

However, the pkg-config file references libunistring.so without the suffix!

First of all, it's indeed a bug that the pkg-config refers to the pathname "libunistring.so", it should refer to "-lunistring" (I opened https://gitlab.com/gnutls/gnutls/issues/675). But that doesn't change anything regarding what you're saying: In any case, gnutls links to a particular version of the .so, e.g., libunistring.so.2, but when using "-lunistring" the compiler looks for a libunistring.so, without the suffix.

In the really old days (maybe 20 years ago?), before we had separate "devel" and "regular" packages, the same package included both the libsomething.so and libsomething.so.2 files. But when the division into separate "devel" packages we established, people realized that:

  1. To run an application using libsomething, you just need libsomething.so.2 only.
  2. To compile an application using libsomething, you need libsomething.so (and also libsomething's header files).

So if you want to divide the package's files into something needed at runtime (the regular package) vs. things needed to build to build with that library (the -devel package), libsomething.so.2 and libsomething.so find themselves in separate packages.

This is why applications (or other libraries) linked with libsomething require libsomething.so.2, but a user who wants to compile with it, needs to use -lsomething (libsomething.so), so this is what the .pc file refers to. It's deliberate, it's not a bug.

What are the dependencies of gnutls-devel?

$ rpm -qR gnutls-devel /bin/sh /bin/sh /sbin/install-info /sbin/install-info /usr/bin/pkg-config gnutls(x86-64) = 3.6.5-2.fc29 gnutls-c++(x86-64) = 3.6.5-2.fc29 gnutls-dane(x86-64) = 3.6.5-2.fc29 libgnutls-dane.so.0()(64bit) libgnutls.so.30()(64bit) libgnutlsxx.so.28()(64bit) pkgconfig pkgconfig(gnutls) pkgconfig(hogweed) pkgconfig(libtasn1) pkgconfig(nettle) pkgconfig(p11-kit-1) rpmlib(CompressedFileNames) <= 3.0.4-1 rpmlib(FileDigests) <= 4.6.0-1 rpmlib(PayloadFilesHavePrefix) <= 4.0-1 rpmlib(PayloadIsXz) <= 5.2-1

Indeed. Fedora is telling you: To build an application (like Seastar) which uses gnutls, you do NOT need libunistring-devel (not listed as a dependency). If the fedora guys agreed with the way you interpret pkg-config --static, they would have to add a dependency from libgnutls-devel to libunistring-devel et al. But they didn't. Because they don't agree with you that the proper way to link with gnutls is -lgnutls -ltspi -lgmp /usr/lib64/libunistring.so -lidn2 -lnettle -lhogweed -lgmp -lnettle -ltasn1 -lp11-kit (the output of pkg-config --static). The proper way is to link just with -lgnutls.

Conclusion

The pkg-config file for gnutls-devel specifies requirements in its private fields which are not actually on the system when the package is installed.

When additional packages are installed for the private dependencies of GnuTLS, then the files referenced in gnutls.pc are available, and invoking pkg-config at the root with --static will work.

Therefore, this is an issue either in Fedora or in the GnuTLS itself and the gnutls.pc file it copies or generates.

Look, I admit it's very easy to look away and just install more packages and be done with all this tiring discussion (sorry...).

But look at what the libunistring-devel package installs. Things like header files and documentation for developers. NONE of that is needed to build Seastar or Seastar applications. The fact we tell people they need to compile against libseastar.a with pkg-config --static --libs seastar and that forces you to install these unneeded header files and documentation, is our bug. It's not a serious bug, not at all. But it saddens me because it's a regression (we never needed these packages before) and I think demonstrates we are misunderstanding the how pkg-config should be used.

nyh commented 5 years ago

On Wed, Jan 16, 2019 at 6:54 PM Jesse Haber-Kucharsky < notifications@github.com> wrote:

Some possible solutions based on the above conclusion:

  • File a bug report with Fedora
  • File a bug report with GnuTLS
  • Add the extra development packages of GnuTLS to install-dependencies.sh with a note referencing this issue (and possibly the above bug report(s))
  • Provide our own gnutls.pc file
  • Create symbolic links to the un-suffixed shared library files in install-dependencies.sh (@eliransin https://github.com/eliransin's interesting idea)

Or, the solution I suggest:

As long as Seastar has a static library only, put Seastar's direct dependencies (now listed on Libs.private) into Libs. Later, when Seastar also has a shared-library version, Seastar.pc will go back to exactly what it has now (and pkg-config --libs seastar will pick up the shared version).

Yes, I know you don't like it. But I actually think this is the correct thing to do.

hakuch commented 5 years ago

On Wed, Jan 16, 2019 at 6:23 PM Jesse Haber-Kucharsky < @.***> wrote: This means that when gnutls-devel was packaged, it was linked against libunistring.so.2. Notice the suffix of .2! However, the pkg-config file references libunistring.so without the suffix! First of all, it's indeed a bug that the pkg-config refers to the pathname "libunistring.so", it should refer to "-lunistring" (I opened https://gitlab.com/gnutls/gnutls/issues/675). But that doesn't change anything regarding what you're saying

This doesn't make sense to me.

We agree that there is a bug in the gnutls-devel RPM (or possibly in GnuTLS, though I think it's a Fedora issue), and that a temporary work-around is to add the packages which should be dependencies to install-dependencies.sh.

It is absolutely not worth "throwing the baby out with the bathwater" (all the benefits of a usable seastar.pc in multiple contexts, including installing it to the system, using it from the build directory, supporting shared or static variants, and allowing for the possibility of packaging Seastar into its own RPM or DEB) when the solution is so straightforward.