From 54977840dbc4450f788e95c1abd78ca88ccdab04 Mon Sep 17 00:00:00 2001 From: Ivo Anjo Date: Mon, 4 Mar 2024 10:34:37 +0000 Subject: [PATCH 1/3] Refactor `current_platform` to separate method --- ruby/lib/libdatadog.rb | 36 +++++++++++++++++++++--------------- 1 file changed, 21 insertions(+), 15 deletions(-) diff --git a/ruby/lib/libdatadog.rb b/ruby/lib/libdatadog.rb index 970d1deb4..3a620db2c 100644 --- a/ruby/lib/libdatadog.rb +++ b/ruby/lib/libdatadog.rb @@ -9,9 +9,25 @@ def self.available_binaries end def self.pkgconfig_folder(pkgconfig_file_name = "datadog_profiling_with_rpath.pc") - current_platform = Gem::Platform.local.to_s + current_platform = self.current_platform - if RbConfig::CONFIG["arch"].include?("-musl") && !current_platform.include?("-musl") + return unless available_binaries.include?(current_platform) + + pkgconfig_file = Dir.glob("#{vendor_directory}/#{current_platform}/**/#{pkgconfig_file_name}").first + + return unless pkgconfig_file + + File.absolute_path(File.dirname(pkgconfig_file)) + end + + private_class_method def self.vendor_directory + ENV["LIBDATADOG_VENDOR_OVERRIDE"] || "#{__dir__}/../vendor/libdatadog-#{Libdatadog::LIB_VERSION}/" + end + + def self.current_platform + platform = Gem::Platform.local.to_s + + if RbConfig::CONFIG["arch"].include?("-musl") && !platform.include?("-musl") # Fix/workaround for https://github.com/DataDog/dd-trace-rb/issues/2222 # # Old versions of rubygems (for instance 3.0.3) don't properly detect alternative libc implementations on Linux; @@ -25,19 +41,9 @@ def self.pkgconfig_folder(pkgconfig_file_name = "datadog_profiling_with_rpath.pc # # See also https://github.com/rubygems/rubygems/pull/2922 and https://github.com/rubygems/rubygems/pull/4082 - current_platform = RbConfig::CONFIG["arch"] + RbConfig::CONFIG["arch"] + else + platform end - - return unless available_binaries.include?(current_platform) - - pkgconfig_file = Dir.glob("#{vendor_directory}/#{current_platform}/**/#{pkgconfig_file_name}").first - - return unless pkgconfig_file - - File.absolute_path(File.dirname(pkgconfig_file)) - end - - private_class_method def self.vendor_directory - ENV["LIBDATADOG_VENDOR_OVERRIDE"] || "#{__dir__}/../vendor/libdatadog-#{Libdatadog::LIB_VERSION}/" end end From 38708f22cc9d0d91a83ce337f26079e82a38243a Mon Sep 17 00:00:00 2001 From: Ivo Anjo Date: Mon, 4 Mar 2024 10:39:23 +0000 Subject: [PATCH 2/3] Update comments to reflect that rubygems doesn't normalize any more --- ruby/lib/libdatadog.rb | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/ruby/lib/libdatadog.rb b/ruby/lib/libdatadog.rb index 3a620db2c..4f7ec405f 100644 --- a/ruby/lib/libdatadog.rb +++ b/ruby/lib/libdatadog.rb @@ -28,16 +28,17 @@ def self.current_platform platform = Gem::Platform.local.to_s if RbConfig::CONFIG["arch"].include?("-musl") && !platform.include?("-musl") - # Fix/workaround for https://github.com/DataDog/dd-trace-rb/issues/2222 + # Fix/workaround for https://github.com/datadog/dd-trace-rb/issues/2222 # # Old versions of rubygems (for instance 3.0.3) don't properly detect alternative libc implementations on Linux; # in particular for our case, they don't detect musl. (For reference, Rubies older than 2.7 may have shipped with # an affected version of rubygems). # In such cases, we fall back to use RbConfig::CONFIG['arch'] instead. # - # Why not use RbConfig::CONFIG['arch'] always? Because Gem::Platform.local.to_s does some normalization we want - # in other situations -- for instance, it turns `x86_64-linux-gnu` to `x86_64-linux`. So for now we only add this - # workaround in a specific situation where we actually know it is wrong. + # Why not use RbConfig::CONFIG['arch'] always? Because Gem::Platform.local.to_s does some normalization that seemed + # useful in the past, although part of it got removed in https://github.com/rubygems/rubygems/pull/5852. + # For now we only add this workaround in a specific situation where we actually know it is wrong, but in the + # future it may be worth re-evaluating if we should move away from `Gem::Platform` altogether. # # See also https://github.com/rubygems/rubygems/pull/2922 and https://github.com/rubygems/rubygems/pull/4082 From 5f18bb3232f2900bc3984de7c8501d50378727c5 Mon Sep 17 00:00:00 2001 From: Ivo Anjo Date: Mon, 4 Mar 2024 11:03:32 +0000 Subject: [PATCH 3/3] [PROF-9257] Fix incorrect platform detection for x86_64-linux-gnu/aarch64-linux-gnu **What does this PR do?** This PR fixes platform detection not working correctly when the user's platform is `x86_64-linux-gnu`/`aarch64-linux-gnu`. This manifested to a user as the following error: > Profiling was requested but is not supported, profiling disabled: Your > ddtrace installation is missing support for the Continuous Profiler > because the `libdatadog` gem installed on your system is missing > binaries for your platform variant. > (Your platform: `x86_64-linux-gnu`) > (Available binaries: `x86_64-linux`, `x86_64-linux-musl`) In the past, rubygems did this normalization for us (we even had a comment mentioning it in the code!) but this was removed in https://github.com/rubygems/rubygems/pull/5852 for Rubygems 3.4+ I'm also bumping the `GEM_MAJOR_VERSION` in the `version.rb` file so I can release this fix as libdatadog 6.0.0.2.0 for Ruby as soon as this PR is merged. **Motivation:** Make sure our platform detection code is correct. **Additional Notes:** N/A **How to test the change?** This change includes test coverage. --- ruby/lib/libdatadog.rb | 9 +++++++++ ruby/lib/libdatadog/version.rb | 2 +- ruby/spec/libdatadog_spec.rb | 13 +++++++++++++ 3 files changed, 23 insertions(+), 1 deletion(-) diff --git a/ruby/lib/libdatadog.rb b/ruby/lib/libdatadog.rb index 4f7ec405f..049085863 100644 --- a/ruby/lib/libdatadog.rb +++ b/ruby/lib/libdatadog.rb @@ -27,6 +27,15 @@ def self.pkgconfig_folder(pkgconfig_file_name = "datadog_profiling_with_rpath.pc def self.current_platform platform = Gem::Platform.local.to_s + if platform.end_with?("-gnu") + # In some cases on Linux with glibc the platform includes a -gnu suffix. We normalize it to not have the suffix. + # + # Note: This should be platform = platform.delete_suffix("-gnu") but it doesn't work on legacy Rubies; once + # dd-trace-rb 2.0 is out we can simplify this. + # + platform = platform[0..-5] + end + if RbConfig::CONFIG["arch"].include?("-musl") && !platform.include?("-musl") # Fix/workaround for https://github.com/datadog/dd-trace-rb/issues/2222 # diff --git a/ruby/lib/libdatadog/version.rb b/ruby/lib/libdatadog/version.rb index 790347758..e4a715a9d 100644 --- a/ruby/lib/libdatadog/version.rb +++ b/ruby/lib/libdatadog/version.rb @@ -4,7 +4,7 @@ module Libdatadog # Current libdatadog version LIB_VERSION = "6.0.0" - GEM_MAJOR_VERSION = "1" + GEM_MAJOR_VERSION = "2" GEM_MINOR_VERSION = "0" GEM_PRERELEASE_VERSION = "" # remember to include dot prefix, if needed! private_constant :GEM_MAJOR_VERSION, :GEM_MINOR_VERSION, :GEM_PRERELEASE_VERSION diff --git a/ruby/spec/libdatadog_spec.rb b/ruby/spec/libdatadog_spec.rb index f433d3d8c..07dc3f3f4 100644 --- a/ruby/spec/libdatadog_spec.rb +++ b/ruby/spec/libdatadog_spec.rb @@ -89,6 +89,7 @@ def create_dummy_pkgconfig_file(pkgconfig_folder) # Fix for https://github.com/DataDog/dd-trace-rb/issues/2222 before do + allow(RbConfig::CONFIG).to receive(:[]).and_call_original allow(RbConfig::CONFIG).to receive(:[]).with("arch").and_return("x86_64-linux-musl") allow(Gem::Platform).to receive(:local).and_return("x86_64-linux") @@ -101,6 +102,18 @@ def create_dummy_pkgconfig_file(pkgconfig_folder) expect(Libdatadog.pkgconfig_folder).to eq "#{temporary_directory}/x86_64-linux-musl/some/folder/containing/the/pkgconfig/file" end end + + context "when platform ends with -gnu" do + let(:pkgconfig_folder) { "#{temporary_directory}/aarch64-linux/some/folder/containing/the/pkgconfig/file" } + + before do + allow(Gem::Platform).to receive(:local).and_return(Gem::Platform.new("aarch64-linux-gnu")) + end + + it "chops off the -gnu suffix and returns the folder containing the pkgconfig file for the non-gnu variant" do + expect(Libdatadog.pkgconfig_folder).to eq pkgconfig_folder + end + end end context "but not for the current platform" do