-
Notifications
You must be signed in to change notification settings - Fork 375
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
[PROF-6566] Fix ddtrace installation issue when users have CI=true #2378
Conversation
There's quite a lot of repetition here. I was about to add a new environment variable, but before doing so, I decided to refactor this a bit to avoid more copy-pasting. We could actually use the same technique in more places in our docker-compose.yml, but I decided to go with a smaller change for now.
**What does this PR do?**: In #2358 we changed how the profiling native extension is built in CI by adding the `-Werror` compiler option to turn warnings into errors. We did this automatically by checking if the `CI` environment variable was set to `true`. Unfortunately, this broke at least one user, see <DataDog/datadog-api-client-ruby#1137>. The warning that was triggered in that case was actually not relevant, see my investigation in <#2377>. The `CI=true` was never meant to trigger outside of our CI, but clearly it's too generic of a name for us to rely on that. To fix this, I've gone ahead and changed the configuration so that `-Werror` only gets added when `DD_PROFILING_CI=true`. I also changed our docker and CircleCI configurations to set this by default. **Motivation**: This issue affected one Datadog internal customer -- <DataDog/datadog-api-client-ruby#1137>. This should never happen -- we don't want any issues when compiling the profiling bits to affect ddtrace installation. **Additional Notes**: (Nothing) **How to test the change?**: Validate that `-Werror` only gets added when `DD_PROFILING_CI=true`. To trigger a warning for testing, try removing `DDTRACE_UNUSED` from function argument declarations, and you should see that the compiler starts failing due to that. --- Fixes #2377
Codecov Report
@@ Coverage Diff @@
## master #2378 +/- ##
==========================================
- Coverage 98.35% 98.34% -0.02%
==========================================
Files 1102 1102
Lines 59296 59296
==========================================
- Hits 58323 58313 -10
- Misses 973 983 +10
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
@@ -16,7 +16,7 @@ services: | |||
- redis | |||
- redis_old | |||
env_file: ./.env | |||
environment: | |||
environment: &common-environment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧹
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor: I think a gem-global, DD_CI
would probably suffice. Maybe DD_CI
is too short and prone to being used by clients as well (e.g. Dunkin' Donuts' CI pipeline? 🍩), so maybe DDTRACE_CI
(but how about Discount Drugmart's own tracing CI tests 🙃)?
Regardless, I'm happy to keep DD_PROFILING_CI
as well, no biggie.
OK I HAVE IT: |
I DON'T HAVE IT: |
Sounds reasonable (not the emoji ones, are you crazy man!?!?!?), I've pushed a rename to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sad approval without an emoji variable.
I'll add myself to the ☝🏼 list of people who ended up here because their CI can't do a |
@ben thanks for the ping! The fix will be released this week, and sorry again for the issue. |
Thanks for the patience, everyone! The fix has now been released as part of version 1.7.0. As always, let us know how we can help and keep the feedback coming! 😄 |
What does this PR do?:
In #2358 we changed how the profiling native extension is built in CI by adding the
-Werror
compiler option to turn warnings into errors.We did this automatically by checking if the
CI
environment variable was set totrue
.Unfortunately, this broke at least one user, see DataDog/datadog-api-client-ruby#1137. The warning that was triggered in that case was actually not relevant, see my investigation in #2377.
The
CI=true
was never meant to trigger outside of our CI, but clearly it's too generic of a name for us to rely on that.To fix this, I've gone ahead and changed the configuration so that
-Werror
only gets added whenDD_PROFILING_CI=true
. I also changed our docker and CircleCI configurations to set this by default.Motivation:
This issue affected one Datadog internal customer -- DataDog/datadog-api-client-ruby#1137.
This should never happen -- we don't want any issues when compiling the profiling bits to affect ddtrace installation.
Additional Notes:
(Nothing)
How to test the change?:
Validate that
-Werror
only gets added whenDD_PROFILING_CI=true
.To trigger a warning for testing, try removing
DDTRACE_UNUSED
from function argument declarations, and you should see that the compiler starts failing due to that.Fixes #2377