Build libdatadog using builder crate by hoolioh · Pull Request #24 · DataDog/libdatadog-rb

hoolioh · 2026-03-27T13:48:40Z

Build libdatadog using builder crate

Why?

The previous approach hardcoded SHA256 checksums and download URLs for each platform release in LIB_GITHUB_RELEASES (Rakefile), required the http gem, and needed manual updates on every libdatadog release.

Two parallel PRs tackled this independently: #24 introduced the full build-from-source + pre-built download workflow with supply-chain hardening, and #33 introduced a cleaner modular structure using namespaced tasks, Pathname-based path handling, and a dedicated tasks/ directory. This PR consolidates both, taking the best of each.

What does this PR do?

Replaces the old fetch-and-extract workflow with two complementary tasks:

libdatadog:build — builds libdatadog from source for the current platform using libdatadog's own builder crate. Requires a Rust toolchain.

Fetches the builder via cargo install --rev <LIB_COMMIT_SHA> --locked (pinned to a commit SHA, not a mutable tag, for supply-chain safety)
Supports local development via LIBDATADOG_SOURCE=/path/to/libdatadog
Supports feature overrides via LIBDATADOG_FEATURES
Outputs artifacts directly to vendor/libdatadog-<version>/<platform>/

fetch_release_artifacts — downloads pre-built tarballs from the GitHub release for all supported platforms into vendor/. Used by CI during publish; no Rust required.

The package task is now driven by the gemspec, which automatically includes all files found under vendor/ for the supported platforms. The hardcoded per-platform gem list in push_to_rubygems is replaced with a Dir.glob.

Structure (from #33)

Build logic lives in tasks/build.rake with a modular BuildFromSource structure (Target, Paths, Builder sub-modules) using Pathname throughout. Tasks are namespaced under libdatadog: (libdatadog:build, libdatadog:clean), keeping the top-level Rakefile focused on packaging and release. The libdatadog:clean task removes intermediates (tmp/) and vendor output in one step.

Security (from #24)

cargo install uses --rev <LIB_COMMIT_SHA> --locked rather than a mutable git tag, ensuring the builder binary is reproducibly pinned to a specific commit. LIB_COMMIT_SHA is tracked alongside LIB_VERSION in version.rb.

How to test the change?

Build from source (requires Rust 1.84.1+):

bundle exec rake libdatadog:build

Build from a local libdatadog checkout:

LIBDATADOG_SOURCE=/path/to/libdatadog bundle exec rake libdatadog:build

Download pre-built artifacts and package the gem locally:

bundle exec rake package_from_github

Clean up intermediates and vendor output:

bundle exec rake libdatadog:clean

Additional Notes

Drops the http gem dependency; uses stdlib net/http instead
Rust toolchain is only needed for libdatadog:build; all other tasks including the default test suite work without it
LIB_COMMIT_SHA added to version.rb alongside LIB_VERSION; both must be updated on each libdatadog release
Nix dev shell updated with rustc, cargo, cmake, and supporting build tools
Consolidates Build libdatadog using builder crate #24 and Build using libdatadog's builder crate #33

lloeki · 2026-03-30T13:29:42Z

  it "prefixes all public symbols in .so files" do
    so_files = Dir.glob("vendor/libdatadog-#{Libdatadog::LIB_VERSION}/**/*.so")
-    expect(so_files.size).to be 4
+    expect(so_files.size).to be >= 1


Something feels off here, I don't think we'd want to be dynamic, as we want to make sure we package the expected files.

Having this to 4 kind of acts as a proxy for exact files; probably we should make it another dedicated test.

I've modified to expect a file for each supported target.

lloeki · 2026-03-30T13:32:59Z

      context "for the current platform" do
        let(:current_platform) do
          platform = Gem::Platform.local.to_s
+          platform = platform[0..-5] if platform.end_with?("-gnu")


Hmm, looks like some normalisation slipped through.

Minor: I don't quite understand why this is needed -- we already had some tests for when the prefix ends with -gnu; why did this part need updating? 👀

lloeki · 2026-03-30T13:34:50Z

+  rustc_output = `rustc -vV`
+  raise "rustc not found or failed" unless $?.success?


I see a couple of tools are needed, either we expect them present or we should probably move this to a dedicated prerequisite task depended upon.

I've added a new task to check that the toolchain is present and write a section in the README to install it.

lloeki · 2026-03-30T13:38:52Z

+    [
+      "cargo", "install",
+      "--path", File.join(source_path, "builder"),
+      "--bin", "release",
+      "--root", install_root,
+      "--force"
+    ]


This shares a lot with the command below (diff: --path vs --git + --tag) and should probably be factored together.

lloeki · 2026-03-30T13:57:57Z

+  puts "Building libdatadog for #{ruby_platform} (#{host_triple})"
+  puts "Output: #{target_directory}"
+
+  system(env, binary, "--out", target_directory) || raise("Builder failed")


How does it know which source tree to build from? Neither source_path nor LIBDATADOG_SOURCE_PATH seem to have been communicated.

LIBDATADOG_SOURCE_PATH is intended to use the builder from a local folder so the builder, when installed, will take that repo as the source tree. Same thing happens when pulled from a tag, when installed it knows that it was installed from that specific tag and will pull all the needed packages from the same reference.

lloeki · 2026-03-30T14:00:30Z

-
-  # macOS package (Apple Silicon)
-  Helpers.package_for(gemspec, ruby_platform: "arm64-darwin", files: Helpers.files_for("arm64-darwin"))
+  built_platforms = Dir.glob("vendor/libdatadog-#{Libdatadog::LIB_VERSION}/*/").map { |d| File.basename(d) }


This makes built_platforms dynamic instead of explicit, which might lead to issues.

+1 the "fallback" gem should always contain all the platforms we specified above and this shouldn't be picked up dynamically. A "fallback" gem without all the platforms is an incorrect fallback gem.

Alternatively, we could allow the gem to be packaged still, but never released on rubygems.org, although since it's easy to build locally I'm not sure it's useful to have this "incomplete fallback gem" build ability.

To be clear this is a blocker in my opinion for merging this PR

lloeki · 2026-03-30T14:01:52Z

+  Helpers.fix_file_permissions_for_gem(gemspec.files)
+
+  # Fallback package with all built binaries
+  Helpers.package_for(gemspec, ruby_platform: nil, files: Helpers.files_for(*built_platforms))


If only one platform is built (e.g the current one) it looks like this would build a ruby package with a single platform; thus a silent failure or other mistake resulting in missing but expected files would produce an incomplete package.

lloeki · 2026-03-30T14:04:52Z

+  built_platforms.each do |platform|
+    Helpers.package_for(gemspec, ruby_platform: platform, files: Helpers.files_for(platform))
+  end


I would suggest three distinct set of tasks:

build for current platform: must run on expected native platform

package binary gem for one platform: can run on any platform but must be provided platform-specific file(s) to package; if platform is same as above it can all be done locally in sequence; otherwise file(s) must come as artifact from another build job

package for ruby (== "all" platforms) gem platform: can run on any platform but must be provided all files to package; file(s) must come as artifact from another build job

In all cases tasks fail when any prerequisite is missing.

This creates a system that cannot fail by being always consistent.

lloeki · 2026-03-30T14:12:01Z

-    "gem push pkg/libdatadog-#{Libdatadog::VERSION}-aarch64-linux.gem",
-    "gem push pkg/libdatadog-#{Libdatadog::VERSION}-arm64-darwin.gem"
-  ].each do |command|
+  Dir.glob("pkg/libdatadog-#{Libdatadog::VERSION}*.gem").each do |gem_file|


Same here, explicit is better than implicit.

We need to explicitly spawn CI jobs on specific kinds of workers anyway so it's not like we're making anything automatic.

ivoanjo

This looks overall sane/reasonable but the dynamic platform detection at this point is dangerous as it would allow us to release in a broken state, so I'm marking as request changes.

ivoanjo · 2026-04-27T07:14:13Z

-  it "prefixes all public symbols in .so files" do
-    so_files = Dir.glob("vendor/libdatadog-#{Libdatadog::LIB_VERSION}/**/*.so")
-    expect(so_files.size).to be 4
-
-    so_files.each do |so_file|
-      raw_symbols = `nm -D --defined-only #{so_file}`
-
-      symbols = raw_symbols.split("\n").map { |symbol| symbol.split(" ").last.downcase }.sort
+  it "prefixes all public symbols in shared library files" do
+    shared_lib_files = Dir.glob("vendor/libdatadog-#{Libdatadog::LIB_VERSION}/**/*.{so,dylib}")
+    expect(shared_lib_files.size).to be >= 1
+
+    shared_lib_files.each do |shared_lib_file|
+      # macOS nm doesn't use a dynamic symbol table (-D); use -g (global) instead
+      nm_flags = shared_lib_file.end_with?(".dylib") ? "-g --defined-only" : "-D --defined-only"
+      raw_symbols = `nm #{nm_flags} #{shared_lib_file}`
+
+      # macOS nm prefixes C symbols with "_"; strip it for consistent matching.
+      # Linker-injected symbols (e.g. __mh_dylib_header) have two leading underscores and
+      # still start with "_" after stripping one — reject them to avoid false failures.
+      symbols = raw_symbols.split("\n")
+        .map { |symbol| symbol.split(" ").last.downcase.sub(/\A_/, "") }
+        .reject { |sym| sym.start_with?("_") }
+        .sort
      expect(symbols.size).to be > 20 # Quick sanity check
      expect(symbols).to all(
        start_with("ddog_").or(start_with("blaze_"))
      )


Minor: This test is so simple that I would soft-suggest breaking it into two: one for Linux, another one for macOS.

That would avoid all the weird branching options for one vs the other as in the end, very little code is shared

ivoanjo · 2026-04-27T07:19:28Z

      context "for the current platform" do
        let(:current_platform) do
          platform = Gem::Platform.local.to_s
+          platform = platform[0..-5] if platform.end_with?("-gnu")


Minor: I don't quite understand why this is needed -- we already had some tests for when the prefix ends with -gnu; why did this part need updating? 👀

ivoanjo · 2026-04-27T07:20:06Z

 /Gemfile.lock

 /vendor/
+/ext/


Minor: Why is an ext folder needed?

ivoanjo · 2026-04-27T07:21:49Z

+  "aarch64-unknown-linux-gnu" => "aarch64-linux",
+  "aarch64-unknown-linux-musl" => "aarch64-linux-musl",
+  "aarch64-apple-darwin" => "arm64-darwin",
+  "x86_64-apple-darwin" => "arm64-darwin"


This is not correct, we deliberately chose to not start supporting the legacy macOS x86-64.

I guess we could've documented it better -- can I ask that you replace this with a comment saying we explicitly are not supporting it?

ivoanjo · 2026-04-27T07:25:28Z

-      target_file_hash = Digest::SHA256.hexdigest(File.read(target_file))
+desc "Build libdatadog FFI library from source using the builder crate"
+task :build_ffi do
+  rustc_output = `rustc -vV`


So I was hoping we could avoid the "everyone builds with a different rust version" kinda thing.

At least, I'd like it to make it opt-in, if possible: That is, by default I think there should be a given rust version enforced by the builder crate, and you need to pass in an extra flag if you want to say "I want to use my own rust".

Would that be possible? This way for instance in the future we could bump the rust version everyone uses in the builder crate itself (+ update our images) rather than having a rag-tag of rust versions that every library picks.

How would that rust version be obtained? A bump means:

everyone's pipeline would break until they figure out how to install the new version

OR

builder installs the necessary rustc via rustup or something; I'm not sure that's in scope

I'm not quite sure of the expected gain here.

Tangent: probably the easiest way to achieve that is via Nix; as in, it's already the case in this repo for Nix users because of flake.lock, making it work across repos would be a mere libdatadog providing their own flake and consumers using it as input. Then you'd get a shot at actual reproducibility.

How would that rust version be obtained? A bump means:

everyone's pipeline would break until they figure out how to install the new version

Yeap, I think that's fine and exactly what I'm suggesting -- if upstream moves, our CI goes red, and we have two options -- pass the --allow-custom-rust-version or update our version, which I think makes sense here?

As a reminder, we don't pick up new major versions of libdatadog anyway from dd-trace-rb, so having this CI needing small updates when something changes upstream is not particularly new.

ivoanjo · 2026-04-27T07:28:44Z

-    end
+  install_cmd = if source_path
+    [
+      "cargo", "install",


Minor: If cargo is missing packaging will fail anyway so not sure it's worth adding an explicit check

ivoanjo · 2026-04-27T07:35:08Z

+
+  env = {
+    "PROFILE" => "release",
+    "OPT_LEVEL" => "3",


+1000 yes please this should be a default in the builder crate and we should not override it unless there's a really good reason to

ivoanjo · 2026-04-27T07:37:44Z

-
-  # macOS package (Apple Silicon)
-  Helpers.package_for(gemspec, ruby_platform: "arm64-darwin", files: Helpers.files_for("arm64-darwin"))
+  built_platforms = Dir.glob("vendor/libdatadog-#{Libdatadog::LIB_VERSION}/*/").map { |d| File.basename(d) }


+1 the "fallback" gem should always contain all the platforms we specified above and this shouldn't be picked up dynamically. A "fallback" gem without all the platforms is an incorrect fallback gem.

Alternatively, we could allow the gem to be packaged still, but never released on rubygems.org, although since it's easy to build locally I'm not sure it's useful to have this "incomplete fallback gem" build ability.

To be clear this is a blocker in my opinion for merging this PR

ivoanjo · 2026-04-27T07:43:22Z

+  def self.fix_file_permissions_for_gem(files)
+    files.each do |path|
+      next unless File.file?(path)
+
+      filename = File.basename(path)
+      current_permissions = File.stat(path).mode & 0o777
+      expected = EXECUTABLE_FILES.include?(filename) ? 0o755 : 0o644
+
+      if current_permissions != expected
+        puts "Fixing permissions for #{path}: #{current_permissions.to_s(8)} -> #{expected.to_s(8)}"
+        FileUtils.chmod(expected, path)
+      end
+    end
+  end


Minor: This looks like a copy of the method after it... why is this needed again?

hoolioh changed the title ~~Use builder crate to build the artifacts.~~ Build libdatadog using builder crate Mar 27, 2026

hoolioh added 2 commits April 21, 2026 12:08

Use builder crate to build the artifacts.

325b694

fix: set proper extension for MacOS files

66e9cd0

hoolioh force-pushed the julio/use-builder-cargo-install branch from c27ac53 to 66e9cd0 Compare April 21, 2026 10:35

fix: set proper flags

39c4952

hoolioh marked this pull request as ready for review April 21, 2026 11:11

hoolioh requested a review from a team as a code owner April 21, 2026 11:11

lloeki reviewed Apr 23, 2026

View reviewed changes

ivoanjo requested changes Apr 27, 2026

View reviewed changes

refactor: use builder-locally and fetch binaries on CI

e87bfce

hoolioh force-pushed the julio/use-builder-cargo-install branch from 80e8dc3 to e87bfce Compare May 4, 2026 10:56

hoolioh added 3 commits May 4, 2026 13:18

chore: add check for the toolchain and instructions for installing it

882464e

chore: remove duplicated normalization

a1959d4

chore: remove ext folder

fcda2e5

lloeki mentioned this pull request May 4, 2026

Build using libdatadog's builder crate #33

Merged

hoolioh added 2 commits May 5, 2026 09:56

chore: hardening against supply chain attacks

72a50d2

refactor: merge #33 approach into the branch

d9128e6

hoolioh requested a review from a team as a code owner May 5, 2026 08:50

hoolioh closed this May 8, 2026

		rustc_output = `rustc -vV`
		raise "rustc not found or failed" unless $?.success?

Conversation

hoolioh commented Mar 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Build libdatadog using builder crate

Why?

What does this PR do?

Structure (from #33)

Security (from #24)

How to test the change?

Additional Notes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

To be clear this is a blocker in my opinion for merging this PR

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ivoanjo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

To be clear this is a blocker in my opinion for merging this PR

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

hoolioh commented Mar 27, 2026 •

edited

Loading