[PROF-4779] Publish libddprof as a Ruby gem on rubygems.org by ivoanjo · Pull Request #16 · DataDog/libddprof

ivoanjo · 2022-02-01T14:43:29Z

What does this PR do?

This PR adds the scaffolding for taking a libddprof binary release from GitHub, and releasing it as a Ruby gem on rubygems.org.

This allows us to use libddprof on the Ruby profiler shipped in dd-trace-rb in a quite simple way: the released library has almost no code, and effectively just distributes the release files in a way that can be easily consumed by a Ruby library.

The release is mostly-automated (detailed instructions are on README.md)

We need to copy-paste the release sha256 for libbprof, as well as bump the version
And then run docker-compose run push_to_rubygems

I will be the one responsible for pushing new releases (on behalf of the profiling team), but still anyone can do a release if needed.

In the future we may want to look at triggering this from CI so that new libddprof releases are automatically pushed on rubygems.org.

Motivation

Having libddprof on rubygems.org and managed directly by the Profiling team (e.g. me) decouples releasing and distribution of the dd-trace-rb library from libddprof.

Additional Notes

I've already pushed libddprof 0.2.0 on rubygems.org using this PR: https://rubygems.org/gems/libddprof .

The current setup pushes two "variants" of the gem -- one with no binaries, which is used as a "no-op" dependency for platforms that libddprof does not support, and a "fat" one for x86-64 Linux, that includes both the glibc, as well as musl library binaries inside.

How to test the change?

Testing for this will be done on the consumer side: in the Ruby profiler, which will run libddprof on different platforms and Ruby versions.

Right now it's empty, still need to add the magic part where we get the libddprof binaries!

lloeki

Made some comments, which are more like suggestions: nothing struck me, so LGTM, but this might need to evolve as platfoms are added and CI develops.

I would suggest quickly adding CI (however crude) to build and test gem packages, like there is for libddwaf.

lloeki · 2022-02-02T13:52:16Z

+        puts "Found #{target_file} matching the expected sha256, skipping download"
+        next
+      else
+        puts "Found #{target_file} BUT IT DID NOT MATCH THE EXPECTED sha256, skipping download"


Outputting the unexpected hash can be useful in practice, so that one can download a known legit file and have the correct hash to fill in.

Also, does this really skip the download? There's no next so it would fall through and proceed, correct?

I've added printing of the hash in 8d23e24 .

Also, does this really skip the download? There's no next so it would fall through and proceed, correct?

Oops, that was just too much copy pasta. The intention is to just redownload the file when the hash doesn't match, so the code was correct, but the logging was misleading. Fixed in the same commit as above.

lloeki · 2022-02-02T13:59:16Z

+end
+
+desc "Release all packaged gems"
+task push_to_rubygems: [


Seems uncanny to have the _to_rubygems suffix. Is there a conflicting push task preexisting? We could also name it push:all, WDYT?

While a bit uncommon, I like that it exactly matches the name of the docker invocation that calls it (docker-compose run push_to_rubygems), which was why I adopted this naming.

@lloeki

Thanks @lloeki for the idea :)

ivoanjo · 2022-02-02T16:38:52Z

I would suggest quickly adding CI (however crude) to build and test gem packages, like there is for libddwaf.

My plan was to handle the CI on the dd-trace-rb side, since we already have the multiple variants and libddprof can be validated in context with its user.

Since I don't expect libddprof to change often, other than updating the binaries shipped, I'm not entirely convinced it's worth the trouble of adding it on this side.

sanchda

Looks good to me. I tried to add some pedantic comments to make you feel like I contributed, but honestly I've got nothing. Thanks!

sanchda · 2022-02-02T18:19:00Z

+3. [ ] Update the <lib/libddprof/version.rb> file with the `LIB_VERSION` and `VERSION` to use
+4. [ ] Commit change, open PR, get it merged
+5. [ ] Release by running `docker-compose run push_to_rubygems`.
+    (When asked for rubygems credentials, check your local friendly 1Password.)


Since libddprof is a public-facing repo, it is meaningful to qualify this statement a little bit more (or less)? Strictly speaking, someone should know whether they do not have the credentials, but perhaps a Datadog employee may not realize that this is in fact directed at them.

No clue, just a random thought.

👍 agreed, clarified in 90af8a2 that these steps are for Datadog employees.

sanchda · 2022-02-02T18:35:25Z

+    {
+      file: "libddprof-x86_64-alpine-linux-musl.tar.gz",
+      sha256: "d519a6241d78260522624b8e79e98502510f11d5d9551f5f80fc1134e95fa146",
+      ruby_platform: "x86_64-linux-musl"


Pedantic, but will anyone be confused by seeing libddprof-x86_64-alpine-linux-musl on their non-Alpine musl system? As far as I know, the liddprof artifacts don't care. In fact, I wouldn't be surprised if the musl artifact works perfectly fine on glibc systems.

(irrelevant comment is irrelevant, sorry)

It's a good question!

Ideally we would upload each ruby_platform variant separately to rubygems.org, and Ruby would pick the right one. But there's a bunch of bugs around that and even if the bugs were fixed, we support really old Ruby versions that wouldn't be running the fixed versions.

So actually we're shipping both variants inside the same release on rubygems.org, and then the consumer will pick the right one:

def self.pkgconfig_folder current_platform = Gem::Platform.local.to_s return unless available_binaries.include?(current_platform) pkgconfig_file = Dir.glob("#{vendor_directory}/#{current_platform}/**/ddprof_ffi.pc").first # <--- magic happens here

But TL;DR I don't think this will confuse customers because they will only get libddprof through dd-trace-rb, which will use the code above to pick the right one and ignore the other.

sanchda · 2022-02-02T18:39:01Z

+  GEM_PRERELEASE_VERSION = ".beta3"
+  private_constant :GEM_MAJOR_VERSION, :GEM_MINOR_VERSION, :GEM_PRERELEASE_VERSION
+
+  # The gem version scheme is lib_version.gem_major.gem_minor[.prerelease].


Ah, this makes sense.

sanchda

Looks good to me! Please feel free to ignore any and all of my comments, they're pedantic/advisory only.

ivoanjo added 7 commits January 27, 2022 14:55

Bootstrap scaffolding for libddprof Ruby gem

06178a5

Right now it's empty, still need to add the magic part where we get the libddprof binaries!

Add Rake task for releasing gem including binary libddprof releases

f1c6da8

Add helper methods for consuming libddprof

6738318

Dockerize and document release process

1dbfad4

Fix flaky test due to filesystem ordering

fd55f0f

Gracefully handle cases where vendor directory does not exist

84c71fd

Bump gem version to beta2

7ead908

ivoanjo requested a review from lloeki February 1, 2022 15:07

lloeki approved these changes Feb 2, 2022

View reviewed changes

ivoanjo added 3 commits February 2, 2022 16:23

Adopt new versioning scheme, as suggested during PR review

0ea7bc4

Thanks @lloeki for the idea :)

Refactor .no_binaries? helper into its reverse .binaries?

cd59500

Improving logging when file did not match the expected hash

8d23e24

ivoanjo changed the title ~~Publish libddprof as a Ruby gem on rubygems.org~~ [PROF-4779] Publish libddprof as a Ruby gem on rubygems.org Feb 2, 2022

sanchda approved these changes Feb 2, 2022

View reviewed changes

Clarify that Ruby release instructions are for Datadog employees

90af8a2

ivoanjo merged commit 11b659c into main Feb 3, 2022

ivoanjo deleted the ivoanjo/scaffold-libddprof-ruby-gem branch February 3, 2022 11:46

Uh oh!

Conversation

ivoanjo commented Feb 1, 2022

What does this PR do?

Motivation

Additional Notes

How to test the change?

Uh oh!

lloeki left a comment

Choose a reason for hiding this comment

Uh oh!

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!

ivoanjo commented Feb 2, 2022

Uh oh!

sanchda 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!

sanchda left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants