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

linux: Using system versions of libraries #13073

Closed
1 task done
ReillyBrogan opened this issue Jun 14, 2024 · 4 comments · Fixed by #13074
Closed
1 task done

linux: Using system versions of libraries #13073

ReillyBrogan opened this issue Jun 14, 2024 · 4 comments · Fixed by #13074
Labels

Comments

@ReillyBrogan
Copy link

Check for existing issues

  • Completed

Describe the feature

We just got done packaging the pre-release of Zed 0.139.3 on Solus so this is part feature request, part bug report, and part guide for other packagers on how to de-bundle Zed.

Linux distributions strongly prefer when applications use system versions of libraries instead of bundled versions for the following reasons (listed roughly in order of priority)

  1. Security: When a CVE is detected in a library the distribution can patch or update just that library and every application that links against it is protected against that vulnerability. If a library is bundled then all bundling applications also need to be identified and updated individually (which can be difficult when libraries build specific versions as other things may need to be updated). For this discussion, libcurl and openssl are the two libraries that are most relevant as they bundled by Zed and the most likely to have CVEs reported
  2. Configuration: Some distributions apply patches to libraries in order to better make them conform to certain standards or otherwise fit in the distribution. Bundled versions of the libraries will behave inconsistently without those patches
  3. Disk space: Removing bundled versions of libs from a package reduces disk space use both in terms of binary size as well as debug symbols size.
  4. Performance: Some distributions perform BOLTing, PGO compilation or other advanced build techniques on their packages that can dramatically improve performance. Such techniques are usually not applied to bundled versions of libs and so they may be slower than using the system versions. Also, common system libs are often in the system page cache and initialization of them can be much faster than bundled versions.
    (I feel like I'm missing a couple of reasons but I'll edit them in later)

While working on our Zed package I went through and identified all native libs that it was bundling that it could be using system versions for, and here are the list of them and how each can be resolved:

Openssl:

This is IMO the most important library that is bundled and the one that is most likely to see CVEs that need it to be patched. It is also the package that is most frequently patched to work with distribution policies.

Diff
diff --git a/crates/client/Cargo.toml b/crates/client/Cargo.toml
index b502c2d1e..5531a68da 100644
--- a/crates/client/Cargo.toml
+++ b/crates/client/Cargo.toml
@@ -19,7 +19,7 @@ test-support = ["clock/test-support", "collections/test-support", "gpui/test-sup
 anyhow.workspace = true
 async-recursion = "0.3"
 async-tungstenite = { version = "0.16", features = ["async-std", "async-native-tls"] }
-async-native-tls = { version = "0.5.0", features = ["vendored"] }
+async-native-tls = { version = "0.5.0" }
 chrono = { workspace = true, features = ["serde"] }
 clock.workspace = true
 collections.workspace = true
@@ -61,7 +61,7 @@ util = { workspace = true, features = ["test-support"] }
 http = { workspace = true, features = ["test-support"] }
 
 [target.'cfg(target_os = "linux")'.dependencies]
-async-native-tls = {"version" = "0.5.0", features = ["vendored"]}
+async-native-tls = {"version" = "0.5.0"}
 # This is an indirect dependency of async-tungstenite that is included
 # here so we can vendor libssl with the feature flag.
 [package.metadata.cargo-machete]

libcurl:

This is another frequent source of CVEs and is also frequently patched, though not as often as openssl

Diff
diff --git a/Cargo.toml b/Cargo.toml
index 72dd44a38..96f82af16 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -296,7 +296,6 @@ ignore = "0.4.22"
 indoc = "1"
 # We explicitly disable http2 support in isahc.
 isahc = { version = "1.7.2", default-features = false, features = [
-    "static-curl",
     "text-decoding",
 ] }
 itertools = "0.11.0"

sqlite3:

This library is ubiquitous and IMO there is no reason not to use the system version.

Diff
diff --git a/crates/sqlez/Cargo.toml b/crates/sqlez/Cargo.toml
index 98b05a06e..b97001dd8 100644
--- a/crates/sqlez/Cargo.toml
+++ b/crates/sqlez/Cargo.toml
@@ -14,7 +14,7 @@ collections.workspace = true
 futures.workspace = true
 indoc.workspace = true
 lazy_static.workspace = true
-libsqlite3-sys = { version = "0.26", features = ["bundled"] }
+libsqlite3-sys = { version = "0.26", features = ["buildtime_bindgen"] }
 parking_lot.workspace = true
 smol.workspace = true
 thread_local = "1.1.4"

libzstd:

This library is also fairly ubiquitous now and is also frequently PGO and BOLT compiled in order to make it faster.

Diff
diff --git a/crates/rpc/Cargo.toml b/crates/rpc/Cargo.toml
index b197073b7..d77ed09fa 100644
--- a/crates/rpc/Cargo.toml
+++ b/crates/rpc/Cargo.toml
@@ -33,7 +33,7 @@ serde_json.workspace = true
 strum.workspace = true
 tracing = { version = "0.1.34", features = ["log"] }
 util.workspace = true
-zstd = "0.11"
+zstd = { version = "0.11", features = [ "pkg-config" ] }
 
 [build-dependencies]
 prost-build.workspace = true

Fontconfig/freetype2

These libraries are already linked against if present and the devel packages are present. Libraries are linked against unless RUST_FONTCONFIG_DLOPEN=on is defined during the build.

There is something broken here though. Our ABI scanning tools detect that libfontconfig and libfreetype2 are only linked against when Zed is built in dev profile. When it's built in release profile they are not linked against and using file open detection tools I confirmed that neither library was loaded when launching in release profile when they were with the dev profile.

ALSA

Linked against as part of the build. I didn't check whether or not this was optional (based on detecting the pkgconfig). I did look into what was using ALSA directly instead of linking against something modern like libpulse and it looks like an issue with the CPAL Rust library not supporting pulse. They do have a PR open for using PipeWire APIs so hopefully we can get this using a modern audio library instead

libgit2

As anyone familiar with anything that links against libgit2 can tell you libgit2 bindings are notoriously flaky and are almost always pinned to a very specific range of libgit2 versions. Our version on Solus is slightly too old for it so I didn't really look into it too much, but it appears that your system libgit2 library should be used automatically assuming it's new enough (At or above v1.7.2 but not v1.8.0). TBH given the fragility of these bindings I'd probably make sure that libgit2 devel packages are available in your build root and if they're used great if not then it's probably not worth the hassle and just mark in your package metadata that the package bundles libgit2.

libz

I also didn't look too much into it but it looks like it builds a bundled libz library but uses the system one via dlopen() if compatible (which almost all distro builds are going to be). I confirmed that it loaded the libz library at startup.

Wayland

The wayland libraries are built and bundled from included headers and if the system has a compatible version (IE all systems with anything even remotely Wayland-related installed) then the system one is dlopen()ed. I don't think this can actually use the bundled version in any scenario and in fact might just build the bindings for Wayland (didn't check that far enough)

(If anyone is interested in it here is our complete patch that we apply as part of the build).

Regarding making this easier for building for distributions without requiring patches it seems to me that we could modify the build.rs files of the given crates in order to change the features during the build if certain environmental variables are set, and defaulting to bundled if not. This would also improve things for the Flatpak since they also provide a runtime with most (all?) of these libs that they keep updated without requiring the application layer to be rebuilt on updates.

If applicable, add mockups / screenshots to help present your vision of the feature

No response

@ReillyBrogan ReillyBrogan added admin read Pending admin review enhancement [core label] triage Maintainer needs to classify the issue labels Jun 14, 2024
@notpeter notpeter added installer / updater Feedback for installation and update process linux labels Jun 14, 2024
@ConradIrwin
Copy link
Collaborator

@ReillyBrogan Thanks for writing this up!

I'd just begun the process of doing something similar in parallel. I'll take a pass at merging the suggestions to dynamically link soon; and in the longer term I'd love to move away from libssl/libcurl if we can avoid them

@ReillyBrogan
Copy link
Author

There should probably be a separate issue about not linking to Fontconfig/Freetype2. That one is strange, perhaps LTO in the release build is optimizing something necessary away? The yeslogic-fontconfig-sys library version is almost two years old, it might be an upstream issue that is already resolved.

Actually, in general it seems that a lot of dependencies are rather out of date. Does Zed have dependabot/renovate setup to keep those updated? The configuration might need looking at if so.

I also noticed extraneous Cargo.lock files throughout the repo that I'm pretty sure are unused. They are probably leftover from a migration of some kind or use of an old cargo version and can likely be deleted without issue.

@ConradIrwin
Copy link
Collaborator

@ReillyBrogan I think we're using font-config for Mac, but probably don't need it on linux.

In general we don't have anything automatically updating dependencies (though the codebase is now old enough we probably should).

@ReillyBrogan
Copy link
Author

In general we don't have anything automatically updating dependencies (though the codebase is now old enough we probably should).

Yeah, this is going to be problematic as most distributions have policies in place for cargo builds that require the use of the --frozen flag to build with the exact dependencies specified in the Cargo.lock file (for reproducibility mainly, and also since it implies that those versions are what the developers used and tested with).

With how many dependencies you use the only scalable solution is an automated dependency management tool like Dependabot or Renovate. Since you seem to have many Rust repositories Renovate is probably going to be a better choice since you can define a common configuration in one repo and have your other ones inherit from that config. It'll probably be a bit painful to get caught up but once you are and have it configured to your preference it's much easier to stay updated (most people end up with a weekly or bi-weekly PR with all updates bundled rather than a single PR-per-update).

I think we're using font-config for Mac, but probably don't need it on linux.

That would explain why it's not linking in the release profile with dead code elimination removing the need for the linker to do so. That's interesting that you're not using it on Linux, I would have assumed you still would as fontconfig is more or less the core font configuration library for most (all?) Linux distros. Would that mean that Zed isn't obeying the system-wide defaults for say, monospace fonts? That wouldn't be ideal from a user perspective since most users would prefer that it uses the system-wide default monospace font unless they manually specified an alternative in the Zed config.

@Moshyfawn Moshyfawn removed triage Maintainer needs to classify the issue installer / updater Feedback for installation and update process labels Jun 15, 2024
@notpeter notpeter removed the admin read Pending admin review label Jun 17, 2024
mikayla-maki added a commit that referenced this issue Jun 21, 2024
Fixes #13073

Note that, contrary to the issue's text, we're still shipping a
statically bundled sqlite3 after this PR. We use enough new features of
sqlite, like `sqlite3_error_offset` and `STRICT`, that our minimum
version (v3.38.0) is higher than is presumably accessible on Ubuntu.

Release Notes:

- N/A

---------

Co-authored-by: Mikayla <mikayla@zed.dev>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants