Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Setup clangd-based native dev env via rake native_dev:setup #3245

Merged
merged 4 commits into from
Nov 20, 2023

Conversation

AlexJF
Copy link
Contributor

@AlexJF AlexJF commented Nov 7, 2023

What does this PR do?
Add some helpers to facilitate development on the native extensions of dd-trace-rb using the clangd language server.

  1. rake native_dev:setup task to generate the required compile_commands.json files through bear (following the clangd getting started guide).
  2. rake native_dev:clean task to cleanup the files generated by the previous task (and integration into global rake clean).
  3. Disable automatic clang formatting on the ext/* directories since the current source code format does not align with clang and it'll try to reformat whole files on every save.
  4. Add a .gitignore specific to native extensions to ignore the compilation-flags.json files created from this setup and keep them out of source control.

Motivation:
Auto complete, navigation and diagnostic support during native extension development:

image image image

Additional Notes:

How to test the change?

  1. Install bear.
  2. Run rake native_dev:setup.
  3. Setup clangd LSP support in your IDE: vscode | neovim | sublime.
  4. Check that completion and diagnostics work while editing files in ext/.

I tested it with neovim on my local setup.

For Datadog employees:

  • If this PR touches code that signs or publishes builds or packages, or handles
    credentials of any kind, I've requested a review from @DataDog/security-design-and-guidance.
  • This PR doesn't touch any of that.

Unsure? Have a question? Request a review!

@github-actions github-actions bot added the profiling Involves Datadog profiling label Nov 7, 2023
@AlexJF AlexJF force-pushed the alexjf/ruby-native-dev-stuff branch from e0bc04f to e1802a9 Compare November 7, 2023 17:18
@AlexJF AlexJF marked this pull request as ready for review November 7, 2023 17:20
@AlexJF AlexJF requested review from a team as code owners November 7, 2023 17:20
@AlexJF AlexJF force-pushed the alexjf/ruby-native-dev-stuff branch from e1802a9 to b18d2f8 Compare November 7, 2023 17:29
@codecov-commenter
Copy link

codecov-commenter commented Nov 7, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (3ef068c) 98.22% compared to head (66785dc) 98.22%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #3245   +/-   ##
=======================================
  Coverage   98.22%   98.22%           
=======================================
  Files        1253     1253           
  Lines       72198    72198           
  Branches     3350     3350           
=======================================
  Hits        70917    70917           
  Misses       1281     1281           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@ivoanjo ivoanjo left a comment

Choose a reason for hiding this comment

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

Left a few notes! I'm curious about the choice to develop in ext/ instead of in tmp/ 🤔

Rakefile Outdated
Comment on lines 605 to 613
Rake::ExtensionTask.new("ddtrace_profiling_native_extension.#{RUBY_VERSION}_#{RUBY_PLATFORM}") do |ext|
native_exts = []

native_exts << Rake::ExtensionTask.new("ddtrace_profiling_native_extension.#{RUBY_VERSION}_#{RUBY_PLATFORM}") do |ext|
ext.ext_dir = 'ext/ddtrace_profiling_native_extension'
end

Rake::ExtensionTask.new("ddtrace_profiling_loader.#{RUBY_VERSION}_#{RUBY_PLATFORM}") do |ext|
native_exts << Rake::ExtensionTask.new("ddtrace_profiling_loader.#{RUBY_VERSION}_#{RUBY_PLATFORM}") do |ext|
ext.ext_dir = 'ext/ddtrace_profiling_loader'
end
Copy link
Member

Choose a reason for hiding this comment

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

To be honest, I'm not sure the extra complexity to support the ddtrace_profiling_loader here and below is entirely worth it; after all, it's self-contained and received all of 5 commits since its creation.

Up to you ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd personally prefer to keep it. It looks less complex with the latest version and makes for full coverage and easy change if any other native extension appears in the future. But not a strong opinion.

Copy link
Member

Choose a reason for hiding this comment

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

Not a strong opinion either, so let's keep it!

Rakefile Outdated Show resolved Hide resolved
Copy link
Member

@ivoanjo ivoanjo left a comment

Choose a reason for hiding this comment

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

Thanks for the great contribution, LGTM 👍

Rakefile Outdated Show resolved Hide resolved
Rakefile Outdated Show resolved Hide resolved
directory tmp_dir_dd_native_dev
NATIVE_CLEAN << tmp_dir_dd_native_dev

compile_commands_task = file "#{ext.ext_dir}/compile_commands.json" => [tmp_dir_dd_native_dev] do |t|
Copy link
Member

Choose a reason for hiding this comment

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

I guess one downside of this approach is that it works only for a specific Ruby version, so if we need to switch around, we'll need to re-run the rake task every time (and remember to do so).

On the other hand, I think that's fine ATM -- I'm guessing it may be non-trivial to make this per-ruby-version.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I think doing some automation here may require either encoding the switch operation as a rake task or formally aligning on an official way to handle different Ruby versions (e.g. asdf) so we could use some kind of hook system.

Both options sound way too complicated for the benefit atm 😅

@AlexJF AlexJF merged commit 5906a20 into master Nov 20, 2023
218 checks passed
@AlexJF AlexJF deleted the alexjf/ruby-native-dev-stuff branch November 20, 2023 10:51
@github-actions github-actions bot added this to the 1.17.0 milestone Nov 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
profiling Involves Datadog profiling
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants