[WIP] bazel: Allow to link third party libraries dynamically#4585
[WIP] bazel: Allow to link third party libraries dynamically#4585vadorovsky wants to merge 1 commit intoenvoyproxy:masterfrom vadorovsky:dynamic-linking
Conversation
htuch
left a comment
htuch
left a comment
There was a problem hiding this comment.
Seems reasonable to enable as opt-in, thanks.
There was a problem hiding this comment.
We shouldn't have to push this out to individual BUILD files; can't we hide the details inside envoy_build_system.bzl, in the envoy_cc_library definition?
There was a problem hiding this comment.
Is dynamic linking really orthogonal to both OS and CPU architecture?
There was a problem hiding this comment.
It definitely should work the same for those libraries on Linux, *BSD and OS X. Do we support any other platforms?
There was a problem hiding this comment.
And handling different CPU architectures on RPM-based distros (which use /usr/lib64 dir instead of /usr/lib for 64-bit systems) is the reason why I decided to use glob there.
There was a problem hiding this comment.
Can you comment on how you expect the dynamic libraries to be distributed? As things stand, once you do a dynamic link, the .so artifacts that you rely on live in the Bazel cache. How would a user be expected to bundle them up for distribution? Or, is the idea that the user installs these libraries from completely independent sources always (as is generally the case for dynamic deps)? In the latter case, maybe spell this out in the docs.
There was a problem hiding this comment.
The last question is correct. I expect dynamic libraries just to be in /usr/lib(64) in form of .so files, which usually means installing those libraries from packages (like libevent-dev or libevent-devel, depends on distrubition) or from manual compilation (./configure --disable-static --enable-shared && make && make install). But as you mentioned in the last question, they might come from completely independent sources.
I will write it as a comment and in README.
My main motivation behind this PR is that I would like to package Envoy in openSUSE. We already ship all libraries needed by Envoy only as shared libraries. Generally we don't accept shipping static libraries unless we have really good reasons for doing so.
vadorovsky
changed the title
There was a problem hiding this comment.
Can you factor this pattern out into a macro? I think if it's just a single return select(...) that Skylark is able to compose this.
There was a problem hiding this comment.
I would still argue that version numbers etc. don't belong in general BUILD files. Ideally we capture this kind of detail alongside the repository_locations.bzl or somewhere else confined to bazel/ directory.
There was a problem hiding this comment.
I have even better idea. Since libluajit version may vary in different distributions, it might be reasonable to use pkg-config for outputing proper flags. Like i.e.:
$ pkg-config --libs luajit
-lluajit-5.1
or
$ pkg-config --libs libevent_pthreads
-pthread -levent_pthreads -levent
I will try to write some macro or function for that in bazel/ dir.
There was a problem hiding this comment.
Yeah, trying to hide this sounds nice, but using pkg-config is kind of non-hermetic, so that might be hard to fit into the Bazel model. I'd like to loop our resident Bazel expert @jmillikin-stripe into this discussion and also @irengrig from the Bazel development team who has been working on better integration with external build systems.
From a distribution perspective (i.e. SUSE) perspective, you probably don't even want to be using the version of LuaJIT that Envoy is pulling down from GitHub, you probably want to point at the existing installation in /usr/lib. This would involve setting up these external dependencies yourself in your WORKSPACE. I.e. before calling envoy_dependencies(), you already establish what //external:luajit and use the skip_targets arg.
There was a problem hiding this comment.
bazel allow toolchains to rely on system installed libararies, so if you want to use headers/libraries from /usr, you may just need to generate your own https://github.com/envoyproxy/envoy/blob/master/ci/prebuilt/BUILD file with correct path.
There was a problem hiding this comment.
@htuch Yes, being able LuaJIT and the other dependencies available in /usr/lib would be great.
@lizan OK, so I get that relying on system installed libraries is possible. Can you point me to some documentation or give some advice how should I generate my own BUILD file? Where exactly (in which directory) should I put this file?
Sorry for asking that basic questions, but using Google and looking at the documentation didn't make that clear for me and I feel lost.
There was a problem hiding this comment.
@lizan And another question. Is there something wrong with my attempt of changing ci/prebuilt/BUILD in this PR? third_party/... means /usr/... in practice, so I already teached this file to look for .so files in /usr/lib*.
There was a problem hiding this comment.
Can you comment on how you expect the dynamic libraries to be distributed? As things stand, once you do a dynamic link, the .so artifacts that you rely on live in the Bazel cache. How would a user be expected to bundle them up for distribution? Or, is the idea that the user installs these libraries from completely independent sources always (as is generally the case for dynamic deps)? In the latter case, maybe spell this out in the docs.
|
@mrostecki The mac CI job looks genuinely broken, can you take a look? |
Provide an option `--define dynamic_linking=enabled` which allows to link third-party libraries dynamically. An another change is using `glob` function to resolve library paths. Some Linux distributions use /usr/lib64 directory instead of /usr/lib (if system is 64-bit). To fix build on them, the wildard expression like `thirdparty/lib*/name_of_library.a` needs to be used. Risk Level: Low Testing: None, but some CI tests in future could be added Docs Changes: Option described in bazel/README.md Release Notes: n/a Signed-off-by: Michal Rostecki <mrostecki@suse.de>
|
@htuch I added macros, I still need to investigate the CI failure on mac and add more info to docs. I will do that on Monday. |
|
This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
stale
Bot
added
the
stale
|
I'm trying to make Envoy build working there: https://build.opensuse.org/package/show/home:mrostecki/envoy It's not building successfully yet, but almost... And here are my patches: https://build.opensuse.org/package/view_file/home:mrostecki/envoy/bazel-link-third-party-libraries-dynamically.patch?expand=1 So, the motivation behind my work there is that:
For now, libraries from
Sorry if it sounds a little bit harsh, that's not my intention. I know that our (SUSE vs Envoy upstream developers) philosophies about building and distributing software differ a lot. And what I'm trying to do is to reach some agreement. We prefer dynamic linking of dependencies, because if there is some bug or security vulnerability, we want to apply the fix once for the whole Linux distribution. Sometimes we would like to do that downstream (it means - apply some patches before they are accepted upstream, or cherry-pick some commit before it's released, i.e. to fix customer's environment). Bundling libraries for each piece of software means that we need to care about applying fixes and upgrades multiple times and tracking that kind of activity is really painful. For now I'm closing this pull request. It cannot be merged in the current shape. I will let you know (especially @lizan, thanks for all your help and guidance!) how my efforts of packaging Envoy go. At the point when I will have fully working package and I will ensure that I dynamically linked as much as possible, I will tell you. And then we can start discussing about upstreaming that (or whether it even makes sense to upstream that). |
|
@mrostecki Over time we expect our external dependency handling story to improve, based on work that @irengrig is doing to allow Bazel to natively support external cmake builds. At that time, we should see convergence of the different build approaches that currently exist in Envoy for external deps (see https://github.com/envoyproxy/envoy/blob/master/bazel/EXTERNAL_DEPS.md). We don't really like the situation any more than you do; it is what it is because we wanted to have Bazel reasonably work with our OSS dependencies after the cmake migration and native support was lacking. |
|
@mrostecki Glad to see you make some progress, but yeah I know the situation around dependency management is not great today.
This is great, particularly I like your patch to specify all sha256 sums to used with
You should be able to do this with similar techniques you did with //ci/prebuilt/BUILD for some of them. For example, tclap, spdlog. Though some of them are header only so I'm not sure how much gain you would get.
Those are build time dependency only. What's your policy about them?
I understand this philosophical difference between us and distribution maintainers. Note that to make this work the library would be maintaining ABI compatibility. For example BoringSSL that Envoy bundles doesn't guarantee any ABI compatibility. Anyway, let me know if you have any progress or any question. |
|
@htuch Glad to hear about the ongoing work on external cmake bulds. I will keep an eye on it.
Sure, I wil ltry to sumit a PR today.
Even if some dependencies are header only, I still would like to use
If possible, I would like to still use buld time dependencies from system. Especially, I would like to not bundle Go. Bundling Python dependencies for build only maybe will be acceptable for us, but still, I will try to not bundle them if possible.
Yes, I'm aware of that.
Actually, we will make an expection (hopefully the only one) for BoringSSL and bundle it. So we are fine with using |
|
PR about --distdir #4779 |
3 participants
Description:
Provide an option
--define dynamic_linking=enabledwhich allowsto link third-party libraries dynamically.
An another change is using
globfunction to resolve librarypaths. Some Linux distributions use /usr/lib64 directory instead
of /usr/lib (if system is 64-bit). To fix build on them, the
wildard expression like
thirdparty/lib*/name_of_library.aneedsto be used.
Risk Level: Low
Testing: None, but some CI tests in future could be added
Docs Changes: Option described in bazel/README.md
Release Notes: n/a
Signed-off-by: Michal Rostecki mrostecki@suse.de