Skip to content

[PROF-11035] Fix Ruby version matching for preview releases#11

Merged
ivoanjo merged 1 commit into
masterfrom
ivoanjo/prof-11035-improve-beta-header-matching
Dec 16, 2024
Merged

[PROF-11035] Fix Ruby version matching for preview releases#11
ivoanjo merged 1 commit into
masterfrom
ivoanjo/prof-11035-improve-beta-header-matching

Conversation

@ivoanjo

@ivoanjo ivoanjo commented Dec 13, 2024

Copy link
Copy Markdown
Member

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

**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 requested review from a team December 13, 2024 15:08
Comment thread lib/datadog/ruby_core_source.rb
@ivoanjo
ivoanjo merged commit 0d367b1 into master Dec 16, 2024
@ivoanjo
ivoanjo deleted the ivoanjo/prof-11035-improve-beta-header-matching branch December 16, 2024 12:36
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.

2 participants