Skip to content

[PROF-11035] Add Ruby 3.4.0-rc1 headers#8

Merged
ivoanjo merged 4 commits into
masterfrom
ivoanjo/prof-11035-ruby-3.4.0-rc1-headers
Dec 13, 2024
Merged

[PROF-11035] Add Ruby 3.4.0-rc1 headers#8
ivoanjo merged 4 commits into
masterfrom
ivoanjo/prof-11035-ruby-3.4.0-rc1-headers

Conversation

@ivoanjo

@ivoanjo ivoanjo commented Dec 13, 2024

Copy link
Copy Markdown
Member

What does this PR do?

This PR adds the headers from the recently-released 3.4.0-rc1 release and also deletes the headers from 3.4.0-preview1.

Motivation:

There was a change to the layout of the rb_thread_t * structure after -preview2 that breaks the profiler's ability of reading the thread name.

I've locally tested and these headers fix the issue, and hopefully will turn out to be 100% compatible with the 3.4.0 stable release, giving us good support from day 0.

I've also chosen to delete 3.4.0-preview1 because it's no longer needed, as our CI is using preview2 or later. This also means that the total size of the gem should roughly stay the same, rather than growing.

Additional Notes:

I've also submitted an upstream PR to debase-ruby_core_source: ruby-debug/debase-ruby_core_source#14 as I was doing the work anyway.

How to test the change?

Install ruby-3.4.0-rc1. Point dd-trace-rb's Gemfile to this PR. Validate that all profiler unit tests are green.

These are only useful during development, and we no longer need to
keep around such an old development snapshot.
These were the output of running
`bundle exec rake add_source VERSION=3.4.0-rc`
Also manually removed the `*.inc` files.
@ivoanjo
ivoanjo requested review from a team and AlexJF December 13, 2024 09:37

@lloeki lloeki left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I understand

  • 3.4.0-preview1 is removed
  • 3.4.0-preview2 is kept
  • 3.4.0-rc1 is added

So it's all good with images that have a Ruby returning 3.4.0-preview2 as RUBY_VERSION?

If you would be so kind as to remove the empty commit from the history that would be nice.

Comment thread Gemfile Outdated
@ivoanjo
ivoanjo force-pushed the ivoanjo/prof-11035-ruby-3.4.0-rc1-headers branch from f76609b to c18db08 Compare December 13, 2024 13:47
As suggested during PR review

Co-authored-by: Loic Nageleisen <[email protected]>
@ivoanjo

ivoanjo commented Dec 13, 2024

Copy link
Copy Markdown
Member Author

So it's all good with images that have a Ruby returning 3.4.0-preview2 as RUBY_VERSION?

This was actually my intention; yet on re-testing it looks like the matching logic we inherited from debase doesn't work very well.

Regardless of me trying to build on -rc1, -preview2 or even -preview1, it always picks the -rc1 headers. This doesn't seem new; I checked with the previous version of the gem that shipped -preview1 and -preview2, and compiling on -preview1 still picks the -preview2 headers -- it's just that nobody ever noticed because they were actually compatible, whereas the preview headers are not compatible with rc headers and vice-versa.

Let me see if I can include a little tweak to fix this...

@ivoanjo

ivoanjo commented Dec 13, 2024

Copy link
Copy Markdown
Member Author

Let me see if I can include a little tweak to fix this...

Actually, since is not new/related to this PR, I'll merge this PR and open a separate one to tackle this issue.

@ivoanjo
ivoanjo merged commit a6aaf8d into master Dec 13, 2024
@ivoanjo
ivoanjo deleted the ivoanjo/prof-11035-ruby-3.4.0-rc1-headers branch December 13, 2024 14:18
ivoanjo added a commit that referenced this pull request Dec 13, 2024
**What does this PR do?**

This PR fixes an issue that we spotted in #8: that the logic we
inherited for matching on header folders for beta versions of Ruby
was a bit buggy.

Specifically, for preview/rc versions of Ruby, the "preview"/"rc"
part is (as far as I know) only part of the `RUBY_DESCRIPTION` and
does not show up anywhere else (including the `RUBY_VERSION`).

This meant that as we are shipping for instance headers for
* `ruby-3.4.0-preview2`
* `ruby-3.4.0-rc1`

The `RUBY_VERSION` for Ruby 3.4.0-preview2 is 3.4.0, and thus the
fuzzy matching logic would pick the "latest" option out of the
multiple options for 3.4.0, which would mean the 3.4.0-rc1 headers
were used for 3.4.0-preview2.

The way `ruby_core_source` version matching works is in two passes:
1. It first checks if there's an exact hit
2. If not, it falls back to a fuzzy version matching logic

By including the `preview`/`rc` in the string, my change affects 1:
if there's an exact hit on headers (e.g. `preview2` on `preview2`),
then those get used.

**Motivation:**

This change is useful since the 3.4.0-preview2 headers are not
compatible with 3.4.0-rc1 and vice-versa; so this way we can
continue to support both versions until stable 3.4.0 is out.

**Additional Notes:**

My change does not touch the fuzzy matching, e.g. for `preview1`,
since we have no exact match, the latest ones (`-rc1`) will get used.

I believe that there's no use in being "too clever" here.
If we have ruby 3.3.5 headers and 3.4.0-preview2, and we're
in `preview1`, what's the right answer of what to pick?

I argue there's no right answer here, so I didn't touch this
part of the logic.

Two other notes:

1. `RUBY_PATCHLEVEL < 0` is only for beta releases, so this
   change is not expected to impact any other case

2. The `$stderr.puts` is hidden by default during installation, so
   it's fine to leave like that (there's other similar prints in
   the profiler's `extconf.rb`)

**How to test the change?**

I've tested this manually with dd-trace-rb in different Ruby
versions.

```
$ ruby -v
ruby 3.4.0rc1 (2024-12-12 master 29caae9991) +PRISM [x86_64-linux]
Using datadog-ruby_core_source headers from datadog-ruby_core_source/lib/datadog/ruby_core_source/ruby-3.4.0-rc1

$ ruby -v
ruby 3.4.0preview2 (2024-10-07 master 32c733f57b) +PRISM [x86_64-linux]
Using datadog-ruby_core_source headers from datadog-ruby_core_source/lib/datadog/ruby_core_source/ruby-3.4.0-preview2

$ ruby -v
ruby 3.4.0preview1 (2024-05-16 master 9d69619623) [x86_64-linux]
Using datadog-ruby_core_source headers from datadog-ruby_core_source/lib/datadog/ruby_core_source/ruby-3.4.0-rc1
```
@ivoanjo

ivoanjo commented Dec 13, 2024

Copy link
Copy Markdown
Member Author

Fix for version matching is in #11

ivoanjo added a commit that referenced this pull request Dec 16, 2024
…roup

**What does this PR do?**

This PR removes the optional development group with the `pry` gem.
It was added in #8, but it being optional breaks the "publish gem"
github flow with:

```
Run bundle exec rake release
bundler: failed to load command: rake (/opt/hostedtoolcache/Ruby/3.2.4/x64/bin/rake)
/opt/hostedtoolcache/Ruby/3.2.4/x64/lib/ruby/3.2.0/bundler/rubygems_integration.rb:308:in `block in replace_bin_path': can't find executable rake for gem rake. rake is not currently included in the bundle, perhaps you meant to add it to your Gemfile? (Gem::Exception)
```

I was able to reproduce the same issue locally, it happens because
the development group was marked optional.

Since this has been a bit of a back and forth, for now let's
just remove `pry` and we can add it back locally or whatnot if
when needed.

**Motivation:**

This is blocking the 3.3.7 release, see
https://github.com/DataDog/datadog-ruby_core_source/actions/runs/12354204200/job/34475175057#step:4:5

**Additional Notes:**

N/A

**How to test the change?**

Without this PR, locally running `bundle exec rake` fails for me
with the error above; with this change, I'm able to run `rake`
successfully.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants