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

Add missing includes to fix build and LSP errors on Mac #3843

Merged
merged 1 commit into from
Aug 14, 2024

Conversation

AlexJF
Copy link
Contributor

@AlexJF AlexJF commented Aug 14, 2024

What does this PR do?

  • Add missing include to clock_id_noop.c.
  • Move ruby.h include to clock_id.h since that file already references VALUE.

Motivation:

Fix a compilation error on mac:

❯ rake compile                     
/opt/homebrew/bin/gmake install sitearchdir=../../../../lib sitelibdir=../../../../lib target_prefix=
compiling ../../../../ext/datadog_profiling_native_extension/clock_id_from_pthread.c
compiling ../../../../ext/datadog_profiling_native_extension/clock_id_noop.c
../../../../ext/datadog_profiling_native_extension/clock_id_noop.c:14:43: error: unknown type name 'DDTRACE_UNUSED'
thread_cpu_time_id thread_cpu_time_id_for(DDTRACE_UNUSED VALUE _thread) {
                                          ^
../../../../ext/datadog_profiling_native_extension/clock_id_noop.c:14:64: error: expected ')'
thread_cpu_time_id thread_cpu_time_id_for(DDTRACE_UNUSED VALUE _thread) {
                                                               ^
../../../../ext/datadog_profiling_native_extension/clock_id_noop.c:14:42: note: to match this '('
thread_cpu_time_id thread_cpu_time_id_for(DDTRACE_UNUSED VALUE _thread) {
                                         ^
../../../../ext/datadog_profiling_native_extension/clock_id_noop.c:18:37: error: unknown type name 'DDTRACE_UNUSED'
thread_cpu_time thread_cpu_time_for(DDTRACE_UNUSED thread_cpu_time_id _time_id) {
                                    ^
../../../../ext/datadog_profiling_native_extension/clock_id_noop.c:18:71: error: expected ')'
thread_cpu_time thread_cpu_time_for(DDTRACE_UNUSED thread_cpu_time_id _time_id) {
                                                                      ^
../../../../ext/datadog_profiling_native_extension/clock_id_noop.c:18:36: note: to match this '('
thread_cpu_time thread_cpu_time_for(DDTRACE_UNUSED thread_cpu_time_id _time_id) {

This is not a problem on CI or Ivo™️ builds because they'll use clock_id_from_pthread.c, not clock_id_noop.c.

As for the ruby.h include move to the header, it's mainly to address the clang LSP diagnostic:

image

Builds themselves work because all current includers of clock_id.h have already included ruby.h before.

Additional Notes:

How to test the change?

Unsure? Have a question? Request a review!

@github-actions github-actions bot added the profiling Involves Datadog profiling label Aug 14, 2024
@AlexJF AlexJF marked this pull request as ready for review August 14, 2024 09:52
@AlexJF AlexJF requested a review from a team as a code owner August 14, 2024 09:52
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.

👍 LGTM Thanks for keeping our 🍎s in working order :)

@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.84%. Comparing base (93a2891) to head (27acb52).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3843      +/-   ##
==========================================
- Coverage   97.84%   97.84%   -0.01%     
==========================================
  Files        1264     1264              
  Lines       75737    75737              
  Branches     3729     3729              
==========================================
- Hits        74108    74102       -6     
- Misses       1629     1635       +6     

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

@pr-commenter
Copy link

pr-commenter bot commented Aug 14, 2024

Benchmarks

Benchmark execution time: 2024-08-14 10:30:03

Comparing candidate commit 27acb52 in PR branch alexjf/fix-prof-build-mac with baseline commit 93a2891 in branch master.

Found 0 performance improvements and 1 performance regressions! Performance is the same for 22 metrics, 2 unstable metrics.

scenario:profiler - sample timeline=false

  • 🟥 throughput [-0.753op/s; -0.731op/s] or [-10.291%; -9.991%]

@AlexJF AlexJF merged commit a589a98 into master Aug 14, 2024
186 checks passed
@AlexJF AlexJF deleted the alexjf/fix-prof-build-mac branch August 14, 2024 10:30
@github-actions github-actions bot added this to the 2.3.0 milestone Aug 14, 2024
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