Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

install-dependencies.sh: add more packages for static linkage #1997

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

tchaikov
Copy link
Contributor

@tchaikov tchaikov commented Dec 11, 2023

when statically link against Seastar, its dependencies pull in more dependencies. but install-dependencies.sh cannot fulfill these needs. but in the README.md, we are using an example to statically link against Seastar, so let's add the missing bits to install-dependencies.sh .

@tchaikov tchaikov requested a review from denesb December 11, 2023 11:53
@avikivity
Copy link
Member

Do we encourage static linking?

@tchaikov
Copy link
Contributor Author

Do we encourage static linking?

not explicitly. see https://github.com/scylladb/seastar#using-seastar-from-its-build-directory-without-installation .

and search for static.

@mykaul
Copy link

mykaul commented Dec 11, 2023

(I wonder what we statically build against systemd-devel )

@tchaikov
Copy link
Contributor Author

(I wonder what we statically build against systemd-devel )

please see the commit message of ef878cc

@tchaikov
Copy link
Contributor Author

tchaikov commented Dec 11, 2023 via email

@mykaul
Copy link

mykaul commented Dec 11, 2023

(I wonder what we statically build against systemd-devel )

please see the commit message of ef878cc

Still can't figure it out. Unlikely to be

udev = pyudev.Devices.from_device_file(pyudev.Context(), dev_node)

(but I'm nitpicking, feel free to ignore - was more curious about it)

@tchaikov
Copy link
Contributor Author

(I wonder what we statically build against systemd-devel )

please see the commit message of ef878cc

Still can't figure it out. Unlikely to be

udev = pyudev.Devices.from_device_file(pyudev.Context(), dev_node)

(but I'm nitpicking, feel free to ignore - was more curious about it)

lemme elaborate on it a little bit:

$ pkg-config --libs --cflags --static hwloc
-lhwloc -lm -ludev -lpthread

we use pkg-config to retrieve the compile and link flags. and we link against hwloc for retrieving the hardware locality information of the system. see

#include <hwloc.h>
.

if we want to link to libseastar.a, we would need to use the output of pkg-config --libs --cflags --static hwloc as the link flags. so hwloc wants to link against libudev.a or libudev.so. these files are typically packaged by development package, libudev.so is a symlink to something like libudev.so.1, as as the so libraries are always versioned. so to ensure the the linker can have access to libudev.so, we have to install it using the package including it.

Seastar supports both static and dynamic linkage. and we have
command line example in README.md to statically link against
Seastar. this means, all of its linkages are statically linked.

if hwloc is statically linked. it would bring into libudev, See

```
$ pkg-config --libs --cflags --static hwloc
-lhwloc -lm -ludev -lpthread
```
on Debian and its derivative distros, libudev.so is packaged in
libudev-dev, while on Fedora and its derivative distros, libudev.so
in systemd-devel.

in this change, these packages are included in install-dependencies.sh,
so the building system can have access to them when linking Seastar
statically. please note, these packages are not dependencies of
libhwloc-dev or hwloc-devel. Because, in general, distro packacage
guidelines encourage maintainers to package shared library if possible,
and reuse the shared libraries redistributed by the distro whenever
appropriate.

Signed-off-by: Kefu Chai <kefu.chai@scylladb.com>
Seastar supports both static and dynamic linkage. and in README.md,
we have a command line example to statically link against Seastar.
this means, all of its linkages are statically linked.

if gnutls is statically linked. it would bring into libunistring, See

```
pkg-config --libs --cflags --static gnutls
-I/usr/include/p11-kit-1 -lgnutls -lgmp -lunistring -latomic -lnettle -lhogweed -lgmp -lnettle -ltasn1 -lidn2 -lunistring -lp11-kit
```

in this change, these packages are included in install-dependencies.sh,
so the building system can have access to them when linking Seastar
statically. please note, these packages are not dependencies of
libhwloc-dev or hwloc-devel. Because, in general, distro packacage
guidelines encourage maintainers to package shared library if possible,
and reuse the shared libraries redistributed by the distro whenever
appropriate.

Signed-off-by: Kefu Chai <kefu.chai@scylladb.com>
@tchaikov
Copy link
Contributor Author

will update the commit message and cover letter to be more specific. Regards Kefu Chai Le lun. 11 déc. 2023 à 19:54, Avi Kivity @.> a écrit :

Do we encourage static linking? — Reply to this email directly, view it on GitHub <#1997 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAONPZQRHCG47OH5D5GDKLYI3YA3AVCNFSM6AAAAABAPUEVVOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQNBZHEZDKMZZGE . You are receiving this because you authored the thread.Message ID: @.
>

v2:

  • replace "we encourage encourage developers to statically link" with "in README.md, ..."

@tchaikov
Copy link
Contributor Author

if this change is approved. we might want to redo the image along with the change of #1999

@tchaikov
Copy link
Contributor Author

@scylladb/seastar-maint hello maintainers, could you help review this change?

@nyh
Copy link
Contributor

nyh commented May 16, 2024

In #2244 @denesb seems to suggest that libudev-devel is needed not just for static linking but to build Seastar. If that's the case we should add it without suggesting it is for static linking.

But regarding static linking, I am not convinced by your rationale: "install-dependencies.sh" should be what is required to build Seastar, including running its official test suite - but not to make completely general use of it in any concievable way. If nothing in the build process does static linking, we don't need packages that are only used for static linking. The user would install those packages on their own if they want to do static linking, just like they would install "vi" if they want to edit some C++ code using that editor.

@tchaikov
Copy link
Contributor Author

If nothing in the build process does static linking

could you define "the build process"? our README.md actually implies a build process with following steps:

  1. sudo ./install-dependencies.sh
  2. ./configure.py --mode=release
  3. ninja -C build/release

but without the packages installed by this pull request. the above process fails. as unlike the "debug" build, the "release" build actually builds seastar as static libraries.

@nyh
Copy link
Contributor

nyh commented May 16, 2024

our README.md actually implies a build process with following steps:

1. `sudo ./install-dependencies.sh`

2. `./configure.py --mode=release`

3. `ninja -C build/release`

but without the packages installed by this pull request. the above process fails. as unlike the "debug" build, the "release" build actually builds seastar as static libraries.

So you should have opened with that :-) Yes, of course, ninja -C build/release should work. If that needs static libraries, then so be it (I don't know why, but if it's a fact, we need to accept it...).

But... How does our CI always manage to build the release mode if we didn't have this dependency?

@tchaikov
Copy link
Contributor Author

tchaikov commented May 16, 2024

sorry, i should have been less misleading. actually there should have been a step 4. where user uses the built seastar to build his/her application. when building it, the building system in turn consumes the .pc file, which encodes the missing dependency. this is elaborated in the commit message already.

@nyh
Copy link
Contributor

nyh commented May 16, 2024

sorry, i should have been less misleading. actually there should have been a step 4. where user uses the built seastar to build his/her application. when building it, the building system in turn consumes the .pc file, which encodes the missing dependency. this is elaborated in the commit message already.

Ok, so this is different from what you said. It sounds like you're now saying that "ninja -C build/release" does work fully without these packages, but then later, when you try to use it using the ".pc", things will be missing. I am not sure that this is Seastar's concern, any more than Seastar doesn't require you to install "vi" or "gdb" just because as an application developer you are likely to need those too.

Beyond the "waste" of requiring more packages for users that don't need them, what bothers me most about this patch is that I don't understand its story of static libraries at all. Why are all sorts of "-devel" packages needed, when none of those actually installs static libraries (.a). You even mention libudev.so in your discussion of static linking, which doesn't make sense. What am I missing?

@tchaikov
Copy link
Contributor Author

sorry, i should have been less misleading. actually there should have been a step 4. where user uses the built seastar to build his/her application. when building it, the building system in turn consumes the .pc file, which encodes the missing dependency. this is elaborated in the commit message already.

Ok, so this is different from what you said. It sounds like you're now saying that "ninja -C build/release" does work fully without these packages, but then later, when you try to use it using the ".pc", things will be missing. I am not sure that this is Seastar's concern, any more than Seastar doesn't require you to install "vi" or "gdb" just because as an application developer you are likely to need those too.

if the consuming of .pc is not seastar's concern, i think i will just close this PR.

Beyond the "waste" of requiring more packages for users that don't need them, what bothers me most about this patch is that I don't understand its story of static libraries at all. Why are all sorts of "-devel" packages needed, when none of those actually installs static libraries (.a).

i explained this in 3cd63c8

You even mention libudev.so in your discussion of static linking, which doesn't make sense. What am I missing?

yes, for the sake of completeness and correctness, i mentioned both the shared library and static library. it still makes sense, after re-reading my comment at #1997 (comment) .

@nyh
Copy link
Contributor

nyh commented May 16, 2024

Ok, so this is different from what you said. It sounds like you're now saying that "ninja -C build/release" does work fully without these packages, but then later, when you try to use it using the ".pc", things will be missing. I am not sure that this is Seastar's concern, any more than Seastar doesn't require you to install "vi" or "gdb" just because as an application developer you are likely to need those too.

if the consuming of .pc is not seastar's concern, i think i will just close this PR.

But I think @denesb suggested in his PR that some of these packages are needed not because of static linking.

Beyond the "waste" of requiring more packages for users that don't need them, what bothers me most about this patch is that I don't understand its story of static libraries at all. Why are all sorts of "-devel" packages needed, when none of those actually installs static libraries (.a).

i explained this in 3cd63c8

I still don't understand. You say "if hwloc is statically linked. it would bring into libudev", and then talk about where libudev.so comes from. But where does libudev.a come from? Why don't we need it? I looked now, and I can't find libudev.a anywhere on Fedora - how come this is not a problem, and how does the systemd-devel package (which doesn't contain it!) help static linking?

Can you please give a complete example of a problem that this PR solves, maybe I'll finally understand?

@denesb
Copy link
Contributor

denesb commented May 17, 2024

But I think @denesb suggested in his PR that some of these packages are needed not because of static linking.

Turns out -ludev is only needed to link statically against libseastar.a, not for building it.

@tchaikov
Copy link
Contributor Author

tchaikov commented May 17, 2024

Ok, so this is different from what you said. It sounds like you're now saying that "ninja -C build/release" does work fully without these packages, but then later, when you try to use it using the ".pc", things will be missing. I am not sure that this is Seastar's concern, any more than Seastar doesn't require you to install "vi" or "gdb" just because as an application developer you are likely to need those too.

if the consuming of .pc is not seastar's concern, i think i will just close this PR.

But I think @denesb suggested in his PR that some of these packages are needed not because of static linking.

Beyond the "waste" of requiring more packages for users that don't need them, what bothers me most about this patch is that I don't understand its story of static libraries at all. Why are all sorts of "-devel" packages needed, when none of those actually installs static libraries (.a).

again, it's explained in the commit message of 3cd63c8, i am quoting part of it so that we can continue the discussion in a more efficient way:

Because, in general, distro packacage guidelines encourage maintainers to package shared library if possible,
and reuse the shared libraries redistributed by the distro whenever appropriate.

i explained this in 3cd63c8

I still don't understand. You say "if hwloc is statically linked. it would bring into libudev", and then talk about where libudev.so comes from. But where does libudev.a come from? Why don't we need it? I looked now, and I can't find libudev.a anywhere on Fedora - how come this is not a problem, and how does the systemd-devel package (which doesn't contain it!) help static linking?

the reason why i mentioned libudev.a was for the sake of completeness. not because it is provided by the pre-built packages. we don't need libudev.a. what we need is libudev.a or libudev.so. but since libudev.so is packaged, we can and should use it. please read #1997 (comment) for more details.

Can you please give a complete example of a problem that this PR solves, maybe I'll finally understand?

sure. i am basically quoting https://github.com/scylladb/seastar?tab=readme-ov-file#using-an-installed-seastar:

  1. sudo ./install-dependencies.sh

  2. ./configure.py --mode=release --prefix=/usr/local

  3. ninja -C build/release install

  4. g++ my_app.cc $(pkg-config --libs --cflags --static seastar) -o my_app

    developer wants to use the built seastar, via its .pc file. but the .pc file references the output of pkg-config --libs --cflags --static hwloc as seastar's link flags, like

    $ pkg-config --libs --cflags --static hwloc
    -lhwloc -lm -ludev -lpthread

    but neither libudev.so nor libudev.a is available in a base system

  5. fortunately, libudev.so is packaged by libudev-dev, so we can install it to fulfill this needs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants